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

[Chen Gang S]

Firstly, thank you very much for your details patient work!

And sorry, I guess, I can not finish patch v2 within this month. I shall
try to finish patch v2 within 2015-03-15.

The details reply is below, please check, thanks.

On 2/25/15 01:55, Richard Henderson wrote:
> On 02/23/2015 09:53 PM, Chen Gang S wrote:
>> +        env->zero = 0;
> 
> I replied elsewhere about the zero register.
>
 
OK, thanks.

>> +#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.
> 

Yeah, load_gpr and dest_gpr in target-alpha are really cool!!

>> +static const char *reg_names[] = {
> 
> static const char * const reg_names[].
> 
> I.e. the array itself should also be const.
> 

OK, thanks.

> 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.
> 

OK, thanks, what you said above sound reasonable to me.

>> +     "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.
> 

For me, it is a simple way to let the structure only as a name space,
and tmp_regcur is always related with tmp_regs[], so we can let them
together, the code is still simple and clear enough.

> 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...
> 

OK, thanks. For my taste, "uint8_t" is still better, since the idx
should always be within 0x3f.

>> +static int gen_move(struct DisasContext *dc,
>> +                    unsigned char rdst, unsigned char rsrc)
> 
> Use a typedef and not the struct tag.
> 

OK, thanks. In my taste, I dislike "typedef struct foo foo;", but I
shall follow qemu styles for DisasContext.

>> +{
>> +    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.
> 

OK, thanks. I shall use it next

> 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.
> 

Yeah.

>> +static int gen_moveli(struct DisasContext *dc, unsigned char rdst, short 
>> im16)
> 
> Don't use "short", use "int16_t".
> 

OK.

>> +{
>> +    qemu_log("moveli r%d, %d", rdst, im16);
>> +    return gen_move_imm(dc, rdst, (int64_t)im16);
> 
> The cast is useless, implied by C.
> 

OK.

>> +static int gen_movei(struct DisasContext *dc, unsigned char rdst, char im8)
> 
> Don't use "char", use "int8_t".
> 

OK.

>> +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.
> 

OK, thanks. I shall read the details next, and implement it.

>> +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.
> 

Yeah.

>> +static int gen_shl16insli(struct DisasContext *dc,
>> +                          unsigned char rdst, unsigned char rsrc, short 
>> im16)
> 
> Use uint16_t, as the field is not signed.
> 

objdump print it as signed, e.g. "shl16insli r32, r32, -30680", and
tcg_gen_ori_i64 also use int64_t for it.

>> +{
>> +    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,
> 

objdump print it as signed.


>> +    TILEGX_GEN_CHK_SIMPLE(rdst);
>> +
>> +    imm = (int64_t)im16 & 0x000000000000ffffLL;
> 
> and you don't need the mask here.
> 

If im16 is signed, we have to use 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);
> 

Yeah, thanks.

>> +        if (rsrc == 0x3f) {
> 
> You might as well use TILEGX_R_ZERO, now that you've defined it.
> 

Yeah, thanks.

>> +            /* 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.
> 

OK, thanks. What you said above sounds reasonable to me.

>> +    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.
> 

OK, thanks What you said above sounds reasonable to me. But if we use
"arch/opcode_tilegx.h" next, we can use their own related inline
functions instead of.

>> +            /* 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.)
> 

OK, thanks. I shall try for it.

>> +        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.)
> 

For me, the most efficient way is: when we meet it, then fix it.

>> +    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.
> 

Oh, I forgot to provide related comment for them, in ISA.pdf, for x1,
movei is the pseudo instruction for addi when src is zero register.

>> +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.
> 

OK, thanks.

>> +
>> +    /* For can regress completely, it must be y0 -> y1 -> y2, or x0 -> x1. 
>> */
> 
> I don't know what this sentence means.
> 

I guess, it needs more contents:

  y2 and x1 may contents st instruction (x1 may also contents 'jal'),
  which should be executed at last, or when exception raised, we can
  not be sure each bundle must be atomic (none pipes should execute).

>> +    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.
> 

OK, thanks. I guess your meaning is: we need not check the return value
for each pipe, just raise exception is enough. e.g.

 - Add a flag in DisasContext.

 - When exception happens, set the flag.

 - In main looping gen_intermediate_code_internal(), if the flag is set,
   we need break the tb block, and return.

>> +    /* 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.
> 

OK, thanks. What you said above sounds reasonable to me.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed