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

[Andrew Burgess <aburgess@redhat.com>]

Palmer Dabbelt <palmer@dabbelt.com> writes:

> 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 ;)).

Hi Palmer,

Apologies for letting some of the sim/ reviews lag.  I've been pretty
rushed for the last few weeks, but I'm trying to get on top of any
outstanding sim/ patches, and hopefully will try to keep on top of the
reviews going forward.

Thanks,
Andrew