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

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Tue Oct 8 09:46:54 UTC 2019 

> 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