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

[Nils Eliasson]

Looks good!

What benchmark did you use? Is it in the microbenchmark suite?

//N

On 2019-02-15 12:49, Viswanathan, Sandhya wrote:
> 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