[PATCH] 8217561 : X86: Add floating-point Math.min/max intrinsics, approval request

[Viswanathan, Sandhya]

Hi Vladimir/Nils, Could one of you please review and approve the patch? You have looked at the previous versions of this patch so hoping it will be a quick review for you.

The updated patch from Bernard looks ok to me. I am not an official reviewer though.

https://bugs.openjdk.java.net/browse/JDK-8217561
http://cr.openjdk.java.net/~bsrbnd/jdk8217561/webrev.04/

Best Regards,
Sandhya


-----Original Message-----
From: B. Blaser [mailto:bsrbnd at gmail.com] 
Sent: Monday, February 11, 2019 3:17 AM
To: Bhateja, Jatin <jatin.bhateja at intel.com>
Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; hotspot-compiler-dev at openjdk.java.net; Deshpande, Vivek R <vivek.r.deshpande at intel.com>
Subject: Re: [PATCH] 8217561 : X86: Add floating-point Math.min/max intrinsics

Hi Jatin, All,

Thanks for your first contribution, I'll push it to the main-line
repository once approved by a Reviewer which we are waiting for.

Last note about x86_32:

"Only 64-bit Java virtual machines (JVMs) are certified"

see:

https://www.oracle.com/technetwork/java/javase/documentation/jdk11certconfig-5069638.html

which is why I decided to drop this part in the latest webrev:

http://cr.openjdk.java.net/~bsrbnd/jdk8217561/webrev.04/

There's definitely no use taking care of implementing, testing and
maintaining intrinsics for legacy architectures.

Regards,
Bernard


On Sun, 10 Feb 2019 at 10:10, Bhateja, Jatin <jatin.bhateja at intel.com> wrote:
>
> Hi Bernard,
>
> Thanks for your detailed review, I do not have commit access yet since this is my first contribution.
> I request you to kindly upstream the changes from link you shared and add me as a contributor.
>
> Regards,
> Jatin
>
> > -----Original Message-----
> > From: B. Blaser [mailto:bsrbnd at gmail.com]
> > Sent: Sunday, February 10, 2019 12:13 AM
> > To: Bhateja, Jatin <jatin.bhateja at intel.com>
> > Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Viswanathan, Sandhya
> > <sandhya.viswanathan at intel.com>; hotspot-compiler-
> > dev at openjdk.java.net; Deshpande, Vivek R <vivek.r.deshpande at intel.com>
> > Subject: Re: [PATCH] 8217561 : X86: Add floating-point Math.min/max
> > intrinsics
> >
> > Hi Jatin, All,
> >
> > On Fri, 8 Feb 2019 at 06:55, Bhateja, Jatin <jatin.bhateja at intel.com> wrote:
> > >
> > > Hi Bernard, All,
> > >
> > > Please find below the link to updated patch.
> > >
> > > http://cr.openjdk.java.net/~sviswanathan/Jatin/8217561/webrev.03/
> > >
> > > Regards,
> > > Jatin
> >
> > Your patch seems quite good to me.
> >
> > However, I'd like to suggest the following variant (which is mostly a
> > copy/paste of your code) focusing on x86_64 as I don't think that taking care
> > of x86_32 is worth the additional complexity. It uses the min/max scalar
> > variant which is conceptually better than the packed one. Regarding the test
> > case, I simply added a main loop along with an
> Ok, but 32 bit execution context we do have access to XMM0-XMM7 and new
> instruction sequence did show gain there.
>
> > x86_64 test configuration (similar to yours) without modifying the existing
> > AArch64 parameters:
> >
> > http://cr.openjdk.java.net/~bsrbnd/jdk8217561/webrev.04/
> >
> > This variant also includes the following small code improvements:
> >
> > * copyright years updated where necessary
> > * useless assertions removed ('vex_prefix_and_encode' already checks the
> > register encoding in 'legacy_mode')
> > * constants used to improve readability like 0 -> AVX_128bit, 0x3 -> _false, ...
> > * several comments fixed like
> >   * 'java.lang.Max()' -> 'java.lang.Math.max()', ...
> >   * 'if b < 0' -> 'if b < +0.0'
> > * unnecessary trailing spaces removed
> >
> > Test-tier1 is OK on x86_64 (xeon).
> >
> > Could we then have a Reviewer feedback (and eventually an approval) for
> > this?
>
> Above changes looks good.
> >
> > Thanks,
> > Bernard