Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Eric Liu-2
> This patch transforms '(x >>> rshift) + (x << lshift)' into
> 'RotateRight(x, rshift)' during GVN phase when both the shift exponents
> are constants and their sum equals to the number of bits for the type
> of shift base.
>
> This patch implements some new match rules for AArch64 instructions
> which can take ROR as the optional shift. Such instructions are 'and',
> 'or', 'eor', 'eon', 'bic' and 'orn'.
>
>   ror     w11, w2, #5
>   eor     w0, w1, w11
>
> With this patch, above code could be optimized to below:
>
>   eor     w0, w1, w2, ror #5
>
> Finally, the patch refactors TestRotate.java[1][2].
>
> Tested jtreg TestRotate.java, hotspot::hotspot_all_no_apps,
> jdk::jdk_core, langtools::tier1.
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8252776
> [2] https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-September/039911.html

Eric Liu has updated the pull request incrementally with one additional commit since the last revision:

  Add benchmark test
 
  Change-Id: I63ca51d06070a07e5c20daf4b42d2c8d7237a1da

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1858/files
  - new: https://git.openjdk.java.net/jdk/pull/1858/files/afc68c27..492f4ca4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1858&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1858&range=01-02

  Stats: 110 lines in 3 files changed: 108 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1858.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1858/head:pull/1858

PR: https://git.openjdk.java.net/jdk/pull/1858
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Andrew Haley-2
On Mon, 1 Feb 2021 11:20:20 GMT, Eric Liu <[hidden email]> wrote:

>> This patch transforms '(x >>> rshift) + (x << lshift)' into
>> 'RotateRight(x, rshift)' during GVN phase when both the shift exponents
>> are constants and their sum equals to the number of bits for the type
>> of shift base.
>>
>> This patch implements some new match rules for AArch64 instructions
>> which can take ROR as the optional shift. Such instructions are 'and',
>> 'or', 'eor', 'eon', 'bic' and 'orn'.
>>
>>   ror     w11, w2, #5
>>   eor     w0, w1, w11
>>
>> With this patch, above code could be optimized to below:
>>
>>   eor     w0, w1, w2, ror #5
>>
>> Finally, the patch refactors TestRotate.java[1][2].
>>
>> Tested jtreg TestRotate.java, hotspot::hotspot_all_no_apps,
>> jdk::jdk_core, langtools::tier1.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8252776
>> [2] https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-September/039911.html
>
> Eric Liu has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add benchmark test
>  
>   Change-Id: I63ca51d06070a07e5c20daf4b42d2c8d7237a1da

OK. For what it's worth, I doubt that this will be suitable for backporting to 8u or 11u.

-------------

Marked as reviewed by aph (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1858
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Eric Liu-2
On Wed, 17 Feb 2021 16:13:41 GMT, Andrew Haley <[hidden email]> wrote:

> OK. For what it's worth, I doubt that this will be suitable for backporting to 8u or 11u.

Thanks for your review, I'd like to backport this.

BTW, Is there anyone could review those trivial shared code?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1858
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Eric Liu-2
On Thu, 18 Feb 2021 02:00:04 GMT, Eric Liu <[hidden email]> wrote:

>> OK. For what it's worth, I doubt that this will be suitable for backporting to 8u or 11u.
>
>> OK. For what it's worth, I doubt that this will be suitable for backporting to 8u or 11u.
>
> Thanks for your review, I'd like to backport this.
>
> BTW, Is there anyone could review those trivial shared code?

@TobiHartmann Could you help to review those shared code?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1858
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Roland Westrelin-4
In reply to this post by Eric Liu-2
On Mon, 1 Feb 2021 11:20:20 GMT, Eric Liu <[hidden email]> wrote:

>> This patch transforms '(x >>> rshift) + (x << lshift)' into
>> 'RotateRight(x, rshift)' during GVN phase when both the shift exponents
>> are constants and their sum equals to the number of bits for the type
>> of shift base.
>>
>> This patch implements some new match rules for AArch64 instructions
>> which can take ROR as the optional shift. Such instructions are 'and',
>> 'or', 'eor', 'eon', 'bic' and 'orn'.
>>
>>   ror     w11, w2, #5
>>   eor     w0, w1, w11
>>
>> With this patch, above code could be optimized to below:
>>
>>   eor     w0, w1, w2, ror #5
>>
>> Finally, the patch refactors TestRotate.java[1][2].
>>
>> Tested jtreg TestRotate.java, hotspot::hotspot_all_no_apps,
>> jdk::jdk_core, langtools::tier1.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8252776
>> [2] https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-September/039911.html
>
> Eric Liu has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add benchmark test
>  
>   Change-Id: I63ca51d06070a07e5c20daf4b42d2c8d7237a1da

Changes requested by roland (Reviewer).

src/hotspot/share/opto/addnode.cpp line 349:

> 347:     }
> 348:   }
> 349:

Even though existing code in that method seems to assume node's inputs can't be NULL, it's a good practice to protect against unexpected NULLs as that can happen when sub-graphs die during IGVN. So in(1)->in(1), in(1)->in(2), in(2)->in(2) need to be tested for NULL.

That logic and the one for AddLNode are almost identical. So it would be good to have it in a shared method. I've been adding helper methods to make that possible but not all of that code is in yet. Could you file a bug to revisit this issue later and assign it to me?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1858
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Eric Liu-2
On Mon, 22 Feb 2021 09:05:53 GMT, Roland Westrelin <[hidden email]> wrote:

>> Eric Liu has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Add benchmark test
>>  
>>   Change-Id: I63ca51d06070a07e5c20daf4b42d2c8d7237a1da
>
> src/hotspot/share/opto/addnode.cpp line 349:
>
>> 347:     }
>> 348:   }
>> 349:
>
> Even though existing code in that method seems to assume node's inputs can't be NULL, it's a good practice to protect against unexpected NULLs as that can happen when sub-graphs die during IGVN. So in(1)->in(1), in(1)->in(2), in(2)->in(2) need to be tested for NULL.
>
> That logic and the one for AddLNode are almost identical. So it would be good to have it in a shared method. I've been adding helper methods to make that possible but not all of that code is in yet. Could you file a bug to revisit this issue later and assign it to me?

Hi Roland,

Thanks for your feedback.

> Even though existing code in that method seems to assume node's inputs can't be NULL, it's a good practice to protect against unexpected NULLs as that can happen when sub-graphs die during IGVN. So in(1)->in(1), in(1)->in(2), in(2)->in(2) need to be tested for NULL.

Agree

> That logic and the one for AddLNode are almost identical. So it would be good to have it in a shared method. I've been adding helper methods to make that possible but not all of that code is in yet.

As this match rule is trivial enough, how about withdrawing these shared code in this PR for integrating backend first if your helper methods coming soon?  

> Could you file a bug to revisit this issue later and assign it to me?

Okay, it's on my queue now:P


-- Eric

-------------

PR: https://git.openjdk.java.net/jdk/pull/1858
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Roland Westrelin-4
On Mon, 22 Feb 2021 10:30:36 GMT, Eric Liu <[hidden email]> wrote:

> As this match rule is trivial enough, how about withdrawing these shared code in this PR for integrating backend first if your helper methods coming soon?

https://github.com/openjdk/jdk/pull/2045 is the one I'm referring to. No idea when it's going to get in so I would suggest to move forward with the current change and revisit it later.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1858
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8256438: AArch64: Implement match rules with ROR shift register value [v3]

Eric Liu-2
On Tue, 23 Feb 2021 08:03:20 GMT, Roland Westrelin <[hidden email]> wrote:

>> Hi Roland,
>>
>> Thanks for your feedback.
>>
>>> Even though existing code in that method seems to assume node's inputs can't be NULL, it's a good practice to protect against unexpected NULLs as that can happen when sub-graphs die during IGVN. So in(1)->in(1), in(1)->in(2), in(2)->in(2) need to be tested for NULL.
>>
>> Agree
>>
>>> That logic and the one for AddLNode are almost identical. So it would be good to have it in a shared method. I've been adding helper methods to make that possible but not all of that code is in yet.
>>
>> As this match rule is trivial enough, how about withdrawing these shared code in this PR for integrating backend first if your helper methods coming soon?  
>>
>>> Could you file a bug to revisit this issue later and assign it to me?
>>
>> Okay, it's on my queue now:P
>>
>>
>> -- Eric
>
>> As this match rule is trivial enough, how about withdrawing these shared code in this PR for integrating backend first if your helper methods coming soon?
>
> https://github.com/openjdk/jdk/pull/2045 is the one I'm referring to. No idea when it's going to get in so I would suggest to move forward with the current change and revisit it later.

Got it. The patch has been updated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1858