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

[Lindenmaier, Goetz]

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