RFR: 8231649: PPC64: Intrinsics for Math.ceil, floor, rint on Power

Doerr, Martin martin.doerr at sap.com
Wed Oct 9 09:29:53 UTC 2019


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