RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor, rint on Power
Doerr, Martin
martin.doerr at sap.com
Tue Oct 8 10:14:15 UTC 2019
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?
Best regards,
Martin
> -----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 hotspot-compiler-dev
mailing list