RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor, rint on Power
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Sat Oct 12 01:26:36 UTC 2019
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