From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39059) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7MDz-0006KV-BL for qemu-devel@nongnu.org; Tue, 23 Jun 2015 07:18:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7MDw-00036o-5J for qemu-devel@nongnu.org; Tue, 23 Jun 2015 07:18:43 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:14636) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7MDv-00036e-T0 for qemu-devel@nongnu.org; Tue, 23 Jun 2015 07:18:40 -0400 Message-ID: <55894081.3070702@imgtec.com> Date: Tue, 23 Jun 2015 12:18:25 +0100 From: Leon Alrae MIME-Version: 1.0 References: <1434731138-4918-1-git-send-email-yongbok.kim@imgtec.com> <1434731138-4918-15-git-send-email-yongbok.kim@imgtec.com> In-Reply-To: <1434731138-4918-15-git-send-email-yongbok.kim@imgtec.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 14/15] target-mips: microMIPS32 R6 POOL16{A, C} instructions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yongbok Kim , qemu-devel@nongnu.org Cc: aurelien@aurel32.net On 19/06/2015 17:25, Yongbok Kim wrote: > microMIPS32 Release 6 POOL16A/ POOL16C instructions > > Signed-off-by: Yongbok Kim > --- > target-mips/translate.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 105 insertions(+), 2 deletions(-) Looks correct, just minor nits: > > diff --git a/target-mips/translate.c b/target-mips/translate.c > index 9483d31..658ed33 100644 > --- a/target-mips/translate.c > +++ b/target-mips/translate.c > @@ -13163,6 +13163,101 @@ static void gen_pool16c_insn(DisasContext *ctx) > } > } > > +static inline void gen_movep(DisasContext *ctx) > +{ > + int enc_dest = uMIPS_RD(ctx->opcode); > + int enc_rt = uMIPS_RS2(ctx->opcode); > + int enc_rs = (ctx->opcode & 3) | ((ctx->opcode >> 1) & 4); > + int rd, rs, re, rt; > + static const int rd_enc[] = { 5, 5, 6, 4, 4, 4, 4, 4 }; > + static const int re_enc[] = { 6, 7, 7, 21, 22, 5, 6, 7 }; > + static const int rs_rt_enc[] = { 0, 17, 2, 3, 16, 18, 19, 20 }; > + rd = rd_enc[enc_dest]; > + re = re_enc[enc_dest]; > + rs = rs_rt_enc[enc_rs]; > + rt = rs_rt_enc[enc_rt]; > + gen_arith(ctx, OPC_ADDU, rd, rs, 0); > + gen_arith(ctx, OPC_ADDU, re, rt, 0); > +} Could you also update the pre-R6 MOVEP to call this function so we avoid duplicating? > + > +static void gen_pool16c_r6_insn(DisasContext *ctx) > +{ > + int rt = mmreg((ctx->opcode >> 7) & 0x7); > + int rs = mmreg((ctx->opcode >> 4) & 0x7); > + > + switch (ctx->opcode & 0xf) { > + case R6_NOT16: > + gen_logic(ctx, OPC_NOR, rt, rs, 0); > + break; > + case R6_AND16: > + gen_logic(ctx, OPC_AND, rt, rt, rs); > + break; > + case R6_LWM16: > + { > + static const int lwm_convert[] = { 0x11, 0x12, 0x13, 0x14 }; > + int offset = extract32(ctx->opcode, 4, 4); > + gen_ldst_multiple(ctx, LWM32, lwm_convert[(ctx->opcode >> 8) & 0x3], > + 29, offset << 2); Wouldn't be simpler to have something like: int lwm_converted = 0x11 + extract32(ctx->opcode, 8, 2); int offset = extract32(ctx->opcode, 4, 4); gen_ldst_multiple(ctx, LWM32, lwm_converted, 29, offset << 2); > + } > + break; > + case R6_JRC16: /* JRCADDIUSP */ > + if ((ctx->opcode >> 4) & 1) { > + /* JRCADDIUSP */ > + int imm = extract32(ctx->opcode, 5, 5); > + gen_compute_branch(ctx, OPC_JR, 2, 31, 0, 0, 0); > + gen_arith_imm(ctx, OPC_ADDIU, 29, 29, imm << 2); > + } else { > + /* JRC16 */ > + int rs = extract32(ctx->opcode, 5, 5); > + gen_compute_branch(ctx, OPC_JR, 2, rs, 0, 0, 0); > + } > + break; > + case MOVEP ... MOVEP_07: > + case MOVEP_0C ... MOVEP_0F: > + gen_movep(ctx); > + break; > + case R6_XOR16: > + gen_logic(ctx, OPC_XOR, rt, rt, rs); > + break; > + case R6_OR16: > + gen_logic(ctx, OPC_OR, rt, rt, rs); > + break; > + case R6_SWM16: > + { > + static const int swm_convert[] = { 0x11, 0x12, 0x13, 0x14 }; > + int offset = extract32(ctx->opcode, 4, 4); > + gen_ldst_multiple(ctx, SWM32, swm_convert[(ctx->opcode >> 8) & 0x3], > + 29, offset << 2); same > + } > + break; > + case JALRC16: /* BREAK16, SDBBP16 */ > + switch (ctx->opcode & 0x3f) { > + case JALRC16: > + case JALRC16 + 0x20: > + /* JALRC16 */ > + gen_compute_branch(ctx, OPC_JALR, 2, (ctx->opcode >> 5) & 0x1f, > + 31, 0, 0); > + break; > + case R6_BREAK16: > + /* BREAK16 */ > + generate_exception(ctx, EXCP_BREAK); > + break; > + case R6_SDBBP16: > + /* SDBBP16 */ > + if (ctx->hflags & MIPS_HFLAG_SBRI) { > + generate_exception(ctx, EXCP_RI); > + } else { > + generate_exception(ctx, EXCP_DBp); > + } > + break; > + } > + break; > + default: > + generate_exception(ctx, EXCP_RI); > + break; > + } > +} > + > static void gen_ldxs (DisasContext *ctx, int base, int index, int rd) > { > TCGv t0 = tcg_temp_new(); > @@ -15163,7 +15258,11 @@ static int decode_micromips_opc (CPUMIPSState *env, DisasContext *ctx) > int rs1 = mmreg(uMIPS_RS1(ctx->opcode)); > int rs2 = mmreg(uMIPS_RS2(ctx->opcode)); > uint32_t opc = 0; > - > + if (ctx->insn_flags & ISA_MIPS32R6) { > + rd = mmreg(uMIPS_RS1(ctx->opcode)); > + rs1 = mmreg(uMIPS_RD(ctx->opcode)); This is correct, although a bit confusing for me. You use uMIPS_RD macro to read rs1, and uMIPS_RS1 to read rd. Wouldn't it be better to remove this and call gen_arith() with just swapped rs1 and rd arguments for R6? Thanks, Leon > + rs2 = mmreg(uMIPS_RS2(ctx->opcode)); > + } > switch (ctx->opcode & 0x1) { > case ADDU16: > opc = OPC_ADDU; > @@ -15197,7 +15296,11 @@ static int decode_micromips_opc (CPUMIPSState *env, DisasContext *ctx) > } > break; > case POOL16C: > - gen_pool16c_insn(ctx); > + if (ctx->insn_flags & ISA_MIPS32R6) { > + gen_pool16c_r6_insn(ctx); > + } else { > + gen_pool16c_insn(ctx); > + } > break; > case LWGP16: > { >