From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xcawg-0000KE-P0 for qemu-devel@nongnu.org; Fri, 10 Oct 2014 10:13:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xcawb-0002cT-LH for qemu-devel@nongnu.org; Fri, 10 Oct 2014 10:13:26 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:58961) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xcawb-0002cL-Cy for qemu-devel@nongnu.org; Fri, 10 Oct 2014 10:13:21 -0400 Message-ID: <5437E97A.2090209@imgtec.com> Date: Fri, 10 Oct 2014 15:13:14 +0100 From: Leon Alrae MIME-Version: 1.0 References: <1405331763-57126-1-git-send-email-yongbok.kim@imgtec.com> <1405331763-57126-10-git-send-email-yongbok.kim@imgtec.com> In-Reply-To: <1405331763-57126-10-git-send-email-yongbok.kim@imgtec.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 09/20] target-mips: add MSA branch instructions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yongbok Kim , qemu-devel@nongnu.org Cc: cristian.cuna@imgtec.com, aurelien@aurel32.net On 14/07/2014 10:55, Yongbok Kim wrote: > add MSA branch instructions > > Signed-off-by: Yongbok Kim > --- > target-mips/translate.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 105 insertions(+), 2 deletions(-) > > diff --git a/target-mips/translate.c b/target-mips/translate.c > index b8dbbdc..0bfbcfe 100644 > --- a/target-mips/translate.c > +++ b/target-mips/translate.c > @@ -14688,6 +14688,95 @@ static inline int check_msa_access(CPUMIPSState *env, DisasContext *ctx, > } > return 1; > } > + > +static void determ_zero_element(TCGv tresult, uint8_t df, uint8_t wt) > +{ nit: if you add gen_ prefix to this function name it will be clear that it generates tcg operations without having to look at the body > + /* Note this function only works with MSA_WRLEN = 128 */ > + uint64_t eval_zero_or_big; > + uint64_t eval_big; > + switch (df) { > + case 0: /*DF_BYTE*/ why not using DF_BYTE, DF_HALF etc. directly rather than putting in comments? > + eval_zero_or_big = 0x0101010101010101ULL; > + eval_big = 0x8080808080808080ULL; > + break; > + case 1: /*DF_HALF*/ > + eval_zero_or_big = 0x0001000100010001ULL; > + eval_big = 0x8000800080008000ULL; > + break; > + case 2: /*DF_WORD*/ > + eval_zero_or_big = 0x0000000100000001ULL; > + eval_big = 0x8000000080000000ULL; > + break; > + case 3: /*DF_DOUBLE*/ > + eval_zero_or_big = 0x0000000000000001ULL; > + eval_big = 0x8000000000000000ULL; > + break; > + } > + TCGv_i64 t0 = tcg_temp_local_new_i64(); > + TCGv_i64 t1 = tcg_temp_local_new_i64(); local temps aren't needed here, normal temps would be sufficient > + tcg_gen_subi_i64(t0, msa_wr_d[wt<<1], eval_zero_or_big); > + tcg_gen_andc_i64(t0, t0, msa_wr_d[wt<<1]); > + tcg_gen_andi_i64(t0, t0, eval_big); > + tcg_gen_subi_i64(t1, msa_wr_d[(wt<<1)+1], eval_zero_or_big); > + tcg_gen_andc_i64(t1, t1, msa_wr_d[(wt<<1)+1]); > + tcg_gen_andi_i64(t1, t1, eval_big); > + tcg_gen_or_i64(t0, t0, t1); > + /* if all bits is zero then all element is not zero */ > + /* if some bit is non-zero then some element is zero */ > + tcg_gen_setcondi_i64(TCG_COND_NE, t0, t0, 0); > + tcg_gen_trunc_i64_tl(tresult, t0); > + tcg_temp_free_i64(t0); > + tcg_temp_free_i64(t1); > +} > + > +static void gen_msa_branch(CPUMIPSState *env, DisasContext *ctx, uint32_t op1) > +{ > + check_insn(ctx, ASE_MSA); > + > + if (ctx->hflags & MIPS_HFLAG_BMASK) { > + generate_exception(ctx, EXCP_RI); > + return; > + } > + > + uint8_t df = (ctx->opcode >> 21) & 0x3 /* df [22:21] */; > + uint8_t wt = (ctx->opcode >> 16) & 0x1f /* wt [20:16] */; > + int64_t s16 = (ctx->opcode >> 0) & 0xffff /* s16 [15:0] */; > + s16 = (s16 << 48) >> 48; /* sign extend s16 to 64 bits*/ int64_t s16 = (int16_t)ctx->opcode :) Also, I think in QEMU it's preferable to have declarations at the beginning of a block > + > + check_msa_access(env, ctx, wt, -1, -1); > + > + switch (op1) { > + case OPC_MSA_BZ_V: > + case OPC_MSA_BNZ_V: > + { > + TCGv_i64 t0 = tcg_temp_local_new_i64(); > + tcg_gen_or_i64(t0, msa_wr_d[wt<<1], msa_wr_d[(wt<<1)+1]); > + tcg_gen_setcondi_i64((op1 == OPC_MSA_BZ_V) ? > + TCG_COND_EQ : TCG_COND_NE, t0, t0, 0); > + tcg_gen_trunc_i64_tl(bcond, t0); > + tcg_temp_free_i64(t0); > + } > + break; > + case OPC_MSA_BZ_B: > + case OPC_MSA_BZ_H: > + case OPC_MSA_BZ_W: > + case OPC_MSA_BZ_D: > + determ_zero_element(bcond, df, wt); > + break; > + case OPC_MSA_BNZ_B: > + case OPC_MSA_BNZ_H: > + case OPC_MSA_BNZ_W: > + case OPC_MSA_BNZ_D: > + determ_zero_element(bcond, df, wt); > + tcg_gen_setcondi_tl(TCG_COND_EQ, bcond, bcond, 0); > + break; > + } > + > + int64_t offset = s16 << 2; > + ctx->btarget = ctx->pc + offset + 4; > + > + ctx->hflags |= MIPS_HFLAG_BC; > +} > static void decode_opc (CPUMIPSState *env, DisasContext *ctx) > { > int32_t offset; > @@ -15729,9 +15818,23 @@ static void decode_opc (CPUMIPSState *env, DisasContext *ctx) > break; > > case OPC_CP1: > - if (ctx->CP0_Config1 & (1 << CP0C1_FP)) { > + op1 = MASK_CP1(ctx->opcode); > + > + if ((ctx->insn_flags & ASE_MSA) && > + (op1 == OPC_MSA_BZ_V || > + op1 == OPC_MSA_BNZ_V || > + op1 == OPC_MSA_BZ_B || > + op1 == OPC_MSA_BZ_H || > + op1 == OPC_MSA_BZ_W || > + op1 == OPC_MSA_BZ_D || > + op1 == OPC_MSA_BNZ_B || > + op1 == OPC_MSA_BNZ_H || > + op1 == OPC_MSA_BNZ_W || > + op1 == OPC_MSA_BNZ_D)) { > + gen_msa_branch(env, ctx, op1); can't this be merged into the switch below? > + } else if (ctx->CP0_Config1 & (1 << CP0C1_FP)) { > check_cp1_enabled(ctx); > - op1 = MASK_CP1(ctx->opcode); > + > switch (op1) { > case OPC_MFHC1: > case OPC_MTHC1: > Regards, Leon