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

[Lindenmaier, Goetz]

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