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

[Lindenmaier, Goetz]

Thanks for editing this once more, looks good!
And good to have the gtest.

Best regards,
  Goetz.



-----Original Message-----
From: Rickard Bäckman [mailto:rickard.backman at oracle.com] 
Sent: Tuesday, January 16, 2018 2:07 PM
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
Cc: Tobias Hartmann <tobias.hartmann at oracle.com>; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; hs-comp-dev <hotspot-compiler-dev at openjdk.java.net>
Subject: Re: RFR (XS): 8191915: JCK tests produce incorrect results with C2

Thank you for the input.
Here is an updated version.

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

/R

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