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

Tue Sep 24 22:43:26 UTC 2019

On 24/09/2019 15:08, Vladimir Ivanov wrote:
> 
>> Webrev: http://cr.openjdk.java.net/~jbhateja/8226721/webrev.07/
> 
> Overall, looks good.

On a second thought, IR shape looks too x86-specific:

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;

+  case vmIntrinsics::_ceil:
+  case vmIntrinsics::_floor:
+  case vmIntrinsics::_rint:   return 
Matcher::match_rule_supported(Op_RoundDoubleMode) ? 
inline_double_math(id) : false;

src/hotspot/cpu/x86/x86.ad:

+instruct roundD_reg(legRegD dst, legRegD src, immU8 rmode) %{
+  predicate(UseSSE>=4);
+  match(Set dst (RoundDoubleMode src rmode));
+  format %{ "roundsd  $dst, $src" %}
+  ins_cost(150);
+  ins_encode %{
+    __ roundsd($dst$$XMMRegister, $src$$XMMRegister, $rmode$$constant);
+  %}
+  ins_pipe(pipe_slow);
+%}

src/hotspot/cpu/x86/assembler_x86.cpp:
+void Assembler::roundsd(XMMRegister dst, XMMRegister src, int32_t rmode) {
...
+  emit_int8((unsigned char)rmode);
+}

I'd prefer to see dedicated node types for every operation instead and 
encoding details to be encapsulated in AD files.

As an alternative, you can put rounding mode constant into 
RoundDoubleModeNode itself (as a field storing an enum of different 
modes), but it doesn't help if a platform has different instructions for 
different rounding modes and need to check for their presence.

Best regards,
Vladimir Ivanov

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