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

Fri Sep 27 15:29:43 UTC 2019

Hi Vladimir,

Pease find below an updated patch with your comments resolved.

http://cr.openjdk.java.net/~jbhateja/8226721/webrev.08/

1)  Create a separate nodes for each operation :  This will mean multiplicity in instruction patterns, passing an extra rmode option as an int constant factorizes the patterns.

2)  Putting the rmode inside the ideal node instead of passing the int constant node :  This would again boil down to multiplicity in patterns since ideal field based check will be part of predicate. 
      Unless we make a change in ADLC to pass IDEAL attribute (rmode) to MachNode from where it can be further passed down to assembler.

3)  Usage of control edge: Addressed, since we do not need to additionally constrain the scheduler for these RoundingDoubleModeNode , data dependency due to inputs is sufficient for adding the scheduling constraint,  as a side observation similar handling of passing control edge is there in existing SqrtDNode().

4)   Adding extra tests to cover corner cases : Addressed.

For point #1 and #2 we are already working over reducing the instruction patterns through various approaches and so these will be addressed as a part of that effort.  

Patch has been sponsored and reviewed by Nils Ellison,  getting a green signal from you will allow this to land to repository. 

Best Regards,
Jatin

> -----Original Message-----
> From: hotspot-compiler-dev <hotspot-compiler-dev-
> bounces at openjdk.java.net> On Behalf Of John Rose
> Sent: Friday, September 27, 2019 11:06 AM
> To: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: Re: 8226721: Missing intrinsics for Math.ceil, floor, rint
> 
> A good way to address that would be to regularize a new concept of sub-
> code. A node can have an opcode and some opcodes also support sub-
> codes. The meaning of the sub-code depends on the op-code but is regularly
> plumbed through ADLC layers. That would cover the present case and also
> maybe allow us to do more folding of AD rules.
> 
> > On Sep 26, 2019, at 9:00 PM, Vladimir Ivanov
> <vladimir.x.ivanov at oracle.com> wrote:
> >
> > Hi Sandhya,
> >
> >> Dedicated nodes would result in larger libjvm binary. I think the
> alternative you suggest with the enum for the rounding mode would be
> better.
> >
> > Yes, though dedicated nodes look most natural in the code, they add up
> and increase libjvm. Using a shared node type is better in that respect, but
> some additional hurdles may arise.
> >
> > For example, Matcher::match_rule_supported(int opcode) doesn't take
> rounding mode into account:
> >
> > > +  case vmIntrinsics::_ceil:
> > > +  case vmIntrinsics::_floor:
> > > +  case vmIntrinsics::_rint:   return
> > > Matcher::match_rule_supported(Op_RoundDoubleMode) ?
> > > inline_double_math(id) : false;
> >
> > As I see in the patch, it's not a problem on x86 since all rounding modes
> (both scalar and vector variants) are uniformly supported (requires SSE>=4
> and AVX>0 respectively).
> >
> > I'm OK with addressing that later, but the more cases we share node types
> the higher the chances we need to take operation specific information into
> account when deciding whether it's supported or not.
> >
> > Best regards,
> > Vladimir Ivanov
> >
> >> -----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/s
> >>> ha
> >>> 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