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

Michihiro Horie HORIE at jp.ibm.com
Tue Oct 8 06:33:55 UTC 2019 

Hi Martin, Vladimir,

> 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?
Thanks for your recommendation, I didn't know adlc's include generator.
(Now I understand why intrinsicnode.hpp was included.)

> 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.
I will kept the change as-is if Martin is ok on this point.

> 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.
Thank you for telling the details, I understand the code better.

Latest webrev is as follows:
http://cr.openjdk.java.net/~mhorie/8231649/webrev.05/

Best regards,
Michihiro



From:	Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
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"
            <ppc-aix-port-dev at openjdk.java.net>
Date:	2019/10/08 02:31
Subject:	[EXTERNAL] Re: RFR: 8231649: PPC64: Intrinsics for Math.ceil,
            floor, rint on Power




> 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:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Emhorie_8231649_webrev.04_&d=DwID-g&c=jf_iaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=fSzwlsfGWRzgRyzRwoRijgP0dNfRTTp9AtcEU_pRSh4&s=IlXiSrIMK6_IZWaQkNhJKAF-apNgFw0My38tD3o6324&e=

>
> 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.
>
>>
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Emhorie_8231649_webrev.03_&d=DwID-g&c=jf_iaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=fSzwlsfGWRzgRyzRwoRijgP0dNfRTTp9AtcEU_pRSh4&s=LdjN1SKMJL0PdT8OIqU60Xg3ULNBAM24D5I4CNOW0hw&e=

>
> 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:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Emhorie_8231649_webrev.02_&d=DwID-g&c=jf_iaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=fSzwlsfGWRzgRyzRwoRijgP0dNfRTTp9AtcEU_pRSh4&s=m5k26bqmrP_HZ8c6w0ffEa4cS8nKw12n-wMOrn7gQIQ&e=

>>  > >
>>  > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8231649&d=DwID-g&c=jf_iaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=fSzwlsfGWRzgRyzRwoRijgP0dNfRTTp9AtcEU_pRSh4&s=SlyNAQFdnb87Dn0EZNWzIxgm1-5luZZxasXkBUoZeP0&e=

>>  > >> Webrev:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Emhorie_8231649_webrev.00&d=DwID-g&c=jf_iaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=fSzwlsfGWRzgRyzRwoRijgP0dNfRTTp9AtcEU_pRSh4&s=-zp4Mak75FNz1Gel2QUNoWyK4E0EGS6YrZSVDDZhbKg&e=

>>  > >>
>>  > >> 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
>>  > >>
>>  > >
>>  > >
>>  > >
>>
>>
>>
>>
>
>
>



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20191008/cb2e0cd3/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20191008/cb2e0cd3/graycol-0001.gif>

More information about the ppc-aix-port-dev mailing list