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

[Vladimir Kozlov]

I run tier1-3 testing with these changes.
All passed except compiler/intrinsics/math/TestFpMinMaxIntrinsics.java failed on SPARC:

java.lang.reflect.InvocationTargetException
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
...
Caused by: java.lang.StackOverflowError
	at compiler.intrinsics.math.TestFpMinMaxIntrinsics$Node.toString(TestFpMinMaxIntrinsics.java:262)
	at java.base/java.lang.invoke.StringConcatFactory$Stringifiers$ObjectStringifier.valueOf(StringConcatFactory.java:1702)
	at compiler.intrinsics.math.TestFpMinMaxIntrinsics$Node.toString(TestFpMinMaxIntrinsics.java:264)
	at java.base/java.lang.invoke.StringConcatFactory$Stringifiers$ObjectStringifier.valueOf(StringConcatFactory.java:1702)
	at compiler.intrinsics.math.TestFpMinMaxIntrinsics$Node.toString(TestFpMinMaxIntrinsics.java:264)

Looks like deep recursion.

It failed for last new @run commands with -XX:CompileCommand=dontinline,TestFpMinMaxIntrinsics.min* 
TestFpMinMaxIntrinsics sortedSearchTree 1

Vladimir

On 3/6/19 4:55 PM, Vladimir Kozlov wrote:
> Okay. Lets push this version. Do you need sponsor to push?
> 
> Thanks,
> Vladimir
> 
> On 3/6/19 2:25 PM, B. Blaser wrote:
>> Here it is:
>>
>> http://cr.openjdk.java.net/~bsrbnd/jdk8217561/webrev.06/
>>
>> Any feedback is welcome (jdk/submit report is good),
>> Bernard
>>
>> On Wed, 6 Mar 2019 at 20:57, B. Blaser <bsrbnd at gmail.com> wrote:
>>>
>>> Hi Vladimir,
>>>
>>> I'd like to keep branch predictions commented out until method data is
>>> collected per call-site because current statistics aren't accurate
>>> enough to really improve the following numbers.
>>>
>>> I tried Math.min(float) with the current patch [1] on both standard
>>> and reduction scenarios [2] for negative zero, zero, constant and
>>> random arrays (NaN being rather uncommon). I had to make an average
>>> between min(a,b) and its mirror min(b,a) for reductions because of the
>>> asymmetrical API implementation.
>>>
>>> To summarize:
>>>
>>>                |            pattern          |          array          |
>>>                | blend/min/max | one ucomisd | +/-0.0 | const | random |
>>> --------------|---------------|-------------|--------|-------|--------|
>>> predictable   |    8% gain    |   unused    |  yes   |  yes  |  no    |
>>> unpredictable |   57% gain    |   unused    |  no    |  no   |  yes   |
>>> reduction     |    unused     |  25% gain   |  yes   |  yes  |  yes   |
>>>
>>> We see that the suggested fix to use 'ucomisd' for reductions and
>>> 'blend/min/max' otherwise is always faster than before. I'll prepare
>>> the final webrev based on all JDK-8217561 changesets very soon.
>>>
>>> Thanks,
>>> Bernard
>>>
>>> [1] http://hg.openjdk.java.net/jdk/submit/log?rev=branch%28%22JDK-8217561%22%29
>>> [2] 
>>> http://hg.openjdk.java.net/jdk/submit/file/ab2b1418f0db/test/micro/org/openjdk/bench/vm/compiler/FpMinMaxIntrinsics.java
>>>
>>>
>>> On Wed, 6 Mar 2019 at 19:03, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>
>>>> Hi Bernard,
>>>>
>>>> Can you prepare final patch for review? Changesets are good to see incremental changes but I already lost what whole
>>>> changes are.
>>>>
>>>> Also in latest changeset branch prediction code in library_call.cpp is commented. Is this what you want in final 
>>>> changes?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 3/4/19 1:15 PM, B. Blaser wrote:
>>>>> On Sat, 2 Mar 2019 at 20:51, Bhateja, Jatin <jatin.bhateja at intel.com> wrote:
>>>>>>
>>>>>> Having multiple selection patterns based on node properties is good if we have
>>>>>> optimized selection patterns with and without properties (in this case reduction)
>>>>>
>>>>> Pushed to jdk/submit as third changeset on branch JDK-8217561:
>>>>>
>>>>> http://hg.openjdk.java.net/jdk/submit/rev/9aa98249f99c
>>>>>
>>>>> I think this is our best solution, could we have a Reviewer feedback
>>>>> for this (hotspot:tier1 is OK on x86_64 xeon)?
>>>>>
>>>>> Thanks,
>>>>> Bernard
>>>>>