RFR (XS): 8191915: JCK tests produce incorrect results with C2

[Rickard Bäckman]

I did the multiply as unsigned and then cast to to signed thing.
Renamed the test to LongMulOverflowTest.

Updated.

http://cr.openjdk.java.net/~rbackman/8191915.1/

I agree that the best solution would be to use compiler builtins but I'm
not sure all the compilers support them and makes portability a pain.

Thanks
/R

On 12/14, Tobias Hartmann wrote:
> On 14.12.2017 11:54, Rickard Bäckman wrote:
> > Yes you are right, it doesn't work for all values. Seems to work for
> > negatives though?
> 
> I think for negative values this does not work at all. If you set sv1 = -5 in my example code, you get:
> 
> -10
> 18446744073709551606
> Overflow detected
> 
> Because we cast a negative value to unsigned.
> 
> > However casting the result back to signed after doing the multiplication
> > and doing the division with the signed values get us back to getting the
> > overflow check correct and no undefined behaviour as far as I can tell?
> 
> Yes, that seems to work. The best solution would be to use the corresponding compiler build-in functions (for example,
> __builtin_smulll_overflow for gcc [1]) but I guess not all compilers support that.
> 
> Best regards,
> Tobias
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
> 
> > On 12/14, Tobias Hartmann wrote:
> >> Hi Rickard,
> >>
> >> I don't understand why casting from signed to unsigned is correct here because if there's an overflow of the signed
> >> range, there is not necessarily and overflow of the unsigned range, right?
> >>
> >> For example, this program:
> >>
> >>   signed long long sv1 = LONG_MAX/2 + 1;
> >>   signed long long sv2 = 2;
> >>   signed long long sR = sv1 * sv2;
> >>   printf("%li\n", sR);
> >>
> >>   unsigned long long uv1 = (unsigned long long) sv1;	
> >>   unsigned long long uv2 = (unsigned long long) sv2;
> >>   unsigned long long uR = uv1 * uv2;
> >>   printf("%lu\n", uR);
> >>
> >>   if (uR / uv1 != uv2) {
> >>     printf("Overflow detected");
> >>   } else {
> >>     printf("No overflow detected");
> >>   }
> >>
> >> Output:
> >>
> >>   -9223372036854775808
> >>   9223372036854775808
> >>   No overflow detected
> >>
> >> So there is an overflow of the signed long but the unsigned long does not overflow and thus an overflow is not detected.
> >> Do I miss something?
> >>
> >> Regarding the jtreg test: Please do not use bug numbers as test names but use something more meaningful like
> >> TestLongMulOverflow or similar. I would also suggest to move the allocation of the test instance out of the loop and
> >> check if 500.000 iterations are really necessary to trigger compilation (you can run with -Xbatch and maybe
> >> -XX:-TieredCompilation).
> >>
> >> Best regards,
> >> Tobias
> >>
> >> On 14.12.2017 08:30, Rickard Bäckman wrote:
> >>> Hi, can I please have this change reviewed?
> >>>
> >>> Summary is that we had an overflow check for mulitplyExact(long, long)
> >>> intrinsic that relied on undefined behaviour. clang / llvm started
> >>> optimizing that so that if a = b * c then a / b must be equal to c and
> >>> removed half of the overflow check. 
> >>>
> >>> Changing the multiplication and division to be unsigned.
> >>>
> >>> Webrev: http://cr.openjdk.java.net/~rbackman/8191915/
> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191915
> >>>
> >>> Thanks
> >>> /R
> >>>