Re: RFR: 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace

[Vladimir Kozlov]

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>
Sent by: "hotspot-compiler-dev" <hotspot-compiler-dev-boun...@openjdk.java.net>
To: "Doerr, Martin" <martin.do...@sap.com>
Cc: "Simonis, Volker" <volker.simo...@sap.com>, "hotspot-compiler-...@openjdk.java.net" 
<hotspot-compiler-...@openjdk.java.net>, "core-libs-dev@openjdk.java.net" <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 the 
direction 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 java 
code much"Doerr, Martin" ---2018/11/22 01:33:34---Hi Michihiro, thanks. This proposal makes the java code much more 
intrinsics friendly.

From: "Doerr, Martin" <martin.do...@sap.com>
To: Michihiro Horie <ho...@jp.ibm.com>, "core-libs-dev@openjdk.java.net" 
<core-libs-dev@openjdk.java.net>
Cc: "hotspot-compiler-...@openjdk.java.net" <hotspot-compiler-...@openjdk.java.net>, "Simonis, Volker" 
<volker.simo...@sap.com>, Gustavo Romero <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 the 
direction regarding the java classes is clear.

Best regards,
Martin

*
**From:*Michihiro Horie <ho...@jp.ibm.com>*
**Sent:*Mittwoch, 21. November 2018 17:14*
**To:*core-libs-dev@openjdk.java.net; Doerr, Martin <martin.do...@sap.com>*
**Cc:*hotspot-compiler-...@openjdk.java.net; Simonis, Volker <volker.simo...@sap.com>; Gustavo Romero 
<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>_/_

I followed the coding way of Character.isWhitespace() that invokes each ChracterData’s isWhitespace() to refactor 
isDigit(), 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.do...@sap.com>>
Cc: "Simonis, Volker" <_volker.simonis@sap.com_ <mailto:volker.simo...@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 any 
optimizations, 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 on 
Power9 opt"Doerr, Martin" ---2018/11/20 01:55:27---Hi Michihiro, first of all, thanks for working on Power9 
optimizations. Please note that we don't ha

From: "Doerr, Martin" <_martin.doerr@sap.com_ <mailto:martin.do...@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.simo...@sap.com>>, "Lindenmaier, Goetz" 
<_goetz.lindenmaier@sap.com_ <mailto:goetz.lindenma...@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 people 
will have to test.

I think it may be problematic to insert a slow path by “generate_method_call_static”. This may be a performance 
disadvantage 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.do...@sap.com>>; Simonis, Volker <_volker.simonis@sap.com_ 
<mailto:volker.simo...@sap.com>>; Lindenmaier, Goetz <_goetz.lindenmaier@sap.com_ <mailto:goetz.lindenma...@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>

This change includes the intrinsics of Character isDigit, isLowerCase, isUpperCase, and isWhitespace to support the 
Latin1 block using POWER9’s instructions cmprb and cmpeqb. The cmprb enables to compare a character with 1 or 2 ranged 
bytes, 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