Re: [Qemu-devel] [PATCH v2 14/15] target-mips: microMIPS32 R6 POOL16{A, C} instructions

[Leon Alrae]

On 19/06/2015 17:25, Yongbok Kim wrote:
> microMIPS32 Release 6 POOL16A/ POOL16C instructions
> 
> Signed-off-by: Yongbok Kim <address@hidden>
> ---
>  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:
>          {
>