8226721: Missing intrinsics for Math.ceil, floor, rint

[Vladimir Ivanov]

> Webrev: http://cr.openjdk.java.net/~jbhateja/8226721/webrev.07/

Overall, looks good.

I'd like to ensure that the tests cover important corner cases. For 
example, Math.ceil() has the following:
 
http://hg.openjdk.java.net/jdk/jdk/file/c3b93d6603f5/src/java.base/share/classes/java/lang/Math.java#l425

If the argument value is less than zero but greater than -1.0, then the 
result is negative zero.

(There are other corner cases there.)

Considering you add C2-specific implementation, it's important to keep 
it in agreement with fallback implementation (StrictMath.*) for both 
scalar and vectorized cases.

Also, speaking about testing, does it make sense to add a configuration 
with auto-vectorization completely turned off (to check scalar case)?

   test/hotspot/jtreg/compiler/c2/cr6340864/Test*Vect.java
   test/hotspot/jtreg/compiler/c2//cr7192963/Test*Vect.java


Small nit:

src/hotspot/share/opto/convertnode.cpp

+const Type* RoundDoubleModeNode::Value(PhaseGVN* phase) const {
+  return Type::DOUBLE;
+}

It's redundant since RoundDoubleModeNode::bottom_type() is already 
Type::DOUBLE.

+class RoundDoubleModeNode: public Node {
...
+  virtual const Type *bottom_type() const { return Type::DOUBLE; }


Best regards,
Vladimir Ivanov


> 
> Nils has reviewed it and taken through a round of testing.
> 
> Best Regards,
> Sandhya
> 
> -----Original Message-----
> From: hotspot-compiler-dev <hotspot-compiler-dev-bounces at openjdk.java.net> On Behalf Of Nils Eliasson
> Sent: Thursday, September 19, 2019 7:13 AM
> To: hotspot-compiler-dev at openjdk.java.net
> Subject: Re: 8226721: Missing intrinsics for Math.ceil, floor, rint
> 
> Hi Jatin,
> 
> You patch looks good! Reviewed.
> 
> We need a second review before I can push this.
> 
> // Nils
> 
> 
> On 2019-09-19 11:02, Nils Eliasson wrote:
>> Yes, I will sponsor it as soon as I have reviewed it.
>>
>> // Nils
>>
>> On 2019-09-18 17:49, Bhateja, Jatin wrote:
>>> Hi Nils,
>>>
>>> Thanks a lot for testing the patch.
>>> I have uploaded the re-based patch at following link.
>>>
>>> http://cr.openjdk.java.net/~jbhateja/8226721/webrev.07/
>>>
>>> It will be helpful if you can sponsor this patch.
>>>
>>> Best Regards,
>>> Jatin
>>>
>>>> -----Original Message-----
>>>> From: hotspot-compiler-dev <hotspot-compiler-dev-
>>>> bounces at openjdk.java.net> On Behalf Of Nils Eliasson
>>>> Sent: Thursday, September 12, 2019 5:07 PM
>>>> To: hotspot-compiler-dev at openjdk.java.net
>>>> Subject: Re: 8226721: Missing intrinsics for Math.ceil, floor, rint
>>>>
>>>> Testing passed.
>>>>
>>>> // Nils
>>>>
>>>> On 2019-09-11 21:59, Nils Eliasson wrote:
>>>>> Hi Jatin,
>>>>>
>>>>> Sorry for the delay. I have started testing now.
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Nils
>>>>>
>>>>> On 2019-09-09 18:45, Bhateja, Jatin wrote:
>>>>>> Hi Nils,
>>>>>>
>>>>>> I have taken care of other C2 specific review comments over this
>>>>>> patch, following is the link to updated patch.
>>>>>> http://cr.openjdk.java.net/~jbhateja/8226721/webrev.06/webrev/
>>>>>>
>>>>>> It will be helpful if you can run these though your tests.
>>>>>>
>>>>>> Best Regards,
>>>>>> Jatin
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: hotspot-compiler-dev <hotspot-compiler-dev-
>>>>>>> bounces at openjdk.java.net> On Behalf Of Bhateja, Jatin
>>>>>>> Sent: Wednesday, September 4, 2019 7:17 AM
>>>>>>> To: Nils Eliasson <nils.eliasson at oracle.com>; hotspot-compiler-
>>>>>>> dev at openjdk.java.net
>>>>>>> Subject: Re: 8226721: Missing intrinsics for Math.ceil, floor,
>>>>>>> rint
>>>>>>>
>>>>>>> Hi Nils,
>>>>>>>
>>>>>>> I will convert new vector patterns added by this patch to generic
>>>>>>> operands patterns once this lands into mainline, will update the
>>>>>>> other patch.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Jatin
>>>>>>>
>>>>>>>
>>>>>>> -------- Original message --------
>>>>>>> From: Nils Eliasson <nils.eliasson at oracle.com>
>>>>>>> Date: 04/09/2019 00:55 (GMT+05:30)
>>>>>>> To: hotspot-compiler-dev at openjdk.java.net
>>>>>>> Subject: Re: 8226721: Missing intrinsics for Math.ceil, floor,
>>>>>>> rint
>>>>>>>
>>>>>>> Hi Jatin,
>>>>>>>
>>>>>>> Thanks for adding these.
>>>>>>>
>>>>>>> Does this patch conflict with your patch that adds generic vector
>>>>>>> operands?
>>>>>>> Do you have any preferred order?
>>>>>>>
>>>>>>> I can take them for a spin through our testing.
>>>>>>>
>>>>>>> Best regards,
>>>>>>>
>>>>>>> Nils
>>>>>>>
>>>>>>>
>>>>>>> On 2019-09-03 11:41, Bhateja, Jatin wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Please find a patch with the following changes:-
>>>>>>>> 1) Intrincifiation for Math.ceil/floor/rint.
>>>>>>>> 2) Auto-vectorizer handling.
>>>>>>>>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8226721
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jbhateja/8226721/webrev.05
>>>>>>>>
>>>>>>>> Kindly review it.
>>>>>>>>
>>>>>>>> Best Regards,
>>>>>>>> Jatin