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

[Vladimir Ivanov]

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

And one more question:

src/hotspot/share/opto/library_call.cpp:

+  case vmIntrinsics::_ceil:   n = new RoundDoubleModeNode(control() , 
arg, makecon(TypeInt::make(2))); break;
+  case vmIntrinsics::_floor:  n = new RoundDoubleModeNode(control() , 
arg, makecon(TypeInt::make(1))); break;
+  case vmIntrinsics::_rint:   n = new RoundDoubleModeNode(control() , 
arg, makecon(TypeInt::make(0))); break;

Why does RoundDoubleMode require control?

Best regards,
Vladimir Ivanov

> 
> 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