Hi Roger,
Thanks a lot for your review. Yes, I’m so glad the change for isDigit is
beneficial commonly.
I updated webrev fixing the copyright format.
http://cr.openjdk.java.net/~mhorie/8213754/webrev.05/
Best regards,
--
Michihiro,
IBM Research - Tokyo
From: Roger Riggs <Roger.Riggs at oracle.com>
To: Michihiro Horie <HORIE at jp.ibm.com>
Cc: core-libs-dev at openjdk.java.net,
hotspot-compiler-dev at openjdk.java.net, martin.doerr at sap.com,
vladimir.kozlov at oracle.com, volker.simonis at sap.com
Date: 2018/12/08 03:08
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for
isDigit/isLowerCase/isUpperCase/isWhitespace
Hi Michihiro,
Looks fine with the updates.
Its great that the change to isDigit is beneficial on other platforms too.
A very small editorial:
There should be a comma "," after the 2018 in the copyright of:
test/micro/org/openjdk/bench/java/lang/Characters.java
Thanks, Roger
On 12/07/2018 03:13 AM, Michihiro Horie wrote:
Hi Roger,
I updated my webrev.
http://cr.openjdk.java.net/~mhorie/8213754/webrev.04/
>0x80 might be a good choice or 0xff.
I agree, thanks.
>I was wondering about the performance without the PPC specific
intrinsic but with the
>CharacterDataLatin1.java.template code for isDigit being:
> return '0' <= ch && ch <= '9';
I now understand your point, thanks for your explanation. Both on
Power9 and Skylake, the isDigit(ch) using ‘0’…’9’ explicitly in
CharacterDataLatin1.java.template without PPC specific intrinsic was
around 15% faster than the original code that does not include my
changes. I fixed my change using ‘0’…’9’ for this isDigit(ch).
Best regards,
--
Michihiro,
IBM Research - Tokyo
Inactive
hide details for Roger Riggs ---2018/12/07
01:23:39---Hi
Michihiro, On 12/06/2018 02:38 AM, Michihiro
Horie wrote:Roger Riggs ---2018/12/07 01:23:39---Hi Michihiro, On
12/06/2018 02:38 AM, Michihiro Horie wrote:
From: Roger Riggs <Roger.Riggs at oracle.com>
To: Michihiro Horie <HORIE at jp.ibm.com>
Cc: core-libs-dev at openjdk.java.net,
hotspot-compiler-dev at openjdk.java.net, martin.doerr at sap.com,
vladimir.kozlov at oracle.com, volker.simonis at sap.com
Date: 2018/12/07 01:23
Subject: Re: RFR: 8213754: PPC64: Add Intrinsics for
isDigit/isLowerCase/isUpperCase/isWhitespace
Hi Michihiro,
On 12/06/2018 02:38 AM, Michihiro Horie wrote:
Hi Roger,
Thank you for your helpful comments. I updated new
webrev, adding a parameter in the jmh test to measure
‘other’ characters and moving the file to an appropriate
location, also fixing the indents and copyright year.
http://cr.openjdk.java.net/~mhorie/8213754/webrev.03/
The choice of 256 for other is outside the range of Latin1 which is
0..255.
0x80 might be a good choice or 0xff.
>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.
Yes, there is an opportunity: intrinsic performed 25%
better than the original on Power9.
I was wondering about the performance without the PPC specific
intrinsic but with the
CharacterDataLatin1.java.template code for isDigit being:
return '0' <= ch && ch <= '9';
>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;
I missed this point, thanks. I tried jstack but could not
find additional level.
LogCompilation shows CharacterDataLatin1.isLowerCase(ch)
is compiled in c1:
<nmethod compile_id='71' compiler='c1' level='3'
entry='0x00003fff5911cd00' size='1472'
address='0x00003fff5911cb90' relocation_offset='344'
insts_offset='368' stub_offset='1136'
scopes_data_offset='1248' scopes_pcs_offset='1336'
dependencies_offset='1448' nul_chk_table_offset='1456'
oops_offset='1184' metadata_offset='1192'
method='java.lang.CharacterDataLatin1 isLowerCase (I)Z'
bytes='15' count='1024' iicount='1025' stamp='0.058'/>
Supposing some existing complex code was already taking advantage of
the full allowed inline depth.
Then adding an extra level might change the inlining behavior.
Existing methods in CharacterDataLatin1.java.template
etc. directly invoke getProperties(ch), so isLowerCase
(ch) would be more aligned with other methods if it looks
like as follows?
+ @HotSpotIntrinsicCandidate
+ boolean isLowerCase(int ch) {
+ int props = getProperties(ch);
+ return (props & $$maskType) ==
Character.LOWERCASE_LETTER;
+ }
Yes, that would alleviate my concern.
Thanks, Roger
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.Riggs at oracle.com>
To: Michihiro Horie <HORIE at jp.ibm.com>,
vladimir.kozlov at oracle.com
Cc: core-libs-dev at openjdk.java.net,
hotspot-compiler-dev at openjdk.java.net,
martin.doerr at sap.com, volker.simonis at 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/
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/
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.kozlov at oracle.com>
Cc: core-libs-dev
<core-libs-dev at openjdk.java.net>,
hotspot-compiler-dev at openjdk.java.net
, martin.doerr at sap.com, Roger
Riggs <roger.riggs at oracle.com>,
volker.simonis at 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.kozlov at oracle.com>
To: Michihiro Horie
<HORIE at jp.ibm.com>, Roger Riggs
<roger.riggs at oracle.com>
Cc: core-libs-dev
<core-libs-dev at openjdk.java.net>,
hotspot-compiler-dev at openjdk.java.net
, martin.doerr at sap.com,
volker.simonis at 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.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
> > 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"
<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
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.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
> >>
> >>
>
------------------------------------------------------------------------------------------------------------------------
> >>
> >>
> >>
> >>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20181210/da298768/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20181210/da298768/graycol-0001.gif>