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/ <http://cr.openjdk.java.net/%7Emhorie/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 intrinsicwas 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.ri...@oracle.com>
To: Michihiro Horie <ho...@jp.ibm.com>
Cc: core-libs-dev@openjdk.java.net, hotspot-compiler-...@openjdk.java.net, martin.do...@sap.com, vladimir.koz...@oracle.com, volker.simo...@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/_
<http://cr.openjdk.java.net/%7Emhorie/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.ri...@oracle.com>_
        <mailto:roger.ri...@oracle.com>
        To: Michihiro Horie _<ho...@jp.ibm.com>_
        <mailto:ho...@jp.ibm.com>, _vladimir.kozlov@oracle.com_
        <mailto:vladimir.koz...@oracle.com>
        Cc: _core-libs-...@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/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
                        >  >>
                        >  >>
                        >
                        >
                        >
                        >
                        >







Reply via email to