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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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

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. 

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


Thanks
Bernd.

> Thanks,
> Andrew
> 
>>        else if (riscv_cpu->regs[rs2])
>>  	tmp = (signed_word) riscv_cpu->regs[rs1] /
>>  	  (signed_word) riscv_cpu->regs[rs2];
>> @@ -793,7 +791,7 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>      case MATCH_REM:
>>        TRACE_INSN (cpu, "rem %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)
>> +      if (riscv_cpu->regs[rs2] == -1)
>>  	tmp = 0;
>>        else if (riscv_cpu->regs[rs2])
>>  	tmp = (signed_word) riscv_cpu->regs[rs1]
>> -- 
>> 2.39.2
>