RFR: 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace (original) (raw)

Vladimir Kozlov vladimir.kozlov at oracle.com
Thu Nov 29 18:01:26 UTC 2018

Hi Michihiro,

I still do not understand which Region node you are swapping. Where it is coming from?

Regards, Vladimir

On 11/28/18 10:31 PM, Michihiro Horie wrote:

Vladimir,Roger, Thank you so much for your responses.

Vladimir, Regarding the hotspot change,I’m new in changing around librarycall.cpp,so your comments are very helpful. I will fix my code,especially inlinecharactercompare()would be like: +bool LibraryCallKit::inlinecharactercompare(vmIntrinsics::ID id){ + RegionNode* oldrgn = control()->asRegion(); + RegionNode* newrgn = new RegionNode(1); + recordforigvn(newrgn); + + // Swap current RegionNode with new one + Node* newctrl = oldrgn->in(1); + oldrgn->delreq(1); + newrgn->addreq(newctrl); + + setcontrol(gvn.transform(newrgn)); + + // argument(0)is receiver + Node* codePoint = argument(1); + Node* n = NULL; + + switch (id){ + case vmIntrinsics::isDigitc : n = new DigitCNode(control(),codePoint);break; + case vmIntrinsics::isLowerCasec : n = new LowerCaseCNode(control(),codePoint);break; + case vmIntrinsics::isUpperCasec : n = new UpperCaseCNode(control(),codePoint);break; + case vmIntrinsics::isWhitespacec : n = new WhitespaceCNode(control(),codePoint);break; + default: + fatalunexpectediid(id); + } + + setresult(gvn.transform(n)); + return true; +} Microbenchmark showed ~40% improvements. Roger, I would wait foryour review,thanks. I understood the discussion had not been recognized from people in core-libs-dev,and checked it was not actually archived there…. Best regards, -- Michihiro, IBM Research - Tokyo Inactive hide details for Roger Riggs ---2018/11/29 11:21:26---Hi, I'll look at it on Thursday.Roger Riggs ---2018/11/29 11:21:26---Hi, I'll look at it on Thursday. From: Roger Riggs <roger.riggs at oracle.com> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>, Michihiro Horie <HORIE at jp.ibm.com>, core-libs-dev at openjdk.java.net Cc: volker.simonis at sap.com, hotspot-compiler-dev at openjdk.java.net, martin.doerr at sap.com Date: 2018/11/29 11:21 Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace ------------------------------------------------------------------------------------------------------------------------

Hi, I'll look at it on Thursday. In spite of it looking like the email was sent to core-lib-dev, I have not seen it before and its not in the core-libs-dev archive. $.02, Roger On 11/28/18 7:15 PM, Vladimir Kozlov wrote: > You still need review from core-libs. > > About your hotspot changes. You need to add intrinsics to > isdisabledbyflags() to be able switch them off. > > Note, matchrulesupported() calls in > C2Compiler::isintrinsicsupported() executed before intrinsics in C2 > are registered. So they will not be available if they are not > supported. matchrulesupported() checks in > LibraryCallKit::inlinecharactercompare() are not needed. > > I don't get what you code in > LibraryCallKit::inlinecharactercompare() is doing. Why you check > Region node at the beginning and why you add Region and Phi at the end > if you have only one path? > You are creating code for whole method which should return boolean result > > +    boolean isDigit(int ch) { > +      return getType(ch) == Character.DECIMALDIGITNUMBER; > +    } > > but your code produce TypeInt::CHAR. why? > > An finally. Did you compare code generated by default and with you > changes? what improvement you see? > > Thanks, > Vladimir > > On 11/28/18 3:07 PM, Michihiro Horie wrote: >> Is there any response from core-libs-dev? >> >> Vladimir, >> Could you give your suggestion on how to proceed? >> >> >> Best regards, >> -- >> Michihiro, >> IBM Research - Tokyo >> >> >> ----- Original message ----- >> From: "Michihiro Horie" <HORIE at jp.ibm.com> >> Sent by: "hotspot-compiler-dev" >> <hotspot-compiler-dev-bounces at openjdk.java.net> >> To: "Doerr, Martin" <martin.doerr at sap.com> >> Cc: "Simonis, Volker" <volker.simonis at sap.com>, >> "hotspot-compiler-dev at openjdk.java.net"<hotspot-compiler-dev at openjdk.java.net>, >> "core-libs-dev at openjdk.java.net" <core-libs-dev at openjdk.java.net> >> Subject: RE: 8213754: PPC64: Add Intrinsics for >> isDigit/isLowerCase/isUpperCase/isWhitespace >> Date: Thu, Nov 22, 2018 11:28 AM >> >> Hi Martin, >> >> Yes, we should wait for the feedback on class library change. >> >>  >Btw. I think you can further simplify the code in librarycall.cpp >> (no phi). But we can discuss details when thedirection regarding the >> java classes is clear. >> Thank you for pointing out it, I think I understand how to simplify >> code. >> >>  >Hi Michi, >>  > >>  >On 11/20/2018 02:52 PM, Michihiro Horie wrote: >>  >> >Please note that we don’t have a machine, yet. So other people >> will have to test. >>  >> I think Gustavo can help testing this change when its' ready. >>  >Sure :) >>  > >>  >Best regards, >>  >Gustavo >> Thank you, Gustavo. >> >> >> Best regards, >> -- >> Michihiro, >> IBM Research - Tokyo >> >> Inactive hide details for "Doerr, Martin" ---2018/11/22 01:33:34---Hi >> Michihiro, thanks. This proposal makes the javacode much"Doerr, >> Martin" ---2018/11/22 01:33:34---Hi Michihiro, thanks. This proposal >> makes the java code much moreintrinsics friendly. >> >> From: "Doerr, Martin" <martin.doerr at sap.com> >> To: Michihiro Horie <HORIE at jp.ibm.com>, >> "core-libs-dev at openjdk.java.net" <core-libs-dev at openjdk.java.net> >> Cc: "hotspot-compiler-dev at openjdk.java.net" >> <hotspot-compiler-dev at openjdk.java.net>, "Simonis, >> Volker"<volker.simonis at sap.com>, Gustavo Romero >> <gromero at linux.vnet.ibm.com> >> Date: 2018/11/22 01:33 >> Subject: RE: 8213754: PPC64: Add Intrinsics for >> isDigit/isLowerCase/isUpperCase/isWhitespace >> >> ------------------------------------------------------------------------------------------------------------------------ >> >> >> >> >> Hi Michihiro, >> >> thanks. This proposal makes the java code much more intrinsics friendly. >> We should wait for feedback from the core lib folks. Maybe they have >> some requirements or other proposals. >> >> I think these intrinsics should be interesting for Oracle, Intel and >> others, too. >> >> Btw. I think you can further simplify the code in librarycall.cpp >> (no phi). But we can discuss details when thedirection regarding the >> java classes is clear. >> >> Best regards, >> Martin >> >> * >> **From:Michihiro Horie <HORIE at jp.ibm.com> >> **Sent:Mittwoch, 21. November 2018 17:14 >> *To:core-libs-dev at openjdk.java.net; Doerr, Martin >> <martin.doerr at sap.com>* >> *Cc:hotspot-compiler-dev at openjdk.java.net; Simonis, Volker >> <volker.simonis at sap.com>; Gustavo Romero<gromero at linux.vnet.ibm.com>* >> **Subject:*RE: 8213754: PPC64: Add Intrinsics for >> isDigit/isLowerCase/isUpperCase/isWhitespace >> >> Hi Martin, >> >> I send this RFR to core-libs-dev too, because it changes the class >> library. >> >> Through trial and error, I separated Latin1 block as in the >> _https://urldefense.proofpoint.com/v2/url?u=http-3A_cr.openjdk.java.net-7Emhorie8213754webrev.01-5F&d=DwIDaQ&c=jfiaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=K5GMqTdhkNMaMnGAqGucn9CsZllScUGvHO427jiTC5E&s=brFCGq8BK0Q6ICnXLNCUF0nI8J5LjSjhtiWAZlhjPb4&e= >> <http://cr.openjdk.java.net/%7Emhorie/8213754/webrev.01>/ >> >> I followed the coding way of Character.isWhitespace() that invokes >> each ChracterData’s isWhitespace() to refactorisDigit(), >> isLowerCase(), and isUpperCase(). >> >> I think this change is also useful on x86, using STTNI. >> >> >> Best regards, >> -- >> Michihiro, >> IBM Research - Tokyo >> >> >> ----- Original message ----- >> From: "Michihiro Horie" <HORIE at jp.ibm.com <mailto:HORIE at jp.ibm.com>> >> Sent by: "hotspot-compiler-dev" >> <hotspot-compiler-dev-bounces at openjdk.java.net<mailto:hotspot-compiler-dev-bounces at openjdk.java.net>> >> To: "Doerr, Martin" <martin.doerr at sap.com_ _>> <mailto:martin.doerr at sap.com>> >> Cc: "Simonis, Volker" <volker.simonis at sap.com_ _>> <mailto:volker.simonis at sap.com>>, >> "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>>, >> "hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>" >> <hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>> >> >> Subject: RE: 8213754: PPC64: Add Intrinsics for >> isDigit/isLowerCase/isUpperCase/isWhitespace >> Date: Wed, Nov 21, 2018 1:53 AM >> >> Hi Martin, >> >> Thank you for giving your helpful comments. I did not recognize >> “generatemethodcallstatic” prevents anyoptimizations, but I now >> checked it actually degraded the performance, thanks. >> >>  >Please note that we don’t have a machine, yet. So other people will >> have to test. >> I think Gustavo can help testing this change when its' ready. >> >>  >Would it be possible to introduce more fine-grained intrinsics such >> that the “slow” path is outside of them? >>  > >>  >Maybe you can factor out as in the following example? >>  >if (latin1) return isLatin1Digit(codePoint); >>  >with isLatin1Digit as HotSpotIntrinsicCandidate. >> Thanks for an example, please let me try to separate the Latin block >> from other blocks for some time. >> >> >> Best regards, >> -- >> Michihiro, >> IBM Research - Tokyo >> >> Inactive hide details for "Doerr, Martin" ---2018/11/20 01:55:27---Hi >> Michihiro, first of all, thanks for working onPower9 opt"Doerr, >> Martin" ---2018/11/20 01:55:27---Hi Michihiro, first of all, thanks >> for working on Power9optimizations. Please note that we don't ha >> >> From: "Doerr, Martin" <martin.doerr at sap.com_ _>> <mailto:martin.doerr at sap.com>> >> To: Michihiro Horie <HORIE at jp.ibm.com <mailto:HORIE at jp.ibm.com>>, >> "hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>" >> <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>> >> Cc: "Simonis, Volker" <volker.simonis at sap.com_ _>> <mailto:volker.simonis at sap.com>>, "Lindenmaier, >> Goetz"<goetz.lindenmaier at sap.com_ _>> <mailto:goetz.lindenmaier at sap.com>>, Gustavo Romero >> <gromero at linux.vnet.ibm.com<mailto:gromero at linux.vnet.ibm.com>> >> Date: 2018/11/20 01:55 >> Subject: RE: 8213754: PPC64: Add Intrinsics for >> isDigit/isLowerCase/isUpperCase/isWhitespace >> >> ------------------------------------------------------------------------------------------------------------------------ >> >> >> >> >> >> Hi Michihiro, >> >> first of all, thanks for working on Power9 optimizations. Please note >> that we don’t have a machine, yet. So other peoplewill have to test. >> >> I think it may be problematic to insert a slow path by >> “generatemethodcallstatic”. This may be a performancedisadvantage >> for some users of other encodings because your intrinsics prevent >> inlining and further optimizations. >> Would it be possible to introduce more fine-grained intrinsics such >> that the “slow” path is outside of them? >> >> Maybe you can factor out as in the following example? >> if (latin1) return isLatin1Digit(codePoint); >> with isLatin1Digit as HotSpotIntrinsicCandidate. >> >> I can’t judge if this is needed, but I think this should be discussed >> first before going into the details. >> >> Best regards, >> Martin >> >> * >> **From:Michihiro Horie <HORIE at jp.ibm.com <mailto:HORIE at jp.ibm.com>> >> **Sent:Freitag, 16. November 2018 12:53 >> *To: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>* >> **Cc:*Doerr, Martin <martin.doerr at sap.com_ _>> <mailto:martin.doerr at sap.com>>; Simonis, Volker >> <volker.simonis at sap.com<mailto:volker.simonis at sap.com>>; >> Lindenmaier, Goetz <goetz.lindenmaier at sap.com_ _>> <mailto:goetz.lindenmaier at sap.com>>;Gustavo Romero >> <gromero at linux.vnet.ibm.com <mailto:gromero at linux.vnet.ibm.com>>* >> **Subject:*RFR: 8213754: PPC64: Add Intrinsics for >> isDigit/isLowerCase/isUpperCase/isWhitespace >> >> Dear all, >> >> Would you please review following change? >> >> Bug: https://urldefense.proofpoint.com/v2/url?u=https-3A_bugs.openjdk.java.netbrowseJDK-2D8213754-5F&d=DwIDaQ&c=jfiaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=K5GMqTdhkNMaMnGAqGucn9CsZllScUGvHO427jiTC5E&s=GdBakBGPQKO3fiM6o8w1OpCp76EtuynzAOZaE-OoQQ&e= >> Webrev: https://urldefense.proofpoint.com/v2/url?u=http-3A_cr.openjdk.java.net-7Emhorie8213754webrev.00-5F&d=DwIDaQ&c=jfiaSHvJObTbx-siA1ZOg&r=oecsIpYF-cifqq2i1JEH0Q&m=K5GMqTdhkNMaMnGAqGucn9CsZllScUGvHO427jiTC5E&s=RqwTJXnDC8QoRxIsFshdedeLrlfJU110wDJcjzjw2c&e= >> <http://cr.openjdk.java.net/%7Emhorie/8213754/webrev.00> >> >> This change includes the intrinsics of Character isDigit, >> isLowerCase, isUpperCase, and isWhitespace to support theLatin1 block >> using POWER9’s instructions cmprb and cmpeqb. The cmprb enables to >> compare a character with 1 or 2 rangedbytes, while the cmpeqb >> compares one with 1 to 8 values. Simple micro benchmark attached >> showed improvements by 20-40%. >> / >> //(See attached file: Latin1Test.java)/ >> >> >> Best regards, >> -- >> Michihiro, >> IBM Research - Tokyo >> >>

More information about the core-libs-dev mailing list