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

[Richard Henderson]

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.

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

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

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

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

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;
  }
}

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

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

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

> +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.  There are enough of these it's probably worth splitting
out decode of this to its own function.

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

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

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


> +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;
    }
}

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


r~