Re: [PING^3 PATCH 1/1] sim/riscv: PR29595, Fix multiply instructions

[Palmer Dabbelt <palmer@dabbelt.com>]

On Thu, 06 Oct 2022 09:14:37 PDT (-0700), research_trasio@irq.a4lg.com wrote:
> On 2022/10/07 0:58, Palmer Dabbelt wrote:
>> On Thu, 06 Oct 2022 03:33:00 PDT (-0700), research_trasio@irq.a4lg.com
>> wrote:
>>> Because of recent 'Zmmul' support, the simulator is now broken.  This is
>>> caused by instruction classification changes:
>>>
>>> [Before]
>>> -   INSN_CLASS_M     : multiply / divide
>>> [After Zmmul]
>>> -   INSN_CLASS_M     : divide
>>> -   INSN_CLASS_ZMMUL : multiply
>>>
>>> The simulator checks the instruction class to execute an instruction:
>>>
>>> -   INSN_CLASS_I  : 'I'
>>> -   INSN_CLASS_M  : 'M' (multiply / divide)
>>> -   INSN_CLASS_A  : 'A'
>>>
>>> 'Zmmul' moved multiply instructions to INSN_CLASS_ZMMUL and that
>>> instruction
>>> class is not handled by the simulator.
>>>
>>> This commit handles INSN_CLASS_ZMMUL for all 'M' instructions and adds a
>>> testcase to test all RV32M instructions run without any faults.
>>> ---
>>>  sim/riscv/sim-main.c        |  1 +
>>>  sim/testsuite/riscv/m-ext.s | 18 ++++++++++++++++++
>>>  2 files changed, 19 insertions(+)
>>>  create mode 100644 sim/testsuite/riscv/m-ext.s
>>>
>>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
>>> index 30d2f1e1c9a..0156f791d4b 100644
>>> --- a/sim/riscv/sim-main.c
>>> +++ b/sim/riscv/sim-main.c
>>> @@ -936,6 +936,7 @@ execute_one (SIM_CPU *cpu, unsigned_word iw, const
>>> struct riscv_opcode *op)
>>>      case INSN_CLASS_I:
>>>        return execute_i (cpu, iw, op);
>>>      case INSN_CLASS_M:
>>> +    case INSN_CLASS_ZMMUL:
>>>        return execute_m (cpu, iw, op);
>>>      default:
>>>        TRACE_INSN (cpu, "UNHANDLED EXTENSION: %d", op->insn_class);
>>> diff --git a/sim/testsuite/riscv/m-ext.s b/sim/testsuite/riscv/m-ext.s
>>> new file mode 100644
>>> index 00000000000..b85397a32a0
>>> --- /dev/null
>>> +++ b/sim/testsuite/riscv/m-ext.s
>>> @@ -0,0 +1,18 @@
>>> +# check that the RV32M instructions run without any fault.
>>> +# mach: riscv
>>> +
>>> +.include "testutils.inc"
>>> +
>>> +    start
>>> +
>>> +    .option    arch, +m
>>> +    mul    x0, x1, x2
>>> +    mulh    x0, x1, x2
>>> +    mulhu    x0, x1, x2
>>> +    mulhsu    x0, x1, x2
>>> +    div    x0, x1, x2
>>> +    divu    x0, x1, x2
>>> +    rem    x0, x1, x2
>>> +    remu    x0, x1, x2
>>> +
>>> +    pass
>>
>> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
>> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> though as we're talking about in this meeting, I'm not actually a gdbsim
>> maintainer so I'm not sure I can formally approve it.
>>
>
> Palmer,
>
> I saw following files and thought you are one of the person who can
> formally approve my patch.
>
> Quoting sim/MAINTAINERS:
>>                 SIM Maintainers
>>
>> The simulator is part of the GDB project, so see the file
>> gdb/MAINTAINERS for general information about maintaining these files....
>> common          Frank Ch. Eigler <fche@redhat.com>
>> *               (target, then global maintainers)
>
> I know that Andrew and Palmer are RISC-V target maintainers of GDB so I
> assumed you are responsible for this area.  But it seems... no one knows
> exactly.  It seems very few people is interested in the simulator so...
> well... for now, I will continue pinging until someone who thinks
> responsible notices.

Ah, I guess I wasn't looking close enough.  I also didn't write the sim 
port, Mike Frysinger did and he's a sim gloabal maintainer so I'd 
generally just deferred to him on these things.

+Mike and Andrew: I'm OK reviewing sim patches, at least for the stuff 
that's pretty much just ISA encoding.  The port certainly needs some 
love and I don't really have the time to write the code, but I'm OK 
finding some time to review stuff if that's what's necessary for patches 
to land (though I'd be very happy to have someone else do the work, as 
usual ;)).