Re: [Qemu-devel] [PATCH] target-tilegx: Execute _start and reach to __libc_start_main successfully

[Richard Henderson]

On 02/23/2015 09:53 PM, Chen Gang S wrote:
> +        env->zero = 0;

I replied elsewhere about the zero register.

> +#define TILEGX_GEN_CHK_BEGIN(x) \
> +    if ((x) == TILEGX_R_ZERO) {
> +#define TILEGX_GEN_CHK_END(x) \
> +        return 0; \
> +    } \
> +    if ((x) >= TILEGX_R_COUNT) { \
> +        return -1; \
> +    }
> +
> +#define TILEGX_GEN_CHK_SIMPLE(x) \
> +    TILEGX_GEN_CHK_BEGIN(x) \
> +    TILEGX_GEN_CHK_END(x)
> +
> +#define TILEGX_GEN_SAVE_PREP(rdst) \
> +    dc->tmp_regcur->idx = rdst; \
> +    dc->tmp_regcur->val = tcg_temp_new_i64();
> +
> +#define TILEGX_GEN_SAVE_TMP_1(func, rdst, x1) \
> +    TILEGX_GEN_SAVE_PREP(rdst) \
> +    func(dc->tmp_regcur->val, x1);
> +
> +#define TILEGX_GEN_SAVE_TMP_2(func, rdst, x1, x2) \
> +    TILEGX_GEN_SAVE_PREP(rdst) \
> +    func(dc->tmp_regcur->val, x1, x2);

I don't like these macros at all.  I think that proper functions would be
easier to read.  In particular, if you use helper functions like the load_gpr
and dest_gpr functions from target-alpha, you'll wind up with much more
readable code.

> +static const char *reg_names[] = {

static const char * const reg_names[].

I.e. the array itself should also be const.

Even better is to use

static const char reg_names[][4]

I.e. make an array of 4 byte arrays.  This will actually save space on 64-bit
hosts.

> +     "r0",  "r1",  "r2",  "r3",  "r4",  "r5",  "r6",  "r7",
> +     "r8",  "r9", "r10", "r11", "r12", "r13", "r14", "r15",
> +    "r16", "r17", "r18", "r19", "r20", "r21", "r22", "r23",
> +    "r24", "r25", "r26", "r27", "r28", "r29", "r30", "r31",
> +    "r32", "r33", "r34", "r35", "r36", "r37", "r38", "r39",
> +    "r40", "r41", "r42", "r43", "r44", "r45", "r46", "r47",
> +    "r48", "r49", "r50", "r51",  "bp",  "tp",  "sp",  "lr"
> +};


> +/* This is the state at translation time.  */
> +struct DisasContext {
> +    uint64_t pc;                   /* Current pc */
> +    unsigned int cpustate_changed; /* Whether cpu state changed */
> +
> +    struct {
> +        unsigned char idx;         /* index */
> +        TCGv val;                  /* value */
> +    } *tmp_regcur,                 /* Current temporary registers */
> +    tmp_regs[TILEGX_BUNDLE_INSNS]; /* All temporary registers */

I think the pointer is overkill, but whatever.  If you keep it, pull this
structure out and give it a proper name.

Please don't use "unsigned char".  "uint8_t" is better, though perhaps just
plain "int" is best.  Surely you're not trying to save space here...

> +static int gen_move(struct DisasContext *dc,
> +                    unsigned char rdst, unsigned char rsrc)

Use a typedef and not the struct tag.

> +{
> +    qemu_log("move r%d, r%d", rdst, rsrc);
> +    TILEGX_GEN_CHK_SIMPLE(rdst);
> +    TILEGX_GEN_CHK_BEGIN(rsrc);
> +        TILEGX_GEN_SAVE_TMP_1(tcg_gen_movi_i64, rdst, 0);
> +    TILEGX_GEN_CHK_END(rsrc);
> +
> +    TILEGX_GEN_SAVE_TMP_1(tcg_gen_mov_i64, rdst, cpu_regs[rsrc]);
> +    return 0;
> +}

Use the proper log flags.  In this case, CPU_LOG_TB_IN_ASM.

With helpers this becomes

static void gen_move(DisasContext *dc, int rdst, int rsrc)
{
    TCGv tdst = dest_gpr(dc, rdst);
    TCGv tsrc = load_gpr(dc, rsrc);
    tcg_gen_mov_tl(tdst, tsrc);
}

which is clearly much much easier to read than your macros.

> +static int gen_moveli(struct DisasContext *dc, unsigned char rdst, short 
> im16)

Don't use "short", use "int16_t".

> +{
> +    qemu_log("moveli r%d, %d", rdst, im16);
> +    return gen_move_imm(dc, rdst, (int64_t)im16);

The cast is useless, implied by C.

> +static int gen_movei(struct DisasContext *dc, unsigned char rdst, char im8)

Don't use "char", use "int8_t".

> +static int gen_ld(struct DisasContext *dc,
> +                  unsigned char rdst, unsigned char rsrc)
> +{
> +    qemu_log("ld r%d, r%d", rdst, rsrc);
> +    TILEGX_GEN_CHK_SIMPLE(rdst);
> +    TILEGX_GEN_CHK_BEGIN(rsrc);
> +        TILEGX_GEN_SAVE_TMP_2(tcg_gen_ld_i64, rdst, cpu_env, 0);
> +    TILEGX_GEN_CHK_END(rsrc);
> +
> +    TILEGX_GEN_SAVE_TMP_2(tcg_gen_ld_i64, rdst, cpu_env,
> +                          offsetof(CPUTLState, regs[rsrc]));

You're using the wrong kind of load.  Indeed, this is completely confused and
amounts to a broken sort of register-to-register move.

The kind of load you are generating is a host-side load.  What you actually
want is a guest-side load.

    tcg_gen_qemu_ld_i64(tdst, tsrc, dc->mem_idx, MO_TEQ)

You probably want to add a parameter of type TCGMemOp so that you can handle
all of the various loads with the same function.

> +static int gen_st(struct DisasContext *dc,
> +                  unsigned char rdst, unsigned char rsrc)
> +{
> +    qemu_log("st r%d, r%d", rdst, rsrc);
> +    TILEGX_GEN_CHK_SIMPLE(rdst);
> +    TILEGX_GEN_CHK_BEGIN(rsrc);
> +        tcg_gen_st_i64(cpu_regs[rdst], cpu_env, offsetof(CPUTLState, zero));
> +    TILEGX_GEN_CHK_END(rsrc);
> +
> +    tcg_gen_st_i64(cpu_regs[rdst], cpu_env, offsetof(CPUTLState, 
> regs[rsrc]));
> +    return 0;
> +}

Similarly, this should be using tcg_gen_qemu_st_i64.

> +static int gen_shl16insli(struct DisasContext *dc,
> +                          unsigned char rdst, unsigned char rsrc, short im16)

Use uint16_t, as the field is not signed.

> +{
> +    int64_t imm;
> +    TCGv_i64 tmp;
> +
> +    qemu_log("shl16insli r%d, r%d, %d", rdst, rsrc, im16);

which means you print the right value here,

> +    TILEGX_GEN_CHK_SIMPLE(rdst);
> +
> +    imm = (int64_t)im16 & 0x000000000000ffffLL;

and you don't need the mask here.

> +
> +    TILEGX_GEN_CHK_BEGIN(rsrc);
> +        TILEGX_GEN_SAVE_TMP_1(tcg_gen_movi_i64, rdst, imm);
> +    TILEGX_GEN_CHK_END(rsrc);
> +
> +    tmp = tcg_temp_new_i64();
> +    tcg_gen_shli_i64(tmp, cpu_regs[rsrc], 16);
> +    TILEGX_GEN_SAVE_TMP_2(tcg_gen_ori_i64, rdst, tmp, imm);
> +    tcg_temp_free_i64(tmp);

You don't need an extra temporary.  You know that dst will be a new temporary.

    tcg_gen_shli_i64(tdst, tsrc, 16);
    tcg_gen_ori_i64(tdst, tdst, imm);

> +        if (rsrc == 0x3f) {

You might as well use TILEGX_R_ZERO, now that you've defined it.

> +            /* moveli Dest, Imm16 */
> +            if (gen_moveli(dc, rdst, im16)) {
> +                /* FIXME: raise an exception for invalid instruction */
> +                return -1;

Please do not scatter these checks everywhere.  I'm not sure what you're trying
to accomplish with them.  Certainly gen_movli can't fail.  In particular, all
invalid instruction detection should happen here and the "gen" routines should
only be given valid inputs.

> +    case 0x0000000040000000ULL:
> +        switch (TILEGX_CODE_X0_20(bundle)) {
> +        /* andi Dest, SrcA, Imm8 */
> +        case 0x0000000000300000ULL:
> +            rdst = (unsigned char)(bundle & TILEGX_DATA_REGISTER);
> +            rsrc = (unsigned char)((bundle >> 6) & TILEGX_DATA_REGISTER);
> +            im8 = (char)((bundle >> 12) & TILEGX_DATA_IMM8);

Pull these field extracts to the top of the switch -- there are only a limited
amount of them that are possible for the X0 pipeline.

Use the extract64 and sextract64 functions instead of bare shifts and masks.

> +            /* FIXME: raise an exception for invalid instruction */
> +            qemu_log("20 bundle value: %16.16llx", 
> TILEGX_CODE_X0_20(bundle));

You might as well get this right from the very beginning.  It is very easy to
define a helper to throw an exception.  (Harder to consume the exception,
perhaps, but "consume" can simply abort for now.)

> +        case 0x0000000001040000ULL:
> +            rdst = (unsigned char)(bundle & TILEGX_DATA_REGISTER);
> +            rsrc = (unsigned char)((bundle >> 6) & TILEGX_DATA_REGISTER);
> +            rsrcb = (unsigned char)((bundle >> 12) & TILEGX_DATA_REGISTER);
> +            /* move Dest, Src */
> +            if (rsrcb == 0x3f) {
> +                if (gen_move(dc, rdst, rsrc)) {
> +                    /* FIXME: raise an exception for invalid instruction */
> +                    return -1;
> +                }
> +            } else {
> +                qemu_log("invalid rsrcb, not 0x3f for move in x0");
> +            }

Um, what instruction is this supposed to be?  (Sadly, the tilegx architecture
pdf that I have doesn't have instructions indexed by opcode, so it's hard for
me to actually look this up.)

> +    case 0x1800000000000000ULL:
> +        switch (TILEGX_CODE_X1_51(bundle)) {
> +        /* addi Dest, SrcA, Imm8 */
> +        case 0x0008000000000000ULL:
> +            rdst = (unsigned char)((bundle >> 31) & TILEGX_DATA_REGISTER);
> +            rsrc = (unsigned char)((bundle >> 37) & TILEGX_DATA_REGISTER);
> +            im8 = (char)((bundle >> 43) & TILEGX_DATA_IMM8);
> +            if (rsrc == 0x3f) {
> +                if (gen_movei(dc, rdst, im8)) {
> +                    /* FIXME: raise an exception for invalid instruction */
> +                    return -1;
> +                }
> +            } else {
> +                if (gen_addi(dc, rdst, rsrc, im8)) {
> +                    /* FIXME: raise an exception for invalid instruction */
> +                    return -1;
> +                }
> +            }

Since this is the 3rd copy of this, clearly the movei optimization should be
moved into gen_addi, and gen_movei should be removed.

> +static int translate_one_bundle(struct DisasContext *dc, uint64_t bundle)
> +{
> +    int i, ret = 0;
> +    TCGv tmp;
> +
> +    for (i = 0; i < TILEGX_BUNDLE_INSNS; i++) {
> +        dc->tmp_regs[i].idx = TILEGX_R_NOREG;
> +        TCGV_UNUSED_I64(dc->tmp_regs[i].val);
> +    }

Emit

    if (unlikely(qemu_loglevel_mask(CPU_LOG_TB_OP | CPU_LOG_TB_OP_OPT))) {
        tcg_gen_debug_insn_start(dc->pc);
    }

here.

> +
> +    /* For can regress completely, it must be y0 -> y1 -> y2, or x0 -> x1. */

I don't know what this sentence means.

> +    if (TILEGX_BUNDLE_TYPE_Y(bundle)) {
> +        dc->tmp_regcur = dc->tmp_regs + 0;
> +        ret = translate_y0_bundle(dc, TILEGX_PIPE_Y0(bundle));
> +        if (ret) {
> +            goto err;
> +        }

You don't need these gotos.  If a given pipeline raises an invalid instruction
exception, then it's true that all code emitted afterward is dead.  But that's
ok, since it simply won't be executed.  Invalid instructions are exceedingly
unlikely and it's silly to optimize for that case.

> +    /* FIXME: do not consider about search_pc firstly. */

You have to do this right away.  This path will be used the very first time you
execute a load or store instruction.

> +err:
> +    exit(-1);
>  }

Huh?


r~