Re: [Qemu-devel] [PATCH 08/35] target/mips: Add nanoMIPS 32bit instructions

[Richard Henderson]

On 06/20/2018 05:05 AM, Yongbok Kim wrote:
> Add nanoMIPS 32bit instructions
> 
> Signed-off-by: Yongbok Kim <address@hidden>
> ---
>  target/mips/translate.c | 285 
> +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 284 insertions(+), 1 deletion(-)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 4ce80bf..c9b46dd 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> +        } else {
> +            uint16_t imm;
> +            imm = (uint16_t) extract32(ctx->opcode, 0, 16);

Unnecessary cast.

> +            if (rs != 0) {
> +                tcg_gen_addi_tl(cpu_gpr[rt], cpu_gpr[rs], imm);
> +                tcg_gen_ext32s_tl(cpu_gpr[rt], cpu_gpr[rt]);
> +            } else {
> +                tcg_gen_movi_tl(cpu_gpr[rt], imm);
> +            }
> +        }
> +        break;
> +    case NM_ADDIUPC:
> +        if (rt != 0) {
> +            int32_t offset = sextract32(ctx->opcode, 0, 1) << 21
> +                            | extract32(ctx->opcode, 1, 20) << 1;
> +            target_long addr = addr_add(ctx, ctx->base.pc_next + 4, offset);
> +            tcg_gen_movi_tl(cpu_gpr[rt], addr);
> +            tcg_gen_ext32s_tl(cpu_gpr[rt], cpu_gpr[rt]);

ext32s is unnecessary.
The addr passed to movi_tl is signed and properly sized.

> +        case NM_ADDIUGP_W:
> +            if (rt != 0) {
> +                uint32_t offset = extract32(ctx->opcode, 0, 21);
> +                if (offset == 0) {
> +                    gen_load_gpr(cpu_gpr[rt], 28);
> +                } else {
> +                    TCGv t0;
> +                    t0 = tcg_temp_new();
> +                    tcg_gen_movi_tl(t0, offset);
> +                    gen_op_addr_add(ctx, cpu_gpr[rt], cpu_gpr[28], t0);
> +                    tcg_temp_free(t0);
> +                }

Your special-case of here fails to sign-extend for 0.
That would want fixing for 64-bit nanoMIPS with AWRAP.

It would be worthwhile to have a gen_op_addr_addi,
and not special-case offset here -- tcg_gen_addi_tl
would do that for you.

> +        case NM_SLTI:
> +            gen_slt_imm(ctx, OPC_SLTI, rt, rs, extract32(ctx->opcode, 0, 
> 12));
> +            break;
> +        case NM_SLTIU:
> +            gen_slt_imm(ctx, OPC_SLTIU, rt, rs, extract32(ctx->opcode, 0, 
> 12));
> +            break;
> +        case NM_SEQI:
> +        {
> +            TCGv t0 = tcg_temp_new();
> +            TCGv t1 = tcg_temp_new();
> +            TCGv t2 = tcg_temp_local_new();
> +            TCGLabel *l1 = gen_new_label();
> +
> +            gen_load_gpr(t0, rs);
> +            tcg_gen_movi_tl(t1, extract32(ctx->opcode, 0, 12));
> +            tcg_gen_movi_tl(t2, 0);
> +            tcg_gen_brcond_tl(TCG_COND_NE, t0, t1, l1);
> +            tcg_gen_movi_tl(t2, 1);
> +            gen_set_label(l1);

No labels required.  This is

    tcg_gen_setcondi_tl(TCG_COND_NE, t2, t0, extract32(...));

> +            gen_store_gpr(t2, rt);
> +
> +            tcg_temp_free(t0);
> +            tcg_temp_free(t1);
> +            tcg_temp_free(t2);
> +        }
> +            break;
> +        case NM_ADDIUNEG:
> +        {
> +            int16_t imm;
> +            imm = (int16_t) extract32(ctx->opcode, 0, 12);

Cast is unnecessary.


r~