Re: [Qemu-devel] [PATCH v3 06/40] target/mips: Add emulation of misc nanoMIPS 16-bit instructions

[Richard Henderson]

On 07/19/2018 05:54 AM, Stefan Markovic wrote:
> From: Yongbok Kim <address@hidden>
> 
> Add emulation of misc nanoMIPS 16-bit instructions from instruction
> pools P16, P16.BR, P16.BRI, P16.4X4 and other related pools.
> 
> Signed-off-by: Yongbok Kim <address@hidden>
> Signed-off-by: Aleksandar Markovic <address@hidden>
> Signed-off-by: Stefan Markovic <address@hidden>
> ---
>  target/mips/translate.c | 258 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 258 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 4e6ae1f..798f977 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -16502,6 +16502,264 @@ static int decode_gpr_gpr4_zero(int r)
>  
>  static int decode_nanomips_opc(CPUMIPSState *env, DisasContext *ctx)
>  {
> +    uint32_t op;
> +    int rt = decode_gpr_gpr3(NANOMIPS_EXTRACT_RD(ctx->opcode));
> +    int rs = decode_gpr_gpr3(NANOMIPS_EXTRACT_RS(ctx->opcode));
> +    int rd = decode_gpr_gpr3(NANOMIPS_EXTRACT_RS1(ctx->opcode));
> +
> +    /* make sure instructions are on a halfword boundary */
> +    if (ctx->base.pc_next & 0x1) {
> +        env->CP0_BadVAddr = ctx->base.pc_next;

You can't assign to ENV here.  You must generate code to do this:

    TCGv tmp = tcg_const_tl(ctx->base.pc_next);
    tcg_gen_st_tl(tmp, cpu_env, offsetof(CPUMIPSState, CP0_BadVAddr));
    tcg_temp_free(tmp);

> +        generate_exception_end(ctx, EXCP_AdEL);
> +        return 2;
> +    }


> +
> +    op = (ctx->opcode >> 10) & 0x3f;
> +    switch (op) {
> +    case NM_P16_MV:
> +        {
> +            int rt = NANOMIPS_EXTRACT_RD5(ctx->opcode);
> +            if (rt != 0) {
> +                /* MOVE */
> +                int rs = NANOMIPS_EXTRACT_RS5(ctx->opcode);
> +                gen_arith(ctx, OPC_ADDU, rt, rs, 0);

If you've any thought for nanoMIPS64 in future, consider using OR instead.

> +        case NM_ADDIUR2:
> +        {
> +            uint8_t u = (uint8_t) extract32(ctx->opcode, 0, 3) << 2;
> +            gen_arith_imm(ctx, OPC_ADDIU, rt, rs, u);

Drop the useless cast?  And the variable, come to that; just above you extract
the immediate to gen_arith_imm as an argument.
        
> +        }
> +            break;
> +        case NM_P_ADDIURS5:
> +        {
> +            int rt  = extract32(ctx->opcode, 5, 5);

This shadows the outer rt variable.  Just overwrite the original.

> +            if (rt != 0) {
> +                int s = (sextract32(ctx->opcode, 4, 1) << 3) |
> +                        extract32(ctx->opcode, 0, 3);
> +                /* s = sign_extend( s[3] . s[2:0] , from_nbits = 4)*/
> +                gen_arith_imm(ctx, OPC_ADDIU, rt, rt, s);
> +            }
> +        }
> +            break;
> +        }
> +        break;

There's some really bad indentation going on here.

> +    case NM_P16_4X4:
> +        {
> +            int rt = (extract32(ctx->opcode, 9, 1) << 3) |
> +                      extract32(ctx->opcode, 5, 3);
> +            int rs = (extract32(ctx->opcode, 4, 1) << 3) |
> +                      extract32(ctx->opcode, 0, 3);

Shadowing again.

> +        default:
> +            /* P16.BRI */
> +            if (extract32(ctx->opcode, 4, 3) < extract32(ctx->opcode, 7, 3)) 
> {

These are already extracted into rs and rt...

> +                /* BEQC16 */
> +                gen_compute_branch(ctx, OPC_BEQ, 2, rs, rt,
> +                                   extract32(ctx->opcode, 0, 4) << 1, 0);

... which you are using here.

And surely, merge these as

    gen_compute_branch(ctx, rs < rt ? OPC_BEQ : OPC_BNE, 2, rs, rt,
                       extract32(ctx->opcode, 0, 4) << 1, 0);


r~