Hi Yongbok,
I know you're preparing another patchset, but thought I may as well
continue reviewing this patchset until that one lands, sorry it's taken
me a while to get round to it.
On Mon, Jul 14, 2014 at 10:55:52AM +0100, Yongbok Kim wrote:
> +static void determ_zero_element(TCGv tresult, uint8_t df, uint8_t wt)
> +{
> + /* Note this function only works with MSA_WRLEN = 128 */
I reckon a quick comment to explain what this function is doing wouldn't
hurt, since not obvious from name.
I'm guessing from knowledge of MSA branches it determines whether there
is a zero element.
> + uint64_t eval_zero_or_big;
> + uint64_t eval_big;
> + switch (df) {
> + case 0: /*DF_BYTE*/
> + 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();
Variable declarations after code
These don't need preserving over any branches, so presumably they don't
need to be local temps.
> + 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 */
nit: s/is/are/, s/element/elements/
> + /* 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] */;
The TRM on public website is wrong about the branch encoding! :-P
> + int64_t s16 = (ctx->opcode >> 0) & 0xffff /* s16 [15:0] */;
> + s16 = (s16 << 48) >> 48; /* sign extend s16 to 64 bits*/
declarations after code
why not just use int16_t and let the compiler worry about sign
extension?
> +
> + 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();
i don't think this needs to be a local
> + 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);
Might be slightly more efficient to just get determ_zero_element to
generatee the correct condition in the first place.
> + break;
> + }
> +
> + int64_t offset = s16 << 2;
declaration after code
> + ctx->btarget = ctx->pc + offset + 4;
> +
> + ctx->hflags |= MIPS_HFLAG_BC;
> +}
blank line would be nice
> static void decode_opc (CPUMIPSState *env, DisasContext *ctx)
> {
> int32_t offset;
Cheers
James
