Fix for MUL instruction on the v850

Fix for MUL instruction on the v850

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

The v850 gcc testsuite started failing test vrp13 about a week ago. This 
was ultimately tracked down on a bug in the simulator's handling of the 
MUL instruction (a change in the compiler resulted in the test no longer 
compile-time optimized away).

MUL on the v850 is a 32x32->64 multiply with the hi/low parts of the 
result going into distinct registers.     mul r1, r2, r3 is r1xr2 -> 
(r3, r2) where r2 holds the low 32 bits of the result and r3 holds the 
high 32 bits of the result.

We had inputs of 2, -10 and produced an output of 20.  Opps ;-)

The v850 is a 32bit processor using 2s complement.  So to check if a 
value is negative we merely need to check if bit 0x80000000 is on. 
Anything else is just introducing dependencies on the host system's 
types, argument promotions, etc, which is precisely what happened in 
this case.

No new testcase for the simulator.  Not only does vrp13 test this, but 
about 1800 other tests which have flipped from failing to passing in the 
GCC testsuite...  So I can say it's being tested and if it were to 
regress again, we'd catch it.

OK for the trunk?

Thanks,
Jeff


[-- Attachment #2: 0001-v850.patch --]
[-- Type: text/plain, Size: 525 bytes --]

	* sim/v850/simops.c (Multiply64): Properly test if we need to negate
	either of the operand.

diff --git a/sim/v850/simops.c b/sim/v850/simops.c
index 8fac8bd9891..94399e2ad4f 100644
--- a/sim/v850/simops.c
+++ b/sim/v850/simops.c
@@ -339,10 +339,10 @@ Multiply64 (int sign, unsigned long op0)
 	  
       sign = (op0 ^ op1) & 0x80000000;
 	  
-      if (((signed long) op0) < 0)
+      if (op0 & 0x80000000)
 	op0 = - op0;
 	  
-      if (((signed long) op1) < 0)
+      if (op1 & 0x80000000)
 	op1 = - op1;
     }
       

Re: Fix for MUL instruction on the v850

[Hans-Peter Nilsson]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

(Wrong list; I CC:ed the right one.)

On Sat, 26 Mar 2022, Jeff Law via Binutils wrote:

> The v850 gcc testsuite started failing test vrp13 about a week ago. This was
> ultimately tracked down on a bug in the simulator's handling of the MUL
> instruction (a change in the compiler resulted in the test no longer
> compile-time optimized away).
>
> MUL on the v850 is a 32x32->64 multiply with the hi/low parts of the result
> going into distinct registers.? ?? mul r1, r2, r3 is r1xr2 -> (r3, r2) where
> r2 holds the low 32 bits of the result and r3 holds the high 32 bits of the
> result.
>
> We had inputs of 2, -10 and produced an output of 20.? Opps ;-)
>
> The v850 is a 32bit processor using 2s complement.? So to check if a value is
> negative we merely need to check if bit 0x80000000 is on. Anything else is
> just introducing dependencies on the host system's types, argument promotions,
> etc, which is precisely what happened in this case.
>
> No new testcase for the simulator.? Not only does vrp13 test this, but about
> 1800 other tests which have flipped from failing to passing in the GCC
> testsuite...? So I can say it's being tested and if it were to regress again,
> we'd catch it.
>
> OK for the trunk?
>
> Thanks,
> Jeff

OK if you mention the gcc test-suite in the commit message or a
comment, but please (re)consider adding a self-contained (i.e.
just binutils+sim) test-case.  Suggested inspiration: "git grep
sim" in the CRIS sim test-suite, though without a "dump
register" framework macro you need to compare with expected
results instead of matching output.

brgds, H-P

[-- Attachment #2: Type: text/plain, Size: 525 bytes --]

	* sim/v850/simops.c (Multiply64): Properly test if we need to negate
	either of the operand.

diff --git a/sim/v850/simops.c b/sim/v850/simops.c
index 8fac8bd9891..94399e2ad4f 100644
--- a/sim/v850/simops.c
+++ b/sim/v850/simops.c
@@ -339,10 +339,10 @@ Multiply64 (int sign, unsigned long op0)
 	  
       sign = (op0 ^ op1) & 0x80000000;
 	  
-      if (((signed long) op0) < 0)
+      if (op0 & 0x80000000)
 	op0 = - op0;
 	  
-      if (((signed long) op1) < 0)
+      if (op1 & 0x80000000)
 	op1 = - op1;
     }
       

Re: Fix for MUL instruction on the v850

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]

On 28 Mar 2022 09:38, Hans-Peter Nilsson wrote:
> On Sat, 26 Mar 2022, Jeff Law via Binutils wrote:
> > The v850 gcc testsuite started failing test vrp13 about a week ago. This was
> > ultimately tracked down on a bug in the simulator's handling of the MUL
> > instruction (a change in the compiler resulted in the test no longer
> > compile-time optimized away).
> >
> > MUL on the v850 is a 32x32->64 multiply with the hi/low parts of the result
> > going into distinct registers.? ?? mul r1, r2, r3 is r1xr2 -> (r3, r2) where
> > r2 holds the low 32 bits of the result and r3 holds the high 32 bits of the
> > result.
> >
> > We had inputs of 2, -10 and produced an output of 20.? Opps ;-)
> >
> > The v850 is a 32bit processor using 2s complement.? So to check if a value is
> > negative we merely need to check if bit 0x80000000 is on. Anything else is
> > just introducing dependencies on the host system's types, argument promotions,
> > etc, which is precisely what happened in this case.
> >
> > No new testcase for the simulator.? Not only does vrp13 test this, but about
> > 1800 other tests which have flipped from failing to passing in the GCC
> > testsuite...? So I can say it's being tested and if it were to regress again,
> > we'd catch it.
> >
> > OK for the trunk?
> 
> OK if you mention the gcc test-suite in the commit message or a
> comment, but please (re)consider adding a self-contained (i.e.
> just binutils+sim) test-case.  Suggested inspiration: "git grep
> sim" in the CRIS sim test-suite, though without a "dump
> register" framework macro you need to compare with expected
> results instead of matching output.

that was going to be my feedback as well.  v850 already has a testsuite
for the sim, so adding another testcase shouldn't be that hard.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Fix for MUL instruction on the v850

[Jeff Law]

On 3/28/2022 9:12 PM, Mike Frysinger wrote:
> On 28 Mar 2022 09:38, Hans-Peter Nilsson wrote:
>> On Sat, 26 Mar 2022, Jeff Law via Binutils wrote:
>>> The v850 gcc testsuite started failing test vrp13 about a week ago. This was
>>> ultimately tracked down on a bug in the simulator's handling of the MUL
>>> instruction (a change in the compiler resulted in the test no longer
>>> compile-time optimized away).
>>>
>>> MUL on the v850 is a 32x32->64 multiply with the hi/low parts of the result
>>> going into distinct registers.? ?? mul r1, r2, r3 is r1xr2 -> (r3, r2) where
>>> r2 holds the low 32 bits of the result and r3 holds the high 32 bits of the
>>> result.
>>>
>>> We had inputs of 2, -10 and produced an output of 20.? Opps ;-)
>>>
>>> The v850 is a 32bit processor using 2s complement.? So to check if a value is
>>> negative we merely need to check if bit 0x80000000 is on. Anything else is
>>> just introducing dependencies on the host system's types, argument promotions,
>>> etc, which is precisely what happened in this case.
>>>
>>> No new testcase for the simulator.? Not only does vrp13 test this, but about
>>> 1800 other tests which have flipped from failing to passing in the GCC
>>> testsuite...? So I can say it's being tested and if it were to regress again,
>>> we'd catch it.
>>>
>>> OK for the trunk?
>> OK if you mention the gcc test-suite in the commit message or a
>> comment, but please (re)consider adding a self-contained (i.e.
>> just binutils+sim) test-case.  Suggested inspiration: "git grep
>> sim" in the CRIS sim test-suite, though without a "dump
>> register" framework macro you need to compare with expected
>> results instead of matching output.
> that was going to be my feedback as well.  v850 already has a testsuite
> for the sim, so adding another testcase shouldn't be that hard.
Fair enough.  I'll cover it with a sim test.

jeff

Re: Fix for MUL instruction on the v850

[Jeff Law]

On 3/28/2022 9:12 PM, Mike Frysinger wrote:
> On 28 Mar 2022 09:38, Hans-Peter Nilsson wrote:
>> On Sat, 26 Mar 2022, Jeff Law via Binutils wrote:
>>> The v850 gcc testsuite started failing test vrp13 about a week ago. This was
>>> ultimately tracked down on a bug in the simulator's handling of the MUL
>>> instruction (a change in the compiler resulted in the test no longer
>>> compile-time optimized away).
>>>
>>> MUL on the v850 is a 32x32->64 multiply with the hi/low parts of the result
>>> going into distinct registers.? ?? mul r1, r2, r3 is r1xr2 -> (r3, r2) where
>>> r2 holds the low 32 bits of the result and r3 holds the high 32 bits of the
>>> result.
>>>
>>> We had inputs of 2, -10 and produced an output of 20.? Opps ;-)
>>>
>>> The v850 is a 32bit processor using 2s complement.? So to check if a value is
>>> negative we merely need to check if bit 0x80000000 is on. Anything else is
>>> just introducing dependencies on the host system's types, argument promotions,
>>> etc, which is precisely what happened in this case.
>>>
>>> No new testcase for the simulator.? Not only does vrp13 test this, but about
>>> 1800 other tests which have flipped from failing to passing in the GCC
>>> testsuite...? So I can say it's being tested and if it were to regress again,
>>> we'd catch it.
>>>
>>> OK for the trunk?
>> OK if you mention the gcc test-suite in the commit message or a
>> comment, but please (re)consider adding a self-contained (i.e.
>> just binutils+sim) test-case.  Suggested inspiration: "git grep
>> sim" in the CRIS sim test-suite, though without a "dump
>> register" framework macro you need to compare with expected
>> results instead of matching output.
> that was going to be my feedback as well.  v850 already has a testsuite
> for the sim, so adding another testcase shouldn't be that hard.
Pushed with a trivial new test.

Thanks,

Jeff