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

[Eric Caspole]

Hi Jatin,
Could you add to the webrev a JMH[1] benchmark to test/show the 
performance of this new intrinsic? The open JMH collection is in the JDK 
repo at:

./test/micro/

The building instructions are in this file:

doc/testing.md

You will end up with the benchmarks jar here:

images/test/micro/benchmarks.jar

To have a JMH benchmark with this intrinsic contribution would be a big 
help for us to test on the hardware we have here.

Thanks,
Eric

[1] http://openjdk.java.net/projects/code-tools/jmh/



On 2/10/19 04:10, Bhateja, Jatin 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