On 4/10/15 06:19, Peter Maydell wrote: > On 27 March 2015 at 11:07, Chen Gang <xili_gchen_5...@hotmail.com> wrote:
[...] >> >> +static void gen_fnop(void) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "(f)nop\n"); > > I really don't much like mixing a fake disassembler in > with the translator. If you want a disassembler, write one > and put it in disas/... > For me, it is necessary (for I am not quite sure whether my disassemble must be correct). It is only for tracing. >> +} >> + >> +static void gen_cmpltui(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, int8_t imm8) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpltui r%d, r%d, %d\n", >> + rdst, rsrc, imm8); >> + tcg_gen_setcondi_i64(TCG_COND_LTU, dest_gr(dc, rdst), load_gr(dc, rsrc), >> + (uint64_t)imm8); >> +} >> + >> +static void gen_cmpeqi(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, int8_t imm8) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpeqi r%d, r%d, %d\n", rdst, rsrc, >> imm8); >> + tcg_gen_setcondi_i64(TCG_COND_EQ, dest_gr(dc, rdst), load_gr(dc, rsrc), >> + (uint64_t)imm8); >> +} >> + >> +static void gen_cmpne(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, uint8_t rsrcb) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpne r%d, r%d, r%d\n", >> + rdst, rsrc, rsrcb); >> + tcg_gen_setcond_i64(TCG_COND_NE, dest_gr(dc, rdst), load_gr(dc, rsrc), >> + load_gr(dc, rsrcb)); >> +} >> + >> +static void gen_cmoveqz(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, uint8_t rsrcb) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmoveqz r%d, r%d, r%d\n", >> + rdst, rsrc, rsrcb); >> + tcg_gen_movcond_i64(TCG_COND_EQ, dest_gr(dc, rdst), load_gr(dc, rsrc), >> + load_zero(dc), load_gr(dc, rsrcb), load_gr(dc, >> rdst)); >> +} >> + >> +static void gen_cmovnez(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, uint8_t rsrcb) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmovnez r%d, r%d, r%d\n", >> + rdst, rsrc, rsrcb); >> + tcg_gen_movcond_i64(TCG_COND_NE, dest_gr(dc, rdst), load_gr(dc, rsrc), >> + load_zero(dc), load_gr(dc, rsrcb), load_gr(dc, >> rdst)); >> +} > > This is hugely repetitive. Write a common function that takes a > TCG_COND_* as a parameter. > OK, thanks. >> + >> +static void gen_add(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, uint8_t rsrcb) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "add r%d, r%d, r%d\n", >> + rdst, rsrc, rsrcb); >> + tcg_gen_add_i64(dest_gr(dc, rdst), load_gr(dc, rsrc), load_gr(dc, >> rsrcb)); >> +} >> + >> +static void gen_addimm(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, int64_t imm) >> +{ >> + tcg_gen_addi_i64(dest_gr(dc, rdst), load_gr(dc, rsrc), imm); >> +} >> + >> +static void gen_addi(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, int8_t imm8) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "addi r%d, r%d, %d\n", rdst, rsrc, >> imm8); >> + gen_addimm(dc, rdst, rsrc, (int64_t)imm8); > > This cast is pointless, because the 'imm' argument to > gen_addimm() already has type int64_t. > OK, thanks. >> +} >> + >> +static void gen_addli(struct DisasContext *dc, >> + uint8_t rdst, uint8_t rsrc, int16_t im16) >> +{ >> + qemu_log_mask(CPU_LOG_TB_IN_ASM, "addli r%d, r%d, %d\n", rdst, rsrc, >> im16); >> + gen_addimm(dc, rdst, rsrc, (int64_t)im16); > > Another pointless cast. > OK, thanks. [...] >> + >> +/* >> + * The related functional description for bfextu in isa document: >> + * >> + * uint64_t mask = 0; >> + * mask = (-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1); >> + * uint64_t rot_src = (((uint64_t) rf[SrcA]) >> BFStart) >> + * | (rf[SrcA] << (64 - BFStart)); >> + * rf[Dest] = rot_src & mask; >> + */ >> +static void gen_bfextu(struct DisasContext *dc, uint8_t rdst, uint8_t rsrc, >> + int8_t start, int8_t end) >> +{ >> + uint64_t mask = (-1ULL) ^ ((-1ULL << ((end - start) & 63)) << 1); >> + TCGv tmp = dest_gr(dc, rdst); > > Are the start and end immediates here limited such that we're > guaranteed not to hit any of C's undefined behaviour for out > of range shifts, and that we don't hit TCG's undefined-value > behaviour on bad rotates? > For me, it is correct, it is only the copy of the document, which has already considered about any cases (include C's undefined behaviour). >> static void decode_addi_opcode_y0(struct DisasContext *dc, >> tilegx_bundle_bits bundle) >> { >> + uint8_t rsrc = (uint8_t)get_SrcA_Y0(bundle); >> + uint8_t rdst = (uint8_t)get_Dest_Y0(bundle); >> + int8_t imm8 = (int8_t)get_Imm8_Y0(bundle); > > These casts are all pointless. > OK, thanks. I shall change the return value of get_SrcA_Y0 and others in opcode_tilegx.h. [...] >> @@ -127,8 +547,13 @@ static void decode_rrr_1_opcode_y0(struct DisasContext >> *dc, >> static void decode_rrr_5_opcode_y0(struct DisasContext *dc, >> tilegx_bundle_bits bundle) >> { >> + uint8_t rsrc = (uint8_t)get_SrcA_Y0(bundle); >> + uint8_t rsrcb = (uint8_t)get_SrcB_Y0(bundle); >> + uint8_t rdst = (uint8_t)get_Dest_Y0(bundle); > > And again, and in many other places. > OK, thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed