Re: [PATCH v11 08/46] target/mips: Add emulation of nanoMIPS 16-bit branch instructions

[Philippe Mathieu-Daudé]

On 5/29/21 3:52 PM, Philippe Mathieu-Daudé wrote:
> On Mon, Aug 20, 2018 at 8:17 PM Aleksandar Markovic
> <aleksandar.markovic@rt-rk.com> wrote:
>>
>> From: Stefan Markovic <smarkovic@wavecomp.com>
>>
>> Add emulation of nanoMIPS 16-bit branch instructions.
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Yongbok Kim <yongbok.kim@mips.com>
>> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
>> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
>> ---
>>  target/mips/translate.c | 158 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 158 insertions(+)
>>
>> diff --git a/target/mips/translate.c b/target/mips/translate.c
>> index 261680e..b0bbf4c 100644
>> --- a/target/mips/translate.c
>> +++ b/target/mips/translate.c
>> @@ -4564,6 +4564,128 @@ static void gen_compute_branch (DisasContext *ctx, 
>> uint32_t opc,
>>      tcg_temp_free(t1);
>>  }
>>
>> +
>> +/* nanoMIPS Branches */
>> +static void gen_compute_branch_nm(DisasContext *ctx, uint32_t opc,
>> +                                int insn_bytes,
>> +                                int rs, int rt, int32_t offset)
>> +{
>> +    target_ulong btgt = -1;
>> +    int bcond_compute = 0;
>> +    TCGv t0 = tcg_temp_new();
>> +    TCGv t1 = tcg_temp_new();
>> +
>> +    /* Load needed operands */
>> +    switch (opc) {
>> +    case OPC_BEQ:
>> +    case OPC_BNE:
>> +        /* Compare two registers */
>> +        if (rs != rt) {
>> +            gen_load_gpr(t0, rs);
>> +            gen_load_gpr(t1, rt);
>> +            bcond_compute = 1;
>> +        }
>> +        btgt = ctx->base.pc_next + insn_bytes + offset;
>> +        break;
>> +    case OPC_BGEZAL:
>> +        /* Compare to zero */
>> +        if (rs != 0) {
>> +            gen_load_gpr(t0, rs);
>> +            bcond_compute = 1;
>> +        }
>> +        btgt = ctx->base.pc_next + insn_bytes + offset;
>> +        break;
>> +    case OPC_BPOSGE32:
>> +        tcg_gen_andi_tl(t0, cpu_dspctrl, 0x3F);
>> +        bcond_compute = 1;
>> +        btgt = ctx->base.pc_next + insn_bytes + offset;

I think this opcode never worked correctly.

Per the "MIPS® Architecture Extension: nanoMIPS32 DSP Technical
Reference Manual — Revision 0.04" p. 88 "BPOSGE32C":

  "First, the offset argument is left-shifted by one bit to form
   a 17-bit signed integer value."

The caller, decode_nanomips_32_48_opc(), doesn't shift the offset:

    case NM_BPOSGE32C:
        check_dsp_r3(ctx);
        {
            int32_t imm = extract32(ctx->opcode, 1, 13) |
                          extract32(ctx->opcode, 0, 1) << 13;

            gen_compute_branch_nm(ctx, OPC_BPOSGE32, 4, -1, -2,
                                  imm);
        }
        break;

>> +        break;
>> +    case OPC_JR:
>> +    case OPC_JALR:
>> +        /* Jump to register */
>> +        if (offset != 0 && offset != 16) {
>> +            /* Hint = 0 is JR/JALR, hint 16 is JR.HB/JALR.HB, the
>> +               others are reserved. */
>> +            MIPS_INVAL("jump hint");
>> +            generate_exception_end(ctx, EXCP_RI);
>> +            goto out;
>> +        }
>> +        gen_load_gpr(btarget, rs);
>> +        break;
>> +    default:
>> +        MIPS_INVAL("branch/jump");
>> +        generate_exception_end(ctx, EXCP_RI);
>> +        goto out;
>> +    }
>> +    if (bcond_compute == 0) {
>> +        /* No condition to be computed */
>> +        switch (opc) {
>> +        case OPC_BEQ:     /* rx == rx        */
>> +            /* Always take */
>> +            ctx->hflags |= MIPS_HFLAG_B;
>> +            break;
>> +        case OPC_BGEZAL:  /* 0 >= 0          */
>> +            /* Always take and link */
>> +            tcg_gen_movi_tl(cpu_gpr[31],
>> +                            ctx->base.pc_next + insn_bytes);
>> +            ctx->hflags |= MIPS_HFLAG_B;
>> +            break;
>> +        case OPC_BNE:     /* rx != rx        */
>> +            tcg_gen_movi_tl(cpu_gpr[31], ctx->base.pc_next + 8);
>> +            /* Skip the instruction in the delay slot */
>> +            ctx->base.pc_next += 4;
>> +            goto out;
>> +        case OPC_JR:
>> +            ctx->hflags |= MIPS_HFLAG_BR;
>> +            break;
>> +        case OPC_JALR:
>> +            if (rt > 0) {
>> +                tcg_gen_movi_tl(cpu_gpr[rt],
>> +                                ctx->base.pc_next + insn_bytes);
>> +            }
>> +            ctx->hflags |= MIPS_HFLAG_BR;
>> +            break;
>> +        default:
>> +            MIPS_INVAL("branch/jump");
>> +            generate_exception_end(ctx, EXCP_RI);
>> +            goto out;
>> +        }
>> +    } else {
>> +        switch (opc) {
>> +        case OPC_BEQ:
>> +            tcg_gen_setcond_tl(TCG_COND_EQ, bcond, t0, t1);
>> +            goto not_likely;
>> +        case OPC_BNE:
>> +            tcg_gen_setcond_tl(TCG_COND_NE, bcond, t0, t1);
>> +            goto not_likely;
>> +        case OPC_BGEZAL:
>> +            tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 0);
>> +            tcg_gen_movi_tl(cpu_gpr[31],
>> +                            ctx->base.pc_next + insn_bytes);
>> +            goto not_likely;
>> +        case OPC_BPOSGE32:
>> +            tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 32);
> 
> This opcode implementation seems incomplete, per the ISA manual:
> 
> If a control transfer instruction (CTI) is executed in the forbidden
> slot of a branch or jump, Release 6 implementations are required to
> signal a Reserved Instruction Exception. A CTI is considered to be one
> of the following instructions: branch, jump, NAL (Release 6), ERET,
> ERETNC (Release 5), DERET, WAIT, or PAUSE (Release 2). An instruction
> is in the forbidden slot if it is the instruction following the
> branch.
> 
>> +        not_likely:
>> +            ctx->hflags |= MIPS_HFLAG_BC;
>> +            break;
>> +        default:
>> +            MIPS_INVAL("conditional branch/jump");
>> +            generate_exception_end(ctx, EXCP_RI);
>> +            goto out;
>> +        }
>> +    }
>> +
>> +    ctx->btarget = btgt;
>> +
>> + out:
>> +    if (insn_bytes == 2) {
>> +        ctx->hflags |= MIPS_HFLAG_B16;
>> +    }
>> +    tcg_temp_free(t0);
>> +    tcg_temp_free(t1);
>> +}
>