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
