RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor, rint on Power
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Tue Oct 22 03:50:06 UTC 2019
> http://cr.openjdk.java.net/~mhorie/8231649/webrev.06/
Changes in shared code look good!
Best regards,
Vladimir Ivanov
> ----- Original message -----
> From: "Doerr, Martin" <martin.doerr at sap.com>
> To: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>, Michihiro Horie
> <HORIE at jp.ibm.com>
> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>,
> "ppc-aix-port-dev at openjdk.java.net" <ppc-aix-port-dev at openjdk.java.net>
> Subject: [EXTERNAL] RE: RFR: 8231649: PPC64: Intrinsics for Math.ceil,
> floor, rint on Power
> Date: Mon, Oct 14, 2019 5:27 PM
>
> Hi Vladimir,
>
> that looks much better. Thank you.
>
> Alternatively, the PhaseGVN could get passed instead of the Compile
> pointer. (Prototype is already included via node.hpp.)
> This is already used by some other nodes like
> LoadNode::make(PhaseGVN& gvn, ...
> So this seems to be a common pattern.
>
> Best regards,
> Martin
>
>
> > -----Original Message-----
> > From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> > Sent: Samstag, 12. Oktober 2019 03:27
> > To: Doerr, Martin <martin.doerr at sap.com>; Michihiro Horie
> > <HORIE at jp.ibm.com>
> > Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>; ppc-aix-
> > port-dev at openjdk.java.net
> > Subject: Re: RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor,
> rint on Power
> >
> > I agree that GraphKit as a ctor argument looks weird.
> >
> > What about having a static factory and encapsulate the code there
> instead?
> >
> > convertnode.hpp:
> >
> > RoundDoubleModeNode(Node* in1, ConINode* rmode): Node(0, in1,
> > rmode) {}
> >
> > static RoundDoubleModeNode make(Compile* C, Node* arg,
> > RoundDoubleModeNode::RoundingMode rmode);
> >
> >
> > convertnode.cpp:
> >
> > RoundDoubleModeNode* RoundDoubleModeNode::make(Compile* C,
> > Node* arg,
> > RoundDoubleModeNode::RoundingMode rmode) {
> > ConINode* rm = C->initial_gvn()->intcon(rmode);
> > return new RoundDoubleModeNode(arg, rm);
> > }
> >
> >
> > library_call.cpp:
> >
> > case vmIntrinsics::_ceil: n = RoundDoubleModeNode::make(C, arg,
> > RoundDoubleModeNode::rmode_ceil);
> >
> >
> >
> >
> > Best regards,
> > Vladimir Ivanov
> >
> > On 09/10/2019 12:29, Doerr, Martin wrote:
> > > Hi Vladimir and Michihiro,
> > >
> > >> ConINode::make() allocates a new node which has to be seen by GVN, so
> > >> there's a type recorded for it.
> > > Makes sense.
> > >
> > > I'd prefer
> > > RoundDoubleModeNode(Node *in1, Node *rmode): Node(0, in1, rmode)
> > {}
> > >
> > > and
> > > case vmIntrinsics::_ceil: n = new RoundDoubleModeNode(arg,
> > makecon(TypeInt::make(RoundDoubleModeNode::rmode_ceil))); break;
> > > case vmIntrinsics::_floor: n = new RoundDoubleModeNode(arg,
> > makecon(TypeInt::make(RoundDoubleModeNode::rmode_floor))); break;
> > > case vmIntrinsics::_rint: n = new RoundDoubleModeNode(arg,
> > makecon(TypeInt::make(RoundDoubleModeNode::rmode_rint))); break;
> > >
> > > That avoids additional includes in convertnode.hpp which can easily
> lead to
> > cyclic dependencies for future changes.
> > >
> > > Best regards,
> > > Martin
> > >
> > >
> > >> -----Original Message-----
> > >> From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> > >> Sent: Dienstag, 8. Oktober 2019 12:31
> > >> To: Doerr, Martin <martin.doerr at sap.com>; Michihiro Horie
> > >> <HORIE at jp.ibm.com>
> > >> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>; ppc-
> > aix-
> > >> port-dev at openjdk.java.net
> > >> Subject: Re: RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor,
> rint on
> > Power
> > >>
> > >>
> > >>
> > >> On 08/10/2019 13:14, Doerr, Martin wrote:
> > >>> Hi Vladimir and Michihiro,
> > >>>
> > >>> I like webrev.04 more than webrev.05.
> > >>> I think it's not good to include graphKit.hpp in convertnode.hpp.
> This
> > >> creates a weird dependency.
> > >>> What was the reason for this change?
> > >>
> > >> webrev.04 has a bug. Quote from earlier email:
> > >>
> > >> "src/hotspot/share/opto/convertnode.hpp:
> > >>
> > >> + RoundDoubleModeNode(Node *in1, int rmode): Node(0, in1,
> > >> ConINode::make(rmode)) {}
> > >>
> > >> ConINode::make() allocates a new node which has to be seen by GVN, so
> > >> there's a type recorded for it. That's why I mentioned GraphKit*. But
> > >> you can use PhaseGVN & intcon as well."
> > >>
> > >> Alternatively, Compile* can be used as well.
> > >>
> > >> Best regards,
> > >> Vladimir Ivanov
> > >>
> > >>>> -----Original Message-----
> > >>>> From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> > >>>> Sent: Dienstag, 8. Oktober 2019 11:47
> > >>>> To: Michihiro Horie <HORIE at jp.ibm.com>
> > >>>> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>;
> > Doerr,
> > >>>> Martin <martin.doerr at sap.com>; ppc-aix-port-dev at openjdk.java.net
> > >>>> Subject: Re: RFR: 8231649: PPC64: Intrinsics for Math.ceil,
> floor, rint on
> > >> Power
> > >>>>
> > >>>>
> > >>>>> _http://cr.openjdk.java.net/~mhorie/8231649/webrev.05/_
> > >>>>
> > >>>> Looks good.
> > >>>>
> > >>>> (Test results are clean.)
> > >>>>
> > >>>> Best regards,
> > >>>> Vladimir Ivanov
> > >>>>
> > >>>>> > I think it would be better to use adlc’s include generator
> instead of
> > >>>>> > including "opto/convertnode.hpp" explicitly in the ad file.
> > >>>>> >
> > >>>>> > src/hotspot/share/adlc/main.cpp
> > >>>>> >
> > >>>>> > AD.addInclude(AD._CPP_file, "opto/cfgnode.hpp");
> > >>>>> >
> > >>>>> > + AD.addInclude(AD._CPP_file, "opto/convertnode.hpp");
> > >>>>> >
> > >>>>> > AD.addInclude(AD._CPP_file, "opto/intrinsicnode.hpp");
> > >>>>> >
> > >>>>> > @Vladimir: Do you agree? Would you do it this way, too?
> > >>>>>
> > >>>>> I prefer current version.
> > >>>>>
> > >>>>> When proposing the refactoring I thought about the change in
> adlc as
> > >>>>> well, but when I saw the patch I noticed that putting it in AD file
> > >>>>> stresses it's a platform-specific dependency.
> > >>>>>
> > >>>>> I think that's the right way to look at it until we get a way
> to funnel
> > >>>>> attributes from ideal nodes to mach nodes and use them during code
> > >>>>> emission.
> > >>>>>
> > >>>>> Best regards,
> > >>>>> Vladimir Ivanov
> > >>>>>
> > >>>>> > *From:*Michihiro Horie <HORIE at jp.ibm.com>
> > >>>>> > *Sent:* Montag, 7. Oktober 2019 17:54
> > >>>>> > *To:* Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> > >>>>> > *Cc:* hotspot compiler <hotspot-compiler-
> > dev at openjdk.java.net>;
> > >>>> Doerr,
> > >>>>> > Martin <martin.doerr at sap.com>; ppc-aix-port-
> > >> dev at openjdk.java.net
> > >>>>> > *Subject:* Re: RFR: 8231649: PPC64: Intrinsics for Math.ceil,
> floor,
> > >>>>> > rint on Power
> > >>>>> >
> > >>>>> > Hi Martin, Vladimir,
> > >>>>> > Thanks a lot for your further feedback, which makes sense.
> > >>>>> >
> > >>>>> > New webrev:
> > >> _http://cr.openjdk.java.net/~mhorie/8231649/webrev.04/_
> > >>>>> >
> > >>>>> > Best regards,
> > >>>>> > Michihiro
> > >>>>> >
> > >>>>> > Inactive hide details for Vladimir Ivanov ---2019/10/07
> 22:51:21--->
> > >>>>> > Fixed webrev is as follows. Thanks, Michihiro.Vladimir Ivanov
> > >>>>> > ---2019/10/07 22:51:21---> Fixed webrev is as follows. Thanks,
> > >> Michihiro.
> > >>>>> >
> > >>>>> > From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com
> > >>>>> > <_mailto:vladimir.x.ivanov at oracle.com_>>
> > >>>>> > To: Michihiro Horie <HORIE at jp.ibm.com
> > >> <_mailto:HORIE at jp.ibm.com_>>,
> > >>>>> > "Doerr, Martin" <martin.doerr at sap.com
> > >>>> <_mailto:martin.doerr at sap.com_>>
> > >>>>> > Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net
> > >>>>> > <_mailto:hotspot-compiler-dev at openjdk.java.net_>>,
> > >>>>> > "ppc-aix-port-dev at openjdk.java.net
> > >>>>> > <_mailto:ppc-aix-port-dev at openjdk.java.net_>"
> > >>>>> > <ppc-aix-port-dev at openjdk.java.net
> > >>>>> > <_mailto:ppc-aix-port-dev at openjdk.java.net_>>
> > >>>>> > Date: 2019/10/07 22:51
> > >>>>> > Subject: [EXTERNAL] Re: RFR: 8231649: PPC64: Intrinsics for
> > Math.ceil,
> > >>>>> > floor, rint on Power
> > >>>>> >
> > >>>>> >
> ------------------------------------------------------------------------
> > >>>>> >
> > >>>>> >
> > >>>>> >
> > >>>>> >
> > >>>>> >
> > >>>>> >> Fixed webrev is as follows.
> > >>>>> >
> > >>>>> > Thanks, Michihiro.
> > >>>>> >
> > >>>>> >> _http://cr.openjdk.java.net/~mhorie/8231649/webrev.03/_
> > >>>>> >
> > >>>>> > src/hotspot/share/opto/convertnode.hpp:
> > >>>>> >
> > >>>>> > + enum RoundingMode { rmode_rint, rmode_floor, rmode_ceil};
> > >>>>> >
> > >>>>> > Please, explicitly initialize the elements. Numbering is
> important
> > since
> > >>>>> > it is aligned with instruction encoding on x86.
> > >>>>> >
> > >>>>> >
> > >>>>> > src/hotspot/share/opto/library_call.cpp:
> > >>>>> >
> > >>>>> > + case vmIntrinsics::_ceil: n = new RoundDoubleModeNode(arg,
> > >>>>> > makecon(TypeInt::make(RoundDoubleModeNode::rmode_ceil)));
> > >>>> break;
> > >>>>> >
> > >>>>> > Please, change RoundDoubleModeNode to accept the enum
> > instead
> > >> and
> > >>>>> > instantiate the ConI node inside it or introduce a static factory
> > >>>>> > (::make(GraphKit*,RoundingMode)) for that purpose.
> > >>>>> >
> > >>>>> >
> > >>>>> > src/hotspot/cpu/ppc/macroAssembler_ppc.hpp:
> > >>>>> >
> > >>>>> > + void math_round(const FloatRegister t, const FloatRegister
> b, int
> > >>>>> rmode);
> > >>>>> > + void math_round_vec(const VectorSRegister t, const
> > >> VectorSRegister
> > >>>> b,
> > >>>>> > int rmode);
> > >>>>> >
> > >>>>> > I suggest to get rid of helpers in MacroAssembler and simply move
> > >> the
> > >>>>> > code into corresponding AD instructions.
> > >>>>> >
> > >>>>> > Best regards,
> > >>>>> > Vladimir Ivanov
> > >>>>> >
> > >>>>> >> Inactive hide details for "Doerr, Martin" ---2019/10/07
> 22:09:15---
> > > I
> > >>>>> >> suggest to put the enum in RoundDoubleModeNode
> > >> (convertn"Doerr,
> > >>>> Martin"
> > >>>>> >> ---2019/10/07 22:09:15---> I suggest to put the enum in
> > >>>>> >> RoundDoubleModeNode (convertnode.hpp) > and lift instruction
> > >>>> selectio
> > >>>>> >>
> > >>>>> >> From: "Doerr, Martin" <martin.doerr at sap.com
> > >>>>> <_mailto:martin.doerr at sap.com_>>
> > >>>>> >> To: Vladimir Ivanov <vladimir.x.ivanov at oracle.com
> > >>>>> <_mailto:vladimir.x.ivanov at oracle.com_>>,
> > >>>>> > Michihiro Horie
> > >>>>> >> <HORIE at jp.ibm.com <_mailto:HORIE at jp.ibm.com_>>
> > >>>>> >> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net
> > >>>>> > <_mailto:hotspot-compiler-dev at openjdk.java.net_>>,
> > >>>>> >> "ppc-aix-port-dev at openjdk.java.net
> > >>>>> > <_mailto:ppc-aix-port-dev at openjdk.java.net_>"
> > >>>>> > <ppc-aix-port-dev at openjdk.java.net
> > >>>>> > <_mailto:ppc-aix-port-dev at openjdk.java.net_>>
> > >>>>> >> Date: 2019/10/07 22:09
> > >>>>> >> Subject: [EXTERNAL] RE: RFR: 8231649: PPC64: Intrinsics for
> > >> Math.ceil,
> > >>>>> >> floor, rint on Power
> > >>>>> >>
> > >>>>> >>
> ------------------------------------------------------------------------
> > >>>>> >>
> > >>>>> >>
> > >>>>> >>
> > >>>>> >> > I suggest to put the enum in RoundDoubleModeNode
> > >>>> (convertnode.hpp)
> > >>>>> >> > and lift instruction selection logic from
> > macroAssembler_ppc.cpp
> > >>>>> into
> > >>>>> >> ppc.ad.
> > >>>>> >> That sounds good.
> > >>>>> >>
> > >>>>> >> Thanks,
> > >>>>> >> Martin
> > >>>>> >>
> > >>>>> >>
> > >>>>> >> > -----Original Message-----
> > >>>>> >> > From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com
> > >>>>> <_mailto:vladimir.x.ivanov at oracle.com_>>
> > >>>>> >> > Sent: Montag, 7. Oktober 2019 12:55
> > >>>>> >> > To: Doerr, Martin <martin.doerr at sap.com
> > >>>>> <_mailto:martin.doerr at sap.com_>>; Michihiro Horie
> > >>>>> >> > <HORIE at jp.ibm.com <_mailto:HORIE at jp.ibm.com_>>
> > >>>>> >> > Cc: hotspot compiler <hotspot-compiler-
> > dev at openjdk.java.net
> > >>>>> > <_mailto:hotspot-compiler-dev at openjdk.java.net_>>; ppc-aix-
> > >>>>> >> > port-dev at openjdk.java.net <_mailto:port-_
> > >> dev at openjdk.java.net>
> > >>>>> >> > Subject: Re: RFR: 8231649: PPC64: Intrinsics for
> Math.ceil, floor,
> > >>>>> >> rint on Power
> > >>>>> >> >
> > >>>>> >> > > I think sharedRuntime is not a good place for the C2 enum.
> > >>>>> >> > Agree.
> > >>>>> >> >
> > >>>>> >> > I suggest to put the enum in RoundDoubleModeNode
> > >>>> (convertnode.hpp)
> > >>>>> >> > and
> > >>>>> >> > lift instruction selection logic from macroAssembler_ppc.cpp
> > into
> > >>>>> ppc.ad.
> > >>>>> >> >
> > >>>>> >> > Best regards,
> > >>>>> >> > Vladimir Ivanov
> > >>>>> >> >
> > >>>>> >> > >
> > >>>>> >> > > Maybe intrinsicnode.hpp would be a better pace for it. But
> > >>>>> then, the
> > >>>>> >> > > code in macroAssembler should be guarded by #ifdef
> > >> COMPILER2.
> > >>>>> >> > >
> > >>>>> >> > > Best regards,
> > >>>>> >> > >
> > >>>>> >> > > Martin
> > >>>>> >> > >
> > >>>>> >> > > *From:*Michihiro Horie <HORIE at jp.ibm.com
> > >>>> <_mailto:HORIE at jp.ibm.com_>>
> > >>>>> >> > > *Sent:* Montag, 7. Oktober 2019 09:14
> > >>>>> >> > > *To:* Vladimir Ivanov <vladimir.x.ivanov at oracle.com
> > >>>>> <_mailto:vladimir.x.ivanov at oracle.com_>>
> > >>>>> >> > > *Cc:* hotspot compiler <hotspot-compiler-
> > >> dev at openjdk.java.net
> > >>>>> > <_mailto:hotspot-compiler-dev at openjdk.java.net_>>;
> > >>>>> >> > > ppc-aix-port-dev at openjdk.java.net
> > >>>>> > <_mailto:ppc-aix-port-dev at openjdk.java.net_>; Doerr, Martin
> > >>>>> >> > <martin.doerr at sap.com <_mailto:martin.doerr at sap.com_>>
> > >>>>> >> > > *Subject:* RE: RFR: 8231649: PPC64: Intrinsics for
> Math.ceil,
> > >>>>> floor,
> > >>>>> >> > > rint on Power
> > >>>>> >> > >
> > >>>>> >> > > Hi Vladimir,
> > >>>>> >> > >
> > >>>>> >> > > Thanks a lot for your nice suggestion. Yes, I prefer
> enum use.
> > >>>>> >> > > I'm wondering if the enum constants can be declared in
> > >>>>> >> > sharedRuntime.hpp.
> > >>>>> >> > > Webrev:
> > >> _http://cr.openjdk.java.net/~mhorie/8231649/webrev.02/_
> > >>>>> >> > >
> > >>>>> >> > > Best regards,
> > >>>>> >> > > Michihiro
> > >>>>> >> > >
> > >>>>> >> > > Inactive hide details for Vladimir Ivanov ---2019/10/04
> > >>>>> 23:05:48---Hi
> > >>>>> >> > > Michihiro,
> > >> src/hotspot/cpu/ppc/macroAssembler_ppc.cpp:Vladimir
> > >>>>> Ivanov
> > >>>>> >> > > ---2019/10/04 23:05:48---Hi Michihiro,
> > >>>>> >> > > src/hotspot/cpu/ppc/macroAssembler_ppc.cpp:
> > >>>>> >> > >
> > >>>>> >> > > From: Vladimir Ivanov <vladimir.x.ivanov at oracle.com
> > >>>>> > <_mailto:vladimir.x.ivanov at oracle.com%0b_>> > >
> > >>>>> > <_mailto:vladimir.x.ivanov at oracle.com_>>
> > >>>>> >> > > To: Michihiro Horie <HORIE at jp.ibm.com
> > >>>> <_mailto:HORIE at jp.ibm.com_>>,
> > >>>>> >> > > ppc-aix-port-dev at openjdk.java.net
> > >>>>> <_mailto:ppc-aix-port-dev at openjdk.java.net_>
> > >>>>> >> > > <_mailto:ppc-aix-port-dev at openjdk.java.net_>, hotspot
> > compiler
> > >>>>> >> > > <hotspot-compiler-dev at openjdk.java.net
> > >>>>> > <_mailto:hotspot-compiler-dev at openjdk.java.net%0b_>> > >
> > >>>>> > <_mailto:hotspot-compiler-dev at openjdk.java.net_>>
> > >>>>> >> > > Date: 2019/10/04 23:05
> > >>>>> >> > > Subject: [EXTERNAL] Re: RFR: 8231649: PPC64: Intrinsics for
> > >>>>> Math.ceil,
> > >>>>> >> > > floor, rint on Power
> > >>>>> >> > >
> > >>>>> >> > >
> > >>>>> >>
> ------------------------------------------------------------------------
> > >>>>> >> > >
> > >>>>> >> > >
> > >>>>> >> > >
> > >>>>> >> > >
> > >>>>> >> > > Hi Michihiro,
> > >>>>> >> > >
> > >>>>> >> > > src/hotspot/cpu/ppc/macroAssembler_ppc.cpp:
> > >>>>> >> > > + switch (rmode) {
> > >>>>> >> > > + case 0: // rint
> > >>>>> >> > > + frin(t, b);
> > >>>>> >> > > + break;
> > >>>>> >> > > + case 1: // floor
> > >>>>> >> > > + frim(t, b);
> > >>>>> >> > > + break;
> > >>>>> >> > > + case 2: // ceil
> > >>>>> >> > > + frip(t, b);
> > >>>>> >> > > + break;
> > >>>>> >> > > + default:
> > >>>>> >> > > + ShouldNotReachHere();
> > >>>>> >> > > + }
> > >>>>> >> > >
> > >>>>> >> > > What do you think about introducing enum constants instead
> > of
> > >>>> using
> > >>>>> >> > > hard-coded 0/1/2?
> > >>>>> >> > >
> > >>>>> >> > > src/hotspot/share/opto/library_call.cpp:
> > >>>>> >> > >
> > >>>>> >> > > case vmIntrinsics::_ceil: n = new
> > >> RoundDoubleModeNode(arg,
> > >>>>> >> > > makecon(TypeInt::make(2))); break;
> > >>>>> >> > > case vmIntrinsics::_floor: n = new
> > >> RoundDoubleModeNode(arg,
> > >>>>> >> > > makecon(TypeInt::make(1))); break;
> > >>>>> >> > > case vmIntrinsics::_rint: n = new
> > >> RoundDoubleModeNode(arg,
> > >>>>> >> > > makecon(TypeInt::make(0))); break;
> > >>>>> >> > >
> > >>>>> >> > > The downside is you have to move the switch from
> > >>>> macroAssembler
> > >>>>> >> > because
> > >>>>> >> > > enum should be visible from both places. (Is there a better
> > >>>>> place for
> > >>>>> >> > > that than roundD_regNode::emit() in AD file?)
> > >>>>> >> > >
> > >>>>> >> > > Best regards,
> > >>>>> >> > > Vladimir Ivanov
> > >>>>> >> > >
> > >>>>> >> > > On 02/10/2019 07:27, Michihiro Horie wrote:
> > >>>>> >> > >>
> > >>>>> >> > >> Dear all,
> > >>>>> >> > >>
> > >>>>> >> > >> Would you please review the following change?
> > >>>>> >> > >> Bug: _https://bugs.openjdk.java.net/browse/JDK-8231649_
> > >>>>> >> > >> Webrev:
> > >> _http://cr.openjdk.java.net/~mhorie/8231649/webrev.00_
> > >>>>> >> > >>
> > >>>>> >> > >> This change adds intrinsics for Math's ceil, floor, and
> rint for
> > >>>>> >> PPC64, on
> > >>>>> >> > >> top of 8226721: Missing intrinsics for Math.ceil,
> floor, rint.
> > >>>>> >> > >>
> > >>>>> >> > >> Best regards,
> > >>>>> >> > >> Michihiro
> > >>>>> >> > >>
> > >>>>> >> > >
> > >>>>> >> > >
> > >>>>> >> > >
> > >>>>> >>
> > >>>>> >>
> > >>>>> >>
> > >>>>> >>
> > >>>>> >
> > >>>>> >
> > >>>>> >
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
>
>
More information about the ppc-aix-port-dev
mailing list