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

Viswanathan, Sandhya sandhya.viswanathan at intel.com
Wed Sep 25 17:25:56 UTC 2019 

Hi Vladimir,

Dedicated nodes would result in larger libjvm binary. I think the alternative you suggest with the enum for the rounding mode would be better. 

Best Regards,
Sandhya


-----Original Message-----
From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com> 
Sent: Tuesday, September 24, 2019 3:43 PM
To: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; Tobias Hartmann <tobias.hartmann at oracle.com>; Vladimir Kozlov <vladimir.kozlov at oracle.com>
Cc: hotspot-compiler-dev at openjdk.java.net
Subject: Re: 8226721: Missing intrinsics for Math.ceil, floor, rint



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/sha
> re/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

More information about the hotspot-compiler-dev mailing list