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

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

[Bernd Edlinger]

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

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

[Andrew Burgess]

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 :)

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

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.

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

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

[Bernd Edlinger]

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
> 

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

[Andrew Burgess]

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

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

[Bernd Edlinger]

On 6/17/24 10:58, Andrew Burgess wrote:
> 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.
> 

Ack.

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

Okay,

I'll add this comment:
--- a/sim/riscv/sim-main.c
+++ b/sim/riscv/sim-main.c
@@ -766,6 +766,9 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
     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);
+      /* Dividing by -1 can potentially overflow, and has therefore
+        to be done separately.  Note that the sign change operation
+        is done on an unsigned value and is therefore safe to do.  */
       if (riscv_cpu->regs[rs2] == -1)
        tmp = -riscv_cpu->regs[rs1];
       else if (riscv_cpu->regs[rs2])


How do you like that explanation?

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

Okay, if you like, I will add some more tests, the only interesting thing
is how they made the result of divide by zero defined, but this patch
did not touch that code, yet it may be worth to have some test coverage
for that else branch here.


Thanks
Bernd.


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