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

[Faraz Shahbazker <faraz.shahbazker@gmail.com>]

Hi Philippe,

On 5/29/21 7:50 PM, Philippe Mathieu-Daudé wrote:
> 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>
>>> +    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;

I agree that the left-shift is missing. One must also note that the text 
in the nanoMIPS32 DSP manual is incorrect. This was most probably 
copy-pasted from the microMIPS DSP manual. The effective offset for 
nanoMIPS DSP is supposed to be 15-bit signed, not 17-bit.

- farazS