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

[Tobias Hartmann]

Hi Rickard,

On 16.01.2018 14:07, Rickard Bäckman wrote:
> http://cr.openjdk.java.net/~rbackman/8191915.4/

Looks good to me!

Little typo in mathexactnode.cpp:136 "and break the" -> "and breaks the" (no new webrev required).

Best regards,
Tobias


> On 01/12, Lindenmaier, Goetz wrote:
>> Oops, in my code, one of the second val2 == 0 should compare for ==1 ...
>>
>> Best regards,
>>   Goetz.
>>
>>> -----Original Message-----
>>> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
>>> bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
>>> Sent: Freitag, 12. Januar 2018 15:36
>>> To: Rickard Bäckman <rickard.backman at oracle.com>; Tobias Hartmann
>>> <tobias.hartmann at oracle.com>
>>> Cc: hs-comp-dev <hotspot-compiler-dev at openjdk.java.net>
>>> Subject: RE: RFR (XS): 8191915: JCK tests produce incorrect results with C2
>>>
>>> Hi Rickard,
>>>
>>> I had a look at the change and it seems fine to me assuming the following:
>>>  * The results of overflowing signed multiplication is undefined in C++.
>>>     (Thus, compilers may optimize x*y/x  to y.)
>>>  * The results of overflowing unsigned multiplication is well defined in C++.
>>>  * Java's math excact requires that -1 * min_jlong is signaled as overflow.
>>>
>>> But I think it would be much easier to understand
>>> if it's noted down differently. The only numbers
>>> min_jlong can be multiplied with legally are 0 and 1.
>>>
>>> Further, I'm not sure why the & CONST64(0xFFFFFFFF00000000))
>>> is needed at all.  It saves the division in some cases, but
>>> I don't think this method is performance relevant in any case.
>>> It just makes reading the code difficult ...
>>>
>>> bool OverflowMulLNode::will_overflow(jlong val1, jlong val2) const {
>>>   // x*1 and x*0 never overflow. Even not for min_jlong.
>>>   if (val1 == 0 || val2 == 0 ||
>>>       val1 == 1 || val2 == 0) {
>>>     return false;
>>>   }
>>>   // x*min_jlong for x not in { 0, 1 } overflows.
>>>   // This holds for -1, too: -1*min_jlong is undefined.
>>>   if (val1 == min_jlong || val2 == min_jlong) {
>>>     return true;
>>>   }
>>>
>>>    // If (x*y)/x == y there is no overflow.
>>>   //
>>>   // The multiplication here is done as unsigned to avoid undefined behaviour
>>> which
>>>   // can be used by the compiler to assume that the check further down
>>> (result / val2 != val1)
>>>   // is always false. This breaks the overflow check.
>>>   julong v1 = (julong) val1;
>>>   julong v2 = (julong) val2;
>>>   julong tmp = v1 * v2;
>>>   jlong result = (jlong) tmp;
>>>   if (result / val2 != val1) {
>>>     return true;
>>>   }
>>>
>>>   return false;
>>> }
>>>
>>>
>>> Best regards,
>>>   Goetz.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
>>>> bounces at openjdk.java.net] On Behalf Of Rickard Bäckman
>>>> Sent: Freitag, 12. Januar 2018 14:22
>>>> To: Tobias Hartmann <tobias.hartmann at oracle.com>
>>>> Cc: hs-comp-dev <hotspot-compiler-dev at openjdk.java.net>
>>>> Subject: Re: RFR (XS): 8191915: JCK tests produce incorrect results with C2
>>>>
>>>> Added a few comments. I still think I need a second reviewer to OK this.
>>>>
>>>> http://cr.openjdk.java.net/~rbackman/8191915.3/
>>>>
>>>> /R
>>>>
>>>> On 01/11, Tobias Hartmann wrote:
>>>>> Hi Rickard,
>>>>>
>>>>> On 11.01.2018 10:30, Rickard Bäckman wrote:
>>>>>> http://cr.openjdk.java.net/~rbackman/8191915.2/
>>>>>
>>>>> Looks correct to me. Maybe add a comment explaining all the casting.
>>>>>
>>>>> Best regards,
>>>>> Tobias
>>>>>
>>>>>
>>>>>> On 01/10, Andrew Haley wrote:
>>>>>>> On 10/01/18 14:17, Rickard Bäckman wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> It's still wrong because
>>>>>>>
>>>>>>>     jlong ax = (val1 < 0 ? -val1 : val1);
>>>>>>>     jlong ay = (val2 < 0 ? -val2 : val2);
>>>>>>>
>>>>>>> is undefined when val1 or val2 is Long.MIN_VALUE.
>>>>>>>
>>>>>>> --
>>>>>>> Andrew Haley
>>>>>>> Java Platform Lead Engineer
>>>>>>> Red Hat UK Ltd. <https://www.redhat.com>
>>>>>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>>>>>> /R
>>>>>>
> /R
>