Re: [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code

[Andrew Burgess <aburgess@redhat.com>]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> On 6/11/24 18:25, Andrew Burgess wrote:
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> 
>>> This uses the idea from the previous patch to
>>> simplify the code for non-overflowing signed
>>> divisions by -1.  This is no bug-fix but it
>>> simplifies the code and avoids some unnecessary
>>> branches.
>>> ---
>>>  sim/riscv/sim-main.c | 10 ++++------
>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
>>> index 515ff835223..e4b15b533ba 100644
>>> --- a/sim/riscv/sim-main.c
>>> +++ b/sim/riscv/sim-main.c
>>> @@ -700,18 +700,16 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>>    const char *rd_name = riscv_gpr_names_abi[rd];
>>>    const char *rs1_name = riscv_gpr_names_abi[rs1];
>>>    const char *rs2_name = riscv_gpr_names_abi[rs2];
>>> -  unsigned_word tmp, dividend_max;
>>> +  unsigned_word tmp;
>>>    sim_cia pc = riscv_cpu->pc + 4;
>>>  
>>> -  dividend_max = -((unsigned_word) 1 << (WITH_TARGET_WORD_BITSIZE - 1));
>> 
>> I really don't understand what this was trying to calculate.  It's not
>> the max value of an unsigned_word.  Nor is it the bit representation of
>> the maximum signed_word value... but you're removing it, so I guess I
>> can not worry about that :)
>> 
>
> This is the largest negative dividend, either -2**31=-2147483648 or -2**63=-9223372036854775808

I guess my definition of 'max' and 'min' differ from whoever wrote this
code!  I'd describe those values as 'dividend_min'.  But as the code is
going away it's really not important.

> and dividing this value by any non-negative value is always fine, except for the divisor=-1,
> because the result will overflow, and can therefore cause problems.

Ah, got you.  This for sure should have a comment explaining the reasoning.

>
> But by my reasoning I found a simple solution that handles the divisor -1 for all dividends
> correctly and more efficiently, by simply computing -(unsigned)x instead of  x/-1.
>
>>> -
>>>    switch (op->match)
>>>      {
>>>      case MATCH_DIV:
>>>        TRACE_INSN (cpu, "div %s, %s, %s;  // %s = %s / %s",
>>>  		  rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name);
>>> -      if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1)
>>> -	tmp = dividend_max;
>>> +      if (riscv_cpu->regs[rs2] == -1)
>>> +	tmp = -riscv_cpu->regs[rs1];
>> 
>> As with the previous commit, can't we drop this whole if check and block?
>> 
>
> No in this case the division would indeed overflow on the division dividend_max/-1. 

Agree.  Just add a comment explaining this please.

>
>> I think we should add some tests for this too.  The test I proposed in
>> the previous commit can be modified to give us div.s and rem.s tests.
>> 
>
> Well, since this is just refactoring, and there is already very good test
> coverage in the gcc testsuite, I would not do that here, but instead

The simulator should have tests in the simulator tree so I think we need
to add tests.

As I've already supplied a rough framework for the previous commit,
which I believe you're going to polish for that test, all you need to do
here is copy that framework, change divw to div, and fill in a few
interesting test cases, so it shouldn't take long.

> I think the overflow bug in the mulh instruction really deserves a test case:
>
> [PATCH] sim: riscv: Fix undefined behaviour in mulh and similar
> https://sourceware.org/pipermail/gdb-patches/2024-April/208616.html
>
> So I will post an update for this one with a test case that demonstrates
> the possible mis-compilation due to a compiler optimization based on the
> assumption that an undefined behaviour may never happen, which caused
> user visible effects in this case.

Great, the more tests the better.

Thanks,
Andrew