Re: [Qemu-devel] [PATCH v3 07/40] target/mips: Add emulation of nanoMIPS 16-bit load and store instructions

[Richard Henderson]

On 07/19/2018 05:54 AM, Stefan Markovic wrote:
> +/* Used for 16-bit store instructions.  */
> +static int decode_gpr_gpr3_src_store(int r)
> +{
> +    static const int map[] = { 0, 17, 18, 19, 4, 5, 6, 7 };
> +
> +    return map[r & 0x7];
> +}

Same comment re comment as before.

>      case NM_P16_LB:
> +        {
> +            uint32_t u = extract32(ctx->opcode, 0, 2);
> +            switch (((ctx->opcode) >> 2) & 0x03) {
> +            case NM_LB16:
> +                gen_ld(ctx, OPC_LB, rt, rs, u);
> +                break;
> +            case NM_SB16:
> +                {
> +                    int rt = decode_gpr_gpr3_src_store(
> +                                 NANOMIPS_EXTRACT_RD(ctx->opcode));

Shadowing outer rt variable.  And indeed, NANOMIPS_EXTRACT_RD has already been
extracted into it, so this becomes just

    rt = decode_gpr_gpr3_src_store(rt);

Similarly throughout the rest of this file.

Consider creating one "int imm" variable at the top of the function that you
can reuse for all of these immediate value extractions and not have to create
these local variable blocks.


r~