8274179: AArch64: Support SVE operations with encodable immediates #6115

fg1417 · 2021-10-26T01:58:40Z

for(int i = 0; i < LENGTH; i++) {
c[i] = a[i] + 2;
}

For the case showed above, after superword optimization with SVE,
without the patch, the vector add operation always has 2 z-reg inputs,
like:
mov z16.s, #2
add z17.s, z17.s, z16.s

Considering sve has supported basic binary operations with immediate,
this pattern could be further optimized to:
add z16.s, z16.s, #2

To implement it, we added some new match rules and assembler rules in
the aarch64 backend. We also made some extensions on immediate types
and functions to keep backward compatible.

With the patch, only these binary integer vector operations, +(add),
-(sub), &(and), |(orr), and ^(eor) with immediate are supported for
the optimization. Other vector operations are not supported currently.

Tested tier1 and test/hotspot/jtreg/compiler on SVE featured AArch64
CPU, no new failure.

There is no obvious performance uplift but it can help remove one
redundant mov instruction.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8274179: AArch64: Support SVE operations with encodable immediates

Reviewers

Andrew Haley (@theRealAph - Reviewer)
Nick Gasson (@nick-arm - Reviewer) ⚠️ Review applies to 05c712d

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6115/head:pull/6115
$ git checkout pull/6115

Update a local copy of the PR:
$ git checkout pull/6115
$ git pull https://git.openjdk.java.net/jdk pull/6115/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6115

View PR using the GUI difftool:
$ git pr show -t 6115

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6115.diff

for(int i = 0; i < LENGTH; i++) { c[i] = a[i] + 2; } For the case showed above, after superword optimization with SVE, without the patch, the vector add operation always has 2 z-reg inputs, like: mov z16.s, openjdk#2 add z17.s, z17.s, z16.s Considering sve has supported basic binary operations with immediate, this pattern could be further optimized to: add z16.s, z16.s, openjdk#2 To implement it, we added some new match rules and assembler rules in the aarch64 backend. We also made some extensions on immediate types and functions to keep backward compatible. With the patch, only these binary integer vector operations, +(add), -(sub), &(and), |(orr), and ^(eor) with immediate are supported for the optimization. Other vector operations are not supported currently. Tested tier1 and test/hotspot/jtreg/compiler on SVE featured AArch64 CPU, no new failure. There is no obvious performance uplift but it can help remove one redundant mov instruction. Change-Id: Iaec40e362918118691083fb171cc4dff390b35a2

bridgekeeper · 2021-10-26T01:59:32Z

👋 Welcome back fg1417! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-10-26T02:03:09Z

@fg1417 The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-10-26T02:06:31Z

Webrevs

dholmes-ora · 2021-10-26T04:29:09Z

@fg1417 I'm running this through our internal testing as we have had problems with other recent SVE changes.

Thanks,
David

fg1417 · 2021-10-26T05:48:36Z

@fg1417 I'm running this through our internal testing as we have had problems with other recent SVE changes.

Thanks, David

Thanks, @dholmes-ora . If you have any problems, please contact me.

dholmes-ora · 2021-10-26T07:00:57Z

Test run was successful - tiers 1-3.

fg1417 · 2021-10-26T08:00:54Z

Test run was successful - tiers 1-3.

Thanks for your effort :)

theRealAph · 2021-10-26T09:38:38Z

src/hotspot/cpu/aarch64/aarch64.ad

+  Node* imm_node = replicate_node->in(1);
+  if (!imm_node->is_Con() ||
+      !(imm_node->bottom_type()->isa_int() || imm_node->bottom_type()->isa_long())) {
+    return false;


I'd split this up a bit.

Please consider

if (!imm_node->is_Con() return false; const Type t = imm_node->bottom_type(); if (! (t->isa_int() || t->isa_long)) return false;

theRealAph · 2021-10-26T11:37:23Z

I'd like you to split this patch into two parts, please.
First, please use the new functions such as Assembler::operand_valid_for_logical_immediate(bool is32, uint64_t imm) only for SVE, leaving the existing logic in Assembler entirely untouched. This will cause some duplication, but that's OK. We can review changes to merge functionality in a separate patch. This will be much easier.

Change-Id: I52aa66d200b74ac312c5d40283b94854bc1142e6

…tirely untouched Change-Id: If8ddcef07b15615d7dd0c3063c44d2b705fac6f7

fg1417 · 2021-10-28T10:02:48Z

I'd like you to split this patch into two parts, please. First, please use the new functions such as Assembler::operand_valid_for_logical_immediate(bool is32, uint64_t imm) only for SVE, leaving the existing logic in Assembler entirely untouched. This will cause some duplication, but that's OK. We can review changes to merge functionality in a separate patch. This will be much easier.

Thanks. Done.

theRealAph · 2021-10-28T11:46:58Z

src/hotspot/cpu/aarch64/assembler_aarch64.cpp

 }

+unsigned Assembler::regVariant_to_elemBits(Assembler::SIMD_RegVariant T){
+  return 1 << (T + 3);


Assert something about T here.

theRealAph · 2021-10-28T11:49:44Z

src/hotspot/cpu/aarch64/assembler_aarch64.cpp

+  uint64_t uimm = (uint64_t)uabs((jlong)imm);
+  if (uimm < (UCONST64(1) << nbits))
+    return true;
+  if (uimm < (UCONST64(1) << (2 * nbits))


Assert something about nbits here. It has to be less than 32, I think.

theRealAph

Looks fine with those minor changes.

Change-Id: Ic9120902bd8f8a8ead2e3740435a40f35d21757c

openjdk · 2021-11-01T04:11:07Z

@fg1417 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8274179: AArch64: Support SVE operations with encodable immediates

Reviewed-by: aph, ngasson

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the master branch:

e9934e1: 8277221: G1: Remove methods without implementations in G1CollectedHeap
9aa30de: 8275317: AArch64: Support some type conversion vectorization in SLP
08f65a5: 8277313: Validate header failed for test/jdk/java/net/httpclient/HeadTest.java
23e5117: 8276559: (httpclient) Consider adding an HttpRequest.Builder.HEAD method to build a HEAD request.
a77d8dd: 8276787: Improve warning messages for -XX:+RecordDynamicDumpInfo
8ed384c: 8276609: Document setting property jdk.serialFilter to an invalid value throws ExceptionInInitializerError
cddc6ce: 8275811: Incorrect instance to dispose
b0a463f: 8169468: NoResizeEventOnDMChangeTest.java fails because FS Window didn't receive all resizes!
e5ffdf9: 8276231: ciReplay: SIGSEGV when replay compiling lambdas
d5e47d6: 8277089: Use system binutils to build hsdis
... and 14 more: https://git.openjdk.java.net/jdk/compare/a59c9b2ac277d6ff6be1700d91ff389f137e61ca...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@theRealAph, @nick-arm) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2021-11-01T04:17:11Z

@fg1417
Your change (at version 05c712d) is now ready to be sponsored by a Committer.

fg1417 · 2021-11-04T02:36:01Z

@theRealAph , could you please help approve it? Thanks for your time :)

Change-Id: I2004dc45f7f0ab44bc22b48083b185e7b3bd5eea

Change-Id: I1292449268c73c8f84cc3ffa7a4c859cf79058eb

fg1417 · 2021-11-17T09:27:18Z

Hi @theRealAph , I rebased my patch and retested it internally. Can I have your review :)? Thanks.

theRealAph

Good job, well done.

mlbridge · 2021-11-17T10:04:05Z

Mailing list message from Andrew Haley on hotspot-dev:

On 11/17/21 09:57, Andrew Haley wrote:

2734: if (is_vshift_con_pattern(n, m) ||
2735: (UseSVE > 0 && m->Opcode() == Op_VectorStoreMask && n->Opcode() == Op_StoreVector) ||
2736: is_vector_arith_imm_pattern(n, m)) {
Indent this line.

Sorry, that was a mistake.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

fg1417 · 2021-11-17T10:06:08Z

Thanks :) @theRealAph

fg1417 · 2021-11-17T10:06:14Z

/integrate

openjdk · 2021-11-17T10:07:58Z

@fg1417
Your change (at version bcd7ac0) is now ready to be sponsored by a Committer.

nsjian · 2021-11-18T02:40:00Z

/sponsor

openjdk · 2021-11-18T02:41:36Z

Going to push as commit 8193800.
Since your change was applied there have been 39 commits pushed to the master branch:

b8453eb: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales
231fb61: 8276970: Default charset for PrintWriter that wraps PrintStream
29e552c: 8272358: Some tests may fail when executed with other locales than the US
ce4471f: 8277346: ProblemList 7 serviceability/sa tests on macosx-x64
45a60db: 8277045: G1: Remove unnecessary set_concurrency call in G1ConcurrentMark::weak_refs_work
6bb0462: 8277224: sun.security.pkcs.PKCS9Attributes.toString() throws NPE
d8c0280: 8277316: ciReplay: dump_replay_data is not thread-safe
007ad7c: 8277303: Terminology mismatch between JLS17-3.9 and SE17's javax.lang.model.SourceVersion method specs
8881f29: 8277310: ciReplay: @CPI MethodHandle references not resolved
262d070: 8277246: Check for NonRepudiation as well when validating a TSA certificate
... and 29 more: https://git.openjdk.java.net/jdk/compare/a59c9b2ac277d6ff6be1700d91ff389f137e61ca...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-18T02:41:51Z

@nsjian @fg1417 Pushed as commit 8193800.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label Oct 26, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 26, 2021

theRealAph reviewed Oct 26, 2021

View reviewed changes

Fei Gao added 2 commits October 28, 2021 09:53

Merge branch 'master' of github.com:fg1417/jdk into fg1417-20211026

7f141b7

Change-Id: I52aa66d200b74ac312c5d40283b94854bc1142e6

Split the original patch and leave the existing logic in Assembler en…

a7a915a

…tirely untouched Change-Id: If8ddcef07b15615d7dd0c3063c44d2b705fac6f7

fg1417 force-pushed the fg1417-20211026 branch from 5ffd5cb to a7a915a Compare October 28, 2021 09:59

theRealAph reviewed Oct 28, 2021

View reviewed changes

Add some assertion lines for help functions

05c712d

Change-Id: Ic9120902bd8f8a8ead2e3740435a40f35d21757c

nick-arm approved these changes Nov 1, 2021

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated sponsor Pull request is ready to be sponsored labels Nov 1, 2021

Fei Gao added 2 commits November 17, 2021 03:46

Merge branch 'master' of github.com:fg1417/jdk into fg1417-20211026

66e8a00

Change-Id: I2004dc45f7f0ab44bc22b48083b185e7b3bd5eea

Regenerate the asmtest.out.h file for aarch64 after rebasing

bcd7ac0

Change-Id: I1292449268c73c8f84cc3ffa7a4c859cf79058eb

openjdk bot removed the sponsor Pull request is ready to be sponsored label Nov 17, 2021

theRealAph approved these changes Nov 17, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 17, 2021

openjdk bot closed this Nov 18, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 18, 2021

8274179: AArch64: Support SVE operations with encodable immediates #6115

8274179: AArch64: Support SVE operations with encodable immediates #6115

Uh oh!

Conversation

fg1417 commented Oct 26, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 26, 2021

Uh oh!

openjdk bot commented Oct 26, 2021

Uh oh!

mlbridge bot commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora commented Oct 26, 2021

Uh oh!

fg1417 commented Oct 26, 2021

Uh oh!

dholmes-ora commented Oct 26, 2021

Uh oh!

fg1417 commented Oct 26, 2021

Uh oh!

theRealAph Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fg1417 Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

theRealAph commented Oct 26, 2021

Uh oh!

fg1417 commented Oct 28, 2021

Uh oh!

theRealAph Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

fg1417 Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

theRealAph Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

fg1417 Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

theRealAph left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 1, 2021

Uh oh!

fg1417 commented Nov 4, 2021

Uh oh!

fg1417 commented Nov 17, 2021

Uh oh!

theRealAph left a comment

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Nov 17, 2021

Uh oh!

fg1417 commented Nov 17, 2021

Uh oh!

fg1417 commented Nov 17, 2021

Uh oh!

openjdk bot commented Nov 17, 2021

Uh oh!

nsjian commented Nov 18, 2021

Uh oh!

openjdk bot commented Nov 18, 2021

Uh oh!

openjdk bot commented Nov 18, 2021

Uh oh!

Reviewers

fg1417 commented Oct 26, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 26, 2021 •

edited

Loading

theRealAph Oct 26, 2021 •

edited

Loading

openjdk bot commented Nov 1, 2021 •

edited

Loading