Re: Fix for MUL instruction on the v850

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