RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor, rint on Power
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Mon Oct 7 16:20:48 UTC 2019
> New webrev: http://cr.openjdk.java.net/~mhorie/8231649/webrev.04/
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.
Otherwise, looks good.
Best regards,
Vladimir Ivanov
> 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>
> To: Michihiro Horie <HORIE at jp.ibm.com>, "Doerr, Martin"
> <martin.doerr at sap.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>
> 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>
> > 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>
> > 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>
> > > Sent: Montag, 7. Oktober 2019 12:55
> > > 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 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>
> > > > *Sent:* Montag, 7. Oktober 2019 09:14
> > > > *To:* Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> > > > *Cc:* hotspot compiler <hotspot-compiler-dev at openjdk.java.net>;
> > > > ppc-aix-port-dev at openjdk.java.net; Doerr, Martin
> > > <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>>
> > > > 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>, hotspot compiler
> > > > <hotspot-compiler-dev at openjdk.java.net
> > > > <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