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

[Chen Gang]

Firstly, thank you very much for your reviewing, and I shall send patch
v3 within this week end (2015-03-15).


On 3/14/15 02:20, Richard Henderson wrote:
> On 03/12/2015 09:06 AM, Chen Gang wrote:
>> +#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);
>> +
>> +#define TILEGX_GEN_SAVE_TMP_3(func, rdst, x1, x2, x3) \
>> +    TILEGX_GEN_SAVE_PREP(rdst) \
>> +    func(dc->tmp_regcur->val, x1, x2, x3);
> 
> Again, I told you to get rid of all of these macros.  I even suggested how to
> do it, using a helper function:
> 
> static TCGv dest_gr(DisasContext *dc, uint8_t rdst)
> {
>     DisasContextTemp *tmp = dc->tmp_regcur;
>     tmp->idx = rdst;
>     tmp->val = tcg_temp_new_i64();
>     return tmp->val;
> }
> 
>> +static const char *reg_names[] = {
> 
> Again, const char * const reg_names.
> 
>> +/* This is the state at translation time.  */
>> +typedef struct DisasContext {
>> +    uint64_t pc;                   /* Current pc */
>> +    uint64_t exception;            /* Current exception, 0 means empty */
>> +
>> +    TCGv zero;                     /* For zero register */
>> +
>> +    struct {
>> +        unsigned char idx;         /* index */
>> +        TCGv val;                  /* value */
>> +    } *tmp_regcur,                 /* Current temporary registers */
>> +    tmp_regs[TILEGX_MAX_INSTRUCTIONS_PER_BUNDLE]; /* All temporary 
>> registers */
> 
> Again, I told you to name this structure.  See DisasContextTemp above.
> 
>> +static void gen_shl16insli(struct DisasContext *dc,
>> +                           uint8_t rdst, uint8_t rsrc, uint16_t uimm16)
>> +{
>> +    TCGv_i64 tmp = tcg_temp_new_i64();
>> +
>> +    qemu_log("shl16insli r%d, r%d, %llx\n", rdst, rsrc, (long long)uimm16);
>> +    tcg_gen_shli_i64(tmp, load_gr(dc, rsrc), 16);
>> +    TILEGX_GEN_SAVE_TMP_2(tcg_gen_ori_i64, rdst, tmp, uimm16);
>> +    tcg_temp_free_i64(tmp);
> 
> Again, you don't need the temporary here, since the one you will have created
> with dest_gr is sufficient.
> 

Oh, sorry for my carelessness for the 'Again' above.


>> +static void gen_ld(struct DisasContext *dc, uint8_t rdst, uint8_t rsrc)
>> +{
>> +    qemu_log("ld r%d, r%d\n", rdst, rsrc);
>> +    tcg_gen_qemu_ld_i64(load_gr(dc, rdst), load_gr(dc, rsrc),
>> +                        MMU_USER_IDX, MO_LEQ);
> 
> You're incorrectly loading into a source temporary.  You need to load into a
> temporary created by dest_gr.
> 

OK, thanks.

>> +    /* for tcg_gen_qemu_st_i64, rsrc and rdst are reverted */
>> +    tcg_gen_qemu_st_i64(load_gr(dc, rsrc), load_gr(dc, rdst),
>> +                        MMU_USER_IDX, MO_LEQ);
> 
> Huh?  The address is always the second argument to tcg_gen_qemu_st_i64.
> It would also probably be better if you named these arguments properly: there
> is no "rdst" for a store instruction.  They're called SrcA and SrcB in the
> architecture manual.
> 

OK, thanks. The comments is incorrect (misleading), and I will use SrcA
and SrcB.

>> +static void decode_insns_y0_10(struct DisasContext *dc,
>> +                               tilegx_bundle_bits bundle)
>> +{
>> +    uint8_t rsrc, rsrcb, rdst;
>> +
>> +    if (get_RRROpcodeExtension_Y0(bundle) == 2) {
> 
> Use the proper symbol, not the number 2.
> Which in this case is OR_RRR_5_OPCODE_Y0.
> 
> There is a very simple pattern to the naming in opcode_tilegx.h.
> The symbols are very easy to look up.
> 

OK, thanks.

>> +        rdst = (uint8_t)get_Dest_Y0(bundle);
>> +        rsrc = (uint8_t)get_SrcA_Y0(bundle);
>> +        rsrcb = (uint8_t)get_SrcB_Y0(bundle);
>> +        /* move Dest, SrcA */
>> +        if (rsrcb == TILEGX_R_ZERO) {
>> +            gen_move(dc, rdst, rsrc);
>> +            return;
>> +        }
> 
> I insist that you *not* special case these pseudo instructions from section
> 4.1.15, at least to start with.  It will be fantastically easier to review if
> we see the symbolic opcode name followed by tcg functions bearing the same 
> name.
> 

OK, I will remove all pseudo instructions.

> Compare the code that you wrote with the following:
> 
> static void decode_rrr_5_opcode_y0(DisasContext *dc, tilegx_bundle_bits 
> bundle)
> {
>   unsigned rsrca = get_SrcA_Y0(bundle);
>   unsigned rsrcb = get_SrcB_Y0(bundle);
>   unsigned rdest = get_Dest_Y0(bundle);
>   unsigned ext = get_RRROpcodeExtension_Y0(bundle);
> 
>   switch (ext) {
>   case OR_RRR_5_OPCODE_Y0:
>     gen_or(rdest, rsrca, rsrcb);
>     break;
> 
>   default:
>     qemu_log_mask(LOG_UNIMP, "UNIMP rrr_5_opcode_y0, extension %d\n", ext);
>     dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>     break;
>   }
> }
>

OK, thank you for your sample above, it is valuable to me.

 
>> +static void decode_insns_y1_1(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
>> +{
>> +    uint8_t rsrc = (uint8_t)get_SrcA_Y1(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_Y1(bundle);
>> +    int8_t imm8 = (int8_t)get_Imm8_Y1(bundle);
>> +
>> +    gen_addi(dc, rdst, rsrc, imm8);
>> +}
> 
> I think the naming on these functions should be better.  "y1_1" does not tell
> me much.  Better is to name them after the symbol in opcode_tilegx.h.  In this
> case the name would be decode_addi_opcode_y1.
> 

OK, thanks.

>> +static void decode_insns_y1_7(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_rrr_1_opcode_y1.
> 
>> +{
>> +    /* fnop */
>> +    if ((get_RRROpcodeExtension_Y1(bundle) == 0x03)
> 
> UNARY_RRR_1_OPCODE_Y1
> 
>> +        && (get_UnaryOpcodeExtension_Y1(bundle) == 0x08)
> 
> FNOP_UNARY_OPCODE_Y1
> 
>> +static void decode_insns_x0_1(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_addli_opcode_x0
> 

OK, thanks

>> +{
>> +    uint8_t rsrc = (uint8_t)get_SrcA_X0(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_X0(bundle);
>> +    int16_t imm16 = (int16_t)get_Imm16_X0(bundle);
>> +
>> +    /* moveli Dest, Imm16 */
>> +    if (rsrc == TILEGX_R_ZERO) {
> 
> No special case for zero.  Just do tcg_gen_addi_tl.
> 
>> +    qemu_log("in x0_1, unimplement %16.16llx\n", bundle);
>> +    dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENT;
>> +}
> 
> which means there's nothing unimplemented.  See how much extra work you're
> making for yourself?
> 

OK, thanks.

>> +static void decode_insns_x0_4(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_imm8_opcode_x0
> 
>> +    uint8_t rsrc = (uint8_t)get_SrcA_X0(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_X0(bundle);
>> +    int8_t imm8 = (int8_t)get_Imm8_X0(bundle);
>> +
>> +    switch (get_Imm8OpcodeExtension_X0(bundle)) {
>> +    /* addi Dest, SrcA, Imm8 */
>> +    case 0x01:
> 
> ADDI_IMM8_OPCODE_X0.  The comment is then obvious and superfluous.
> 
>> +        gen_addi(dc, rdst, rsrc, imm8);
>> +        return;
>> +    /* andi Dest, SrcA, Imm8 */
>> +    case 0x03:
> 
> ANDI_IMM8_OPCODE_X0
> 
>> +static void decode_insns_x0_5(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_rrr_0_opcode_x0
> 
>> +{
>> +    uint8_t rsrc = (uint8_t)get_SrcA_X0(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_X0(bundle);
>> +
>> +    switch (get_RRROpcodeExtension_X0(bundle)) {
>> +    case 0x03:
>> +        /* add, Dest, SrcA, SrcB */
> 
> ADD_RRR_0_OPCODE_X0
> 
>> +        gen_add(dc, rdst, rsrc, (uint8_t)get_SrcB_X0(bundle));
>> +        return;
>> +    case 0x52:
>> +        /* fnop */
> 
> UNARY_RRR_0_OPCODE_X0.
> 
>> +        if ((get_UnaryOpcodeExtension_X0(bundle) == 0x03) && !rsrc && 
>> !rdst) {
> 
> FNOP_UNARY_OPCODE_X0.

OK, thanks.

>                       There are enough of these it's probably worth splitting
> out decode of this to its own function.
> 

OK, I will add gen_fnop() for it.

>> +static void decode_insns_x0_7(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_shl16insli_opcode_x0
> 
>> +static void decode_insns_x1_0(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_addli_opcode_x1
> 
>> +{
>> +    uint8_t rsrc = (uint8_t)get_SrcA_X1(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_X1(bundle);
>> +    int16_t imm16 = (int16_t)get_Imm16_X1(bundle);
>> +
>> +    /* moveli Dest, Imm16 */
>> +    if (rsrc == TILEGX_R_ZERO) {
>> +        gen_moveli(dc, rdst, imm16);
>> +        return;
>> +    }
>> +    qemu_log("in x1_0, unimplement %16.16llx\n", bundle);
>> +    dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENT;
> 
> Again, no special case for R_ZERO.  Again, just implement the add.
> 
>> +static void decode_insns_x1_3(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_imm8_opcode_x1
> 
>> +{
>> +    uint8_t rsrc = (uint8_t)get_SrcA_X1(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_X1(bundle);
>> +    int8_t imm8 = (int8_t)get_Imm8_X1(bundle);
>> +
>> +    /* addi Dest, SrcA, Imm8 */
>> +    if (get_Imm8OpcodeExtension_X1(bundle) == 0x01) {
> 
> ADDI_IMM8_OPCODE_X1
> 
>> +static void decode_insns_x1_5(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_rrr_0_opcode_x1
> 
>> +{
>> +    uint8_t rsrc = (uint8_t)get_SrcA_X1(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_X1(bundle);
>> +
>> +    switch (get_RRROpcodeExtension_X1(bundle)) {
>> +    case 0x03:
>> +        /* add Dest, SrcA, SrcB */
> 
> ADD_RRR_0_OPCODE_X1
> 
>> +        gen_add(dc, rdst, rsrc, (uint8_t)get_SrcB_X1(bundle));
>> +        return;
>> +    case 0x35:
> 
> UNARY_RRR_0_OPCODE_X1
> 
>> +        switch (get_UnaryOpcodeExtension_X1(bundle)) {
>> +        case 0x0e:
>> +             /* jr SrcA */
> 
> JR_UNARY_OPCODE_X1
> 
>> +             if (!rdst) {
>> +                 gen_jr(dc, rsrc);
>> +                 return;
>> +             }
>> +             break;
>> +        case 0x1e:
>> +            /* lnk Dest */
> 
> LNK_UNARY_OPCODE_X1
> 
>> +static void decode_insns_x1_7(struct DisasContext *dc,
>> +                              tilegx_bundle_bits bundle)
> 
> decode_shl16insli_opcode
>

OK, thanks.
 
>> +/* by "grep _OPCODE_X0 opcode_tilegx.h | awk '{print $3, $1}' | sort -n" */
>> +static TileGXDecodeInsn decode_insns_x0[] = {
> 
> These tables are both pointless and incorrect.
> 
> Note that the Opcode_X0 field is only 3 bits wide.  This should have been your
> first hint that having 165 entries in the table could not be right.
> 

OK, thanks.

>> +    decode_insns_x0_1,  /* 1, ADDI_IMM8_OPCODE_X0
>> +                              ADDLI_OPCODE_X0
>> +                              ADDXSC_RRR_0_OPCODE_X0
>> +                              CNTLZ_UNARY_OPCODE_X0
>> +                              ROTLI_SHIFT_OPCODE_X0 */
> 
> ADDI_IMM8_OPCODE_X0 does not come from get_Opcode_X0, but from
> get_Imm8OpcodeExtension_X0.  ADDXSC_RRR_0_OPCODE_X0 does not come from
> get_Opcode_X0, but from get_RRROpcodeExtension_X0.  Etc.
> 

OK, thanks.

> 
>> +static void translate_x0(struct DisasContext *dc, tilegx_bundle_bits bundle)
>> +{
>> +    unsigned int opcode = get_Opcode_X0(bundle);
>> +
>> +    if (opcode > TILEGX_OPCODE_MAX_X0) {
>> +        dc->exception = TILEGX_EXCP_OPCODE_UNKNOWN;
>> +        qemu_log("unknown x0 opcode: %u for bundle: %16.16llx\n",
>> +                 opcode, bundle);
>> +        return;
>> +    }
>> +    if (!decode_insns_x0[opcode]) {
>> +        dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENT;
>> +        qemu_log("unimplement x0 opcode: %u for bundle: %16.16llx\n",
>> +                 opcode, bundle);
>> +        return;
>> +    }
>> +
>> +    dc->tmp_regcur = dc->tmp_regs + 0;
>> +    decode_insns_x0[opcode](dc, bundle);
> 
> Thus I think this function should be written
> 
> static void decode_x0(DisasContext *dc, tilegx_bundle_bits bundle)
> {
>     unsigned int opcode = get_Opcode_X0(bundle);
>     switch (opcode) {
>     case ADDLI_OPCODE_X0:
>         decode_addli_opcode_x0(dc, bundle);
>         break;
>     case RRR_0_opcode_X0:
>         decode_rrr_0_opcode_x0(dc, bundle);
>         break;
>     ...
>     default:
>         qemu_log_mask(LOG_UNIMP, "UNIMP x0, opcode %d\n", opcode);
>         dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>         break;
>     }
> }
>

OK, thanks.
 
>> +static void translate_x1(struct DisasContext *dc, tilegx_bundle_bits bundle)
>> +static void translate_y0(struct DisasContext *dc, tilegx_bundle_bits bundle)
>> +static void translate_y1(struct DisasContext *dc, tilegx_bundle_bits bundle)
>> +static void translate_y2(struct DisasContext *dc, tilegx_bundle_bits bundle)
> 
> And similarly for these others.
> 

OK, thanks.


Thanks.
-- 
Chen Gang

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