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

[Tobias Hartmann]

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