Best regards,
--
Michihiro,
IBM Research - Tokyo
Inactive hide details for Roger Riggs ---2018/12/06 05:09:36---Hi
Michihiro, On 12/05/2018 07:21 AM, Michihiro Horie wrote:Roger Riggs
---2018/12/06 05:09:36---Hi Michihiro, On 12/05/2018 07:21 AM,
Michihiro Horie wrote:
From: Roger Riggs <roger.ri...@oracle.com>
To: Michihiro Horie <ho...@jp.ibm.com>, vladimir.koz...@oracle.com
Cc: core-libs-dev@openjdk.java.net,
hotspot-compiler-...@openjdk.java.net, martin.do...@sap.com,
volker.simo...@sap.com
Date: 2018/12/06 05:09
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for
isDigit/isLowerCase/isUpperCase/isWhitespace
------------------------------------------------------------------------
Hi Michihiro,
On 12/05/2018 07:21 AM, Michihiro Horie wrote:
>There are a few JMH tests for upper and lower in the
jmh-jdk-microbenchmarks repo. [1]
Here is a jmh webrev for the Character methods._
__http://cr.openjdk.java.net/~mhorie/8213754/jmh-webrev.00/_
<http://cr.openjdk.java.net/%7Emhorie/8213754/jmh-webrev.00/>
Please include at least one character value that is not a member of
any of the classes.
The performance impact for 'other' characters is important also.
The JMH tests have been moved to the OpenJDK jdk/jdk repo in the
directory:
test/micro/org/openjdk/bench/java/lang/
Now that they are in that repo, they can be included with the rest of
the changeset.
Also, I updated C2 changes in the latest webrev. (Thank you
for giving valuable comments off-list, Vladimir and Martin!)
With the change in is_disabled_by_flags, I added a JVM flag
that will need another review request. I would proceed for it
if this RFR is accepted._
__http://cr.openjdk.java.net/~mhorie/8213754/webrev.02/_
<http://cr.openjdk.java.net/%7Emhorie/8213754/webrev.02/>
The indent in the Java code should be 4 spaces. (Native lib code is 4
spaces, Hotspot is 2 spaces)
Please update the copyrights to include 2018.
Is there an opportunity to improve the performance of isDigit using
explicit '0'.. '9'
in CharacterDataLatin1.java.template? The performance would need to
be measured and compared.
Is the profile of in-lining methods changed in any measurable way now that
there is an extra level of method invocation?
Character.isLowerCase(ch) -> CharacterData.isLowerCase(ch) ->
getType(ch) == LOWERCASE_LETTER;
instead of:
Character.isLowerCase(ch) -> getType(ch) == LOWERCASE_LETTER;
Regards, Roger
I need a review on changes in the class library, anyway. Would
be grateful if I could have one.
Best regards,
--
Michihiro,
IBM Research - Tokyo
----- Original message -----
From: Michihiro Horie/Japan/IBM
To: Vladimir Kozlov _<vladimir.koz...@oracle.com>_
<mailto:vladimir.koz...@oracle.com>
Cc: core-libs-dev _<core-libs-dev@openjdk.java.net>_
<mailto:core-libs-dev@openjdk.java.net>,
_hotspot-compiler-...@openjdk.java.net_
<mailto:hotspot-compiler-...@openjdk.java.net>,
_martin.doerr@sap.com_ <mailto:martin.do...@sap.com>, Roger
Riggs _<roger.ri...@oracle.com>_
<mailto:roger.ri...@oracle.com>, _volker.simonis@sap.com_
<mailto:volker.simo...@sap.com>
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for
isDigit/isLowerCase/isUpperCase/isWhitespace
Date: Fri, Nov 30, 2018 1:01 PM
Hi Vladimir,
Thank you for your further comments. I fixed my code, but I’m
afraid discussing such a basic topic involving the two big
mailing lists is not good. Please let me have a chance to talk
on this off-list.
Best regards,
--
Michihiro,
IBM Research - Tokyo
Vladimir Kozlov ---2018/11/30 03:01:42---Hi Michihiro, I still
do not understand which Region node you are swapping. Where it
is coming from?
From: Vladimir Kozlov _<vladimir.koz...@oracle.com>_
<mailto:vladimir.koz...@oracle.com>
To: Michihiro Horie _<ho...@jp.ibm.com>_
<mailto:ho...@jp.ibm.com>, Roger Riggs
_<roger.ri...@oracle.com>_ <mailto:roger.ri...@oracle.com>
Cc: core-libs-dev _<core-libs-dev@openjdk.java.net>_
<mailto:core-libs-dev@openjdk.java.net>,
_hotspot-compiler-...@openjdk.java.net_
<mailto:hotspot-compiler-...@openjdk.java.net>,
_martin.doerr@sap.com_ <mailto:martin.do...@sap.com>,
_volker.simonis@sap.com_ <mailto:volker.simo...@sap.com>
Date: 2018/11/30 03:01
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for
isDigit/isLowerCase/isUpperCase/isWhitespace
------------------------------------------------------------------------
Hi Michihiro,
I still do not understand which Region node you are swapping.
Where it is coming from?
> + // Swap current RegionNode with new one
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
library_call.cpp,so your comments are very helpful. I will fix
> my code,especially inline_character_compare()would be like:
>
> +bool
LibraryCallKit::inline_character_compare(vmIntrinsics::ID id){
> + RegionNode* old_rgn = control()->as_Region();
> + RegionNode* new_rgn = new RegionNode(1);
> + record_for_igvn(new_rgn);
> +
> + // Swap current RegionNode with new one
> + Node* new_ctrl = old_rgn->in(1);
> + old_rgn->del_req(1);
> + new_rgn->add_req(new_ctrl);
> +
> + set_control(_gvn.transform(new_rgn));
> +
> + // argument(0)is receiver
> + Node* codePoint = argument(1);
> + Node* n = NULL;
> +
> + switch (id){
> + case vmIntrinsics::_isDigit_c : n = new
DigitCNode(control(),codePoint);break;
> + case vmIntrinsics::_isLowerCase_c : n = new
LowerCaseCNode(control(),codePoint);break;
> + case vmIntrinsics::_isUpperCase_c : n = new
UpperCaseCNode(control(),codePoint);break;
> + case vmIntrinsics::_isWhitespace_c : n = new
WhitespaceCNode(control(),codePoint);break;
> + default:
> + fatal_unexpected_iid(id);
> + }
> +
> + set_result(_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.ri...@oracle.com>_
<mailto:roger.ri...@oracle.com>
> To: Vladimir Kozlov _<vladimir.koz...@oracle.com>_
<mailto:vladimir.koz...@oracle.com>, Michihiro Horie
_<ho...@jp.ibm.com>_ <mailto:ho...@jp.ibm.com>,
_core-libs-...@openjdk.java.net_
<mailto:core-libs-dev@openjdk.java.net>
> Cc: _volker.simonis@sap.com_
<mailto:volker.simo...@sap.com>,
_hotspot-compiler-...@openjdk.java.net_
<mailto:hotspot-compiler-...@openjdk.java.net>,
_martin.doerr@sap.com_ <mailto:martin.do...@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
> > is_disabled_by_flags() to be able switch them off.
> >
> > Note, match_rule_supported() calls in
> > C2Compiler::is_intrinsic_supported() executed before
intrinsics in C2
> > are registered. So they will not be available if they are not
> > supported. match_rule_supported() checks in
> > LibraryCallKit::inline_character_compare() are not needed.
> >
> > I don't get what you code in
> > LibraryCallKit::inline_character_compare() 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.DECIMAL_DIGIT_NUMBER;
> > + }
> >
> > 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" _<ho...@jp.ibm.com>_
<mailto:ho...@jp.ibm.com>
> >> Sent by: "hotspot-compiler-dev"
> >> _<hotspot-compiler-dev-boun...@openjdk.java.net>_
<mailto:hotspot-compiler-dev-boun...@openjdk.java.net>
> >> To: "Doerr, Martin" _<martin.do...@sap.com>_
<mailto:martin.do...@sap.com>
> >> Cc: "Simonis, Volker" _<volker.simo...@sap.com>_
<mailto:volker.simo...@sap.com>,
> >> _"hotspot-compiler-...@openjdk.java.net"_
<mailto:hotspot-compiler-...@openjdk.java.net>_<hotspot-compiler-...@openjdk.java.net>_
<mailto:hotspot-compiler-...@openjdk.java.net>,
> >> _"core-libs-dev@openjdk.java.net"_
<mailto:core-libs-dev@openjdk.java.net>_<core-libs-dev@openjdk.java.net>_
<mailto:core-libs-dev@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
library_call.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.do...@sap.com>_
<mailto:martin.do...@sap.com>
> >> To: Michihiro Horie _<ho...@jp.ibm.com>_
<mailto:ho...@jp.ibm.com>,
> >> _"core-libs-dev@openjdk.java.net"_
<mailto:core-libs-dev@openjdk.java.net>_<core-libs-dev@openjdk.java.net>_
<mailto:core-libs-dev@openjdk.java.net>
> >> Cc: _"hotspot-compiler-...@openjdk.java.net"_
<mailto:hotspot-compiler-...@openjdk.java.net>
> >> _<hotspot-compiler-...@openjdk.java.net>_
<mailto:hotspot-compiler-...@openjdk.java.net>, "Simonis,
> >> Volker"_<volker.simo...@sap.com>_
<mailto:volker.simo...@sap.com>, Gustavo Romero
> >> _<grom...@linux.vnet.ibm.com>_
<mailto:grom...@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
library_call.cpp
> >> (no phi). But we can discuss details when thedirection
regarding the
> >> java classes is clear.
> >>
> >> Best regards,
> >> Martin
> >>
> >> *
> >> **From:*Michihiro Horie _<ho...@jp.ibm.com>_
<mailto:ho...@jp.ibm.com>*
> >> **Sent:*Mittwoch, 21. November 2018 17:14*
> >> **To:*core-libs-dev@openjdk.java.net; Doerr, Martin
> >> _<martin.do...@sap.com>_ <mailto:martin.do...@sap.com>*
> >> **Cc:*hotspot-compiler-...@openjdk.java.net; Simonis, Volker
> >> _<volker.simo...@sap.com>_
<mailto:volker.simo...@sap.com>; Gustavo
Romero_<grom...@linux.vnet.ibm.com>_
<mailto:grom...@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 _
> >>
> ___http://cr.openjdk.java.net/~mhorie/8213754/webrev.01__
<http://cr.openjdk.java.net/%7Emhorie/8213754/webrev.01_>
>
> >>
<_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" <_ho...@jp.ibm.com_
<_mailto:ho...@jp.ibm.com_>>
> >> Sent by: "hotspot-compiler-dev"
> >>
<_hotspot-compiler-dev-boun...@openjdk.java.net_<_mailto:hotspot-compiler-dev-boun...@openjdk.java.net_>>
> >> To: "Doerr, Martin" <_martin.doerr@sap.com_
> >> <_mailto:martin.doerr@sap.com_>>
> >> Cc: "Simonis, Volker" <_volker.simonis@sap.com_
> >> <_mailto:volker.simonis@sap.com_>>,
> >>
"_ppc-aix-port-...@openjdk.java.net_<_mailto:ppc-aix-port-...@openjdk.java.net_>"
> >>
<_ppc-aix-port-...@openjdk.java.net_<_mailto:ppc-aix-port-...@openjdk.java.net_>>,
> >>
"_hotspot-compiler-...@openjdk.java.net_<_mailto:hotspot-compiler-...@openjdk.java.net_>"
> >>
<_hotspot-compiler-...@openjdk.java.net_<_mailto:hotspot-compiler-...@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
> >> “generate_method_call_static” 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@sap.com_
> >> <_mailto:martin.doerr@sap.com_>>
> >> To: Michihiro Horie <_ho...@jp.ibm.com_
<_mailto:ho...@jp.ibm.com_>>,
> >>
"_hotspot-compiler-...@openjdk.java.net_<_mailto:hotspot-compiler-...@openjdk.java.net_>"
> >>
<_hotspot-compiler-...@openjdk.java.net_<_mailto:hotspot-compiler-...@openjdk.java.net_>>,
> >>
"_ppc-aix-port-...@openjdk.java.net_<_mailto:ppc-aix-port-...@openjdk.java.net_>"
> >> <_ppc-aix-port-...@openjdk.java.net_
> >> <_mailto:ppc-aix-port-...@openjdk.java.net_>>
> >> Cc: "Simonis, Volker" <_volker.simonis@sap.com_
> >> <_mailto:volker.simonis@sap.com_>>, "Lindenmaier,
> >> Goetz"<_goetz.lindenmaier@sap.com_
> >> <_mailto:goetz.lindenmaier@sap.com_>>, Gustavo Romero
> >>
<_grom...@linux.vnet.ibm.com_<_mailto:grom...@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
> >> “generate_method_call_static”. 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 <_ho...@jp.ibm.com_
<_mailto:ho...@jp.ibm.com_>>*
> >> **Sent:*Freitag, 16. November 2018 12:53*
> >> **To:*_hotspot-compiler-...@openjdk.java.net_
> >>
<_mailto:hotspot-compiler-...@openjdk.java.net_>;_ppc-aix-port-...@openjdk.java.net_
> >> <_mailto:ppc-aix-port-...@openjdk.java.net_>*
> >> **Cc:*Doerr, Martin <_martin.doerr@sap.com_
> >> <_mailto:martin.doerr@sap.com_>>; Simonis, Volker
> >> <_volker.simonis@sap.com_<_mailto:volker.simonis@sap.com_>>;
> >> Lindenmaier, Goetz <_goetz.lindenmaier@sap.com_
> >> <_mailto:goetz.lindenmaier@sap.com_>>;Gustavo Romero
> >> <_grom...@linux.vnet.ibm.com_
<_mailto:grom...@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://bugs.openjdk.java.net/browse/JDK-8213754__
> >> Webrev:
> __http://cr.openjdk.java.net/~mhorie/8213754/webrev.00__
<http://cr.openjdk.java.net/%7Emhorie/8213754/webrev.00_>
>
> >> <_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
> >>
> >>
>
>
>
>
>
