Re: [Qemu-devel] [PATCH 09/10 v11] target-tilegx: Generate tcg instructions to finish "Hello world"

[Chen Gang]

After thinking of again, for me, I still prefer to keep gen_cntlz() and
others, the reason is below:

 - gen_* (include gen_cntlz) are used in multiple areas, and most gen_*
   are not single statement. For each gen_*, printing insns is easy (and
   may be helpful).

 - decode* is for switch opcode (as branch), not for implementation (as
   leaf).

 - After we use individual functions for all unary opcode extensions, we
   can let decode* very simple (although they may have long code), if we
   let decode* printing insns, it will let them seem a little complex.


Thanks.

On 6/2/15 04:54, Chen Gang wrote:
>> I can't help thinking, as I read all of these decode functions, that it would
>> > be better if the output disassembly, i.e. qemu_log_mask(CPU_LOG_TB_IN_ASM, 
>> > *),
>> > were to happen here, instead of being spread across 99 other functions.
>> > 
>> > This has a side effect of reducing many of your functions to a single
>> > statement, invoking another tcg generator, at which point it's worth 
>> > inlining them.
>> > 
> OK, thanks.
> 
>> > For example:
>> > 
>> > static void decode_rrr_1_unary_y0(struct DisasContext *dc,
>> >                                   tilegx_bundle_bits bundle,
>> >                                   uint8_t rdst, uint8_t rsrc)
>> > {
>> >     unsigned ext = get_UnaryOpcodeExtension_Y0(bundle);
>> >     const char *mnemonic;
>> >     TCGv vdst, vsrc;
>> > 
>> >     if (ext == NOP_UNARY_OPCODE_Y0 || ext == FNOP_UNARY_OPCODE_Y0) {
>> >         if (rsrc != 0 || rdst != 0) {
>> >             goto unimplemented;
>> >         }
>> >         qemu_log_mask(CPU_LOG_TB_IN_ASM, "(f)nop\n");
>> >         return;
>> >     }
>> > 
>> >     vdst = dest_gr(dc, rdst);
>> >     vsrc = load_gr(dc, rsrc);
>> > 
>> >     switch (ext) {
>> >     case CNTLZ_UNARY_OPCODE_Y0:
>> >         gen_helper_cntlz(vdst, vsrc);
>> >         mnemonic = "cntlz";
>> >         break;
>> >     case CNTTZ_UNARY_OPCODE_Y0:
>> >         gen_helper_cnttz(vdst, vsrc);
>> >         mnemonic = "cnttz";
>> >         break;
>> >     case FSINGLE_PACK1_UNARY_OPCODE_Y0:
>> >     case PCNT_UNARY_OPCODE_Y0:
>> >     case REVBITS_UNARY_OPCODE_Y0:
>> >     case REVBYTES_UNARY_OPCODE_Y0:
>> >     case TBLIDXB0_UNARY_OPCODE_Y0:
>> >     case TBLIDXB1_UNARY_OPCODE_Y0:
>> >     case TBLIDXB2_UNARY_OPCODE_Y0:
>> >     case TBLIDXB3_UNARY_OPCODE_Y0:
>> >     default:
>> >     unimplemented:
>> >         qemu_log_mask(LOG_UNIMP, "UNIMP rrr_1_unary_y0, [" FMT64X "]\n",
>> >                       bundle);
>> >         dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>> >         return;
>> >     }
>> > 
>> >     qemu_log_mask(CPU_LOG_TB_IN_ASM, "%s r%d,r%d\n",
>> >                   mnemonic, rdst, rsrc);
>> > }
>> > 
>> > static void decode_rrr_1_opcode_y0(struct DisasContext *dc,
>> >                                    tilegx_bundle_bits bundle)
>> > {
>> >     unsigned ext = get_RRROpcodeExtension_Y0(bundle);
>> >     uint8_t rsrca = get_SrcA_Y0(bundle);
>> >     uint8_t rsrcb = get_SrcB_Y0(bundle);
>> >     uint8_t rdst = get_Dest_Y0(bundle);
>> >     const char *mnemonic;
>> >     TCGv vdst, vsrca, vsrcb;
>> > 
>> >     if (ext == UNARY_RRR_1_OPCODE_Y0) {
>> >         decode_rrr_1_unary_y0(dc, bundle, rdst, rsrc);
>> >         return;
>> >     }
>> > 
>> >     vdst = dest_gr(dc, rdst);
>> >     vsrca = load_gr(dc, rsrca);
>> >     vsrcb = load_gr(dc, rsrcb);
>> > 
>> >     switch (ext) {
>> >     case SHL1ADD_RRR_1_OPCODE_Y0:
>> >         gen_shladd(vdst, vsrca, vsrcb, 1, 0);
>> >         mnemonic = "shl1add";
>> >         break;
>> >     case SHL2ADD_RRR_1_OPCODE_Y0:
>> >         gen_shladd(vdst, vsrca, vsrcb, 2, 0);
>> >         mnemonic = "shl2add";
>> >         break;
>> >     case SHL3ADD_RRR_1_OPCODE_Y0:
>> >         gen_shladd(vdst, vsrca, vsrcb, 3, 0);
>> >         mnemonic = "shl3add";
>> >         break;
>> >     default:
>> >         qemu_log_mask(LOG_UNIMP, "UNIMP rrr_1_opcode_y0, [" FMT64X "]\n",
>> >                       bundle);
>> >         dc->exception = TILEGX_EXCP_OPCODE_UNIMPLEMENTED;
>> >         return;
>> >     }
>> >     qemu_log_mask(CPU_LOG_TB_IN_ASM, "%s r%d,r%d,r%d\n",
>> >                   mnemonic, rdst, rsrca, rsrcb);
>> > }
>> > 
> OK, thank you very much.
> 

-- 
Chen Gang

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