RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor, rint on Power
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Tue Oct 8 10:30:48 UTC 2019
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