8259948: Aarch64: Add cast nodes for Aarch64 Neon backend #4839

Wanghuang-Huawei · 2021-07-20T14:55:42Z

In this issue, we plan to complete all missing implementation for aarch64 neon backend. For example, cast from Byte to Long, cast from Long to Byte, and so on.
It may be a solver of JDK-8269866, or part of it.

Progress

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

Issue

JDK-8259948: Aarch64: Add cast nodes for Aarch64 Neon backend

Reviewers

Andrew Haley (@theRealAph - Reviewer)
Eric Liu (@theRealELiu - Author)
Ningsheng Jian (@nsjian - Committer)

Contributors

Wang Huang <whuang@openjdk.org>
Wu Yan <wuyan@openjdk.org>
Miao Zhuojun <mouzhuojun@huawei.com>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4839

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

Using diff file

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

bridgekeeper · 2021-07-20T14:56:10Z

👋 Welcome back whuang! 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.

Wanghuang-Huawei · 2021-07-20T14:56:36Z

/contributor add Wang Huang whuang@openjdk.org
/contributor add Wu Yan wuyan@openjdk.org
/contributor add Miao Zhuojun mouzhuojun@huawei.com

openjdk · 2021-07-20T14:58:06Z

@Wanghuang-Huawei
Contributor Wang Huang <whuang@openjdk.org> successfully added.

openjdk · 2021-07-20T14:59:15Z

@Wanghuang-Huawei
Contributor Wu Yan <wuyan@openjdk.org> successfully added.

openjdk · 2021-07-20T15:00:19Z

@Wanghuang-Huawei
Contributor Miao Zhuojun <mouzhuojun@huawei.com> successfully added.

openjdk · 2021-07-20T15:01:54Z

@Wanghuang-Huawei The following label will be automatically applied to this pull request:

hotspot-compiler

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.

nsjian · 2021-07-21T03:35:01Z

Thanks for the work! Some general comments:

It may be a solver of JDK-8269866, or part of it.

I would suggest not to have a partial fix of JDK-8269866. I think you can still keep 8259948 as duplicate while targeting this to JDK-8269866 and have a fully proper fix. @theRealELiu may have some thoughts on how to have a clean fix: e.g. there may be some dependency on mid-end part, like JDK-8265244?

@theRealELiu has marked those missing rules opcode (with specific types/sizes) as unsupported in JDK-8268966, but I don't see you have unmarked them in your patch. So your newly added rules are not able to be tested. And there are some test cases included in JDK-8268966, could you please merge your test case into existing tests, if existing tests cannot cover some cases.

P.S. could you please fix the jcheck error?

Wanghuang-Huawei · 2021-08-03T13:25:22Z

Thanks for the work! Some general comments:

It may be a solver of JDK-8269866, or part of it.

I would suggest not to have a partial fix of JDK-8269866. I think you can still keep 8259948 as duplicate while targeting this to JDK-8269866 and have a fully proper fix. @theRealELiu may have some thoughts on how to have a clean fix: e.g. there may be some dependency on mid-end part, like JDK-8265244?

@theRealELiu has marked those missing rules opcode (with specific types/sizes) as unsupported in JDK-8268966, but I don't see you have unmarked them in your patch. So your newly added rules are not able to be tested. And there are some test cases included in JDK-8268966, could you please merge your test case into existing tests, if existing tests cannot cover some cases.

P.S. could you please fix the jcheck error?

Thank you for your suggestion. We fix these errors and upload new patch.

mlbridge · 2021-08-03T13:28:47Z

Webrevs

src/hotspot/cpu/aarch64/aarch64_neon_ad.m4

src/hotspot/cpu/aarch64/assembler_aarch64.hpp

theRealAph

The big question for all of this is: what is the test coverage?

wuyan0 · 2021-08-06T15:43:26Z

The big question for all of this is: what is the test coverage?

We tested the following two test cases:
test/hotspot/jtreg/compiler/vectorapi/VectorCastShape128Test.java
test/hotspot/jtreg/compiler/vectorapi/VectorCastShape64Test.java

test/hotspot/gtest/aarch64/aarch64-asmtest.py

theRealAph · 2021-08-07T09:31:14Z

The big question for all of this is: what is the test coverage?

We tested the following two test cases:
test/hotspot/jtreg/compiler/vectorapi/VectorCastShape128Test.java
test/hotspot/jtreg/compiler/vectorapi/VectorCastShape64Test.java

Thank you, but that doesn't really answer my question. Does your testing cover all that is added in this patch? If so, how did you ascertain it?

e1iu · 2021-08-10T03:04:02Z

src/hotspot/cpu/aarch64/aarch64_neon.ad

+  ins_encode %{
+    // If registers are the same, no register move is required - the
+    // upper bits of "src" are expected to have been initialized
+    // to zero.


I have a little concern about this assumption. How to ensure the upper bits are zero? Since the ReinterpretNode could be used separately and not always with CastNode together. E.g

https://github.com/openjdk/jdk/blob/jdk-18%2B9/src/hotspot/share/opto/vectorIntrinsics.cpp#L844

There are several places in the AArch64 back end where we expect the upper bits of a register to be zero, but we've never depended on it. This is not a good time to start, so let's clear the bits in order to be certain.

This is the same as reinterpretD2X. The upper 96 bits of "src" are zero, I guess it is because when the 128-bit register is initialized, the upper 96 bits will be zeroed, and only its lower 32-bit data will be manipulated later.

I think it's better to clear the higher bits for such cases.

OK, I'll clear the higher bits in the next commit.

wuyan0 · 2021-08-10T03:30:53Z

Does your testing cover all that is added in this patch? If so, how did you ascertain it?

Yes, These two test cases have covered the code in the patch. First, I unmark the unsupported opcodes in JDK-8268966. Then I test the cases in both of the above test files one by one, implementing the rules once the testcase failed until all the testcases pass.

test/hotspot/gtest/aarch64/aarch64-asmtest.py

theRealAph · 2021-08-10T09:13:17Z

test/hotspot/gtest/aarch64/asmtest.out.h

@@ -712,6 +712,7 @@
    __ stxp(r4, zr, zr, r5);                           //       stxp    w4, xzr, xzr, [x5]
    __ stxpw(r6, zr, zr, sp);                          //       stxp    w6, wzr, wzr, [sp]
    __ dup(v0, __ T16B, zr);                           //       dup     v0.16b, wzr
+    __ dups(v0, __ T2S, v1);                           //       mov     s0, v1.s[0]


Please use the DUP AArch64 instruction here, rather than MOV.

theRealAph · 2021-08-10T09:16:51Z

Does your testing cover all that is added in this patch? If so, how did you ascertain it?

Yes, These two test cases have covered the code in the patch. First, I unmark the unsupported opcodes in JDK-8268966. Then I test the cases in both of the above test files one by one, implementing the rules once the testcase failed until all the testcases pass.

OK.

src/hotspot/cpu/aarch64/aarch64_neon.ad

XiaohongGong · 2021-08-12T01:48:30Z

src/hotspot/cpu/aarch64/aarch64_neon.ad

+  ins_encode %{
+    // If registers are the same, no register move is required - the
+    // upper bits of "src" are expected to have been initialized
+    // to zero.


I think it's better to clear the higher bits for such cases.

src/hotspot/cpu/aarch64/aarch64_neon.ad

e1iu · 2021-10-14T03:06:48Z

I triggered the test, hotspot_all(no vmTestBase stress), langtools:tier1, jdk:tier1, tier2, tier3 are passed on aarch64.

nsjian

Looks good to me. Just some style nits.

src/hotspot/cpu/aarch64/aarch64_neon.ad

nsjian · 2021-10-19T10:03:17Z

src/hotspot/cpu/aarch64/aarch64_neon.ad

+  ins_pipe(pipe_slow);
+%}
+instruct vcvt4Fto4B(vecD dst, vecX src)
+%{


src/hotspot/cpu/aarch64/aarch64_neon.ad

e1iu

LGTM

wuyan0 · 2021-10-26T03:47:28Z

Thanks @nsjian @theRealELiu for reviewing this.

wuyan0 · 2021-10-26T03:49:32Z

Hi, @theRealAph do I need another reviewer to review this?

theRealAph

Thanks everyone, for all your hard work.

Wanghuang-Huawei · 2021-10-27T03:09:42Z

/integrate

openjdk · 2021-10-27T03:11:17Z

@Wanghuang-Huawei
Your change (at version f317a0b) is now ready to be sponsored by a Committer.

nsjian · 2021-10-27T05:31:52Z

/sponsor

openjdk · 2021-10-27T05:33:00Z

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

d98b7c2: 8202926: Test java/awt/Focus/WindowUpdateFocusabilityTest/WindowUpdateFocusabilityTest.html fails
b0d1e4f: 8273585: String.charAt performance degrades due to JDK-8268698
7addcd7: 8276034: ProblemList gtest dll_address_to_function_and_library_name on macosx-x64
2448b3f: 8275874: [JVMCI] only support aligned reads in c2v_readFieldValue
f1f5e26: 8275872: Sync J2DBench run and analyze Makefile targets with build.xml
19f76c2: 8275079: Remove unnecessary conversion to String in java.net.http
e5cd269: 8274944: AppCDS dump causes SEGV in VM thread while adjusting lambda proxy class info
82f4aac: 8259609: C2: optimize long range checks in long counted loops
574f890: 8275720: CommonComponentAccessibility.createWithParent isWrapped causes mem leak
7c88a59: 8275809: crash in [CommonComponentAccessibility getCAccessible:withEnv:]
... and 117 more: https://git.openjdk.java.net/jdk/compare/947d52c4c3deec1bdea43959c200201c614ae114...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-27T05:33:17Z

@nsjian @Wanghuang-Huawei Pushed as commit 9f75d5c.

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

8259948: Aarch64: Add cast nodes for Aarch64 Neon backend

b87fc79

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 20, 2021

fix bugs

aa46fae

openjdk bot added the rfr Pull request is ready for review label Aug 3, 2021

theRealAph reviewed Aug 4, 2021

View reviewed changes

src/hotspot/cpu/aarch64/aarch64_neon_ad.m4 Outdated Show resolved Hide resolved

theRealAph reviewed Aug 4, 2021

View reviewed changes

src/hotspot/cpu/aarch64/assembler_aarch64.hpp Outdated Show resolved Hide resolved

theRealAph reviewed Aug 4, 2021

View reviewed changes

src/hotspot/cpu/aarch64/assembler_aarch64.hpp Show resolved Hide resolved

theRealAph reviewed Aug 4, 2021

View reviewed changes

fix code style

620e495

theRealAph reviewed Aug 7, 2021

View reviewed changes

test/hotspot/gtest/aarch64/aarch64-asmtest.py Outdated Show resolved Hide resolved

e1iu reviewed Aug 10, 2021

View reviewed changes

theRealAph reviewed Aug 10, 2021

View reviewed changes

test/hotspot/gtest/aarch64/aarch64-asmtest.py Outdated Show resolved Hide resolved

theRealAph reviewed Aug 10, 2021

View reviewed changes

e1iu reviewed Aug 10, 2021

View reviewed changes

src/hotspot/cpu/aarch64/aarch64_neon.ad Outdated Show resolved Hide resolved

XiaohongGong reviewed Aug 12, 2021

View reviewed changes

fix codes

aa4937c

theRealAph reviewed Aug 18, 2021

View reviewed changes

src/hotspot/cpu/aarch64/aarch64_neon.ad Show resolved Hide resolved

merge master & fix conflict

c8a0134

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Sep 27, 2021

remove useless codes

e8e6f01

sync asmtest.out.h

8627713

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Oct 14, 2021

Merge branch 'master' into JDK-8259948

92c1404

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Oct 19, 2021

merge master

a7c562e

nsjian approved these changes Oct 19, 2021

View reviewed changes

fix styles

f317a0b

e1iu approved these changes Oct 24, 2021

View reviewed changes

nsjian approved these changes Oct 26, 2021

View reviewed changes

theRealAph approved these changes Oct 26, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 27, 2021

openjdk bot closed this Oct 27, 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 Oct 27, 2021

8259948: Aarch64: Add cast nodes for Aarch64 Neon backend #4839

8259948: Aarch64: Add cast nodes for Aarch64 Neon backend #4839

Wanghuang-Huawei commented Jul 20, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jul 20, 2021

Wanghuang-Huawei commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

nsjian commented Jul 21, 2021

Wanghuang-Huawei commented Aug 3, 2021

mlbridge bot commented Aug 3, 2021 •

edited

theRealAph left a comment

wuyan0 commented Aug 6, 2021

theRealAph commented Aug 7, 2021

e1iu Aug 10, 2021

theRealAph Aug 10, 2021

wuyan0 Aug 10, 2021

XiaohongGong Aug 12, 2021

wuyan0 Aug 12, 2021

wuyan0 commented Aug 10, 2021

theRealAph Aug 10, 2021

wuyan0 Aug 12, 2021

theRealAph commented Aug 10, 2021

XiaohongGong Aug 12, 2021

e1iu commented Oct 14, 2021

nsjian left a comment

nsjian Oct 19, 2021

e1iu left a comment

wuyan0 commented Oct 26, 2021

wuyan0 commented Oct 26, 2021

theRealAph left a comment

Wanghuang-Huawei commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

nsjian commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

8259948: Aarch64: Add cast nodes for Aarch64 Neon backend #4839

8259948: Aarch64: Add cast nodes for Aarch64 Neon backend #4839

Conversation

Wanghuang-Huawei commented Jul 20, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Jul 20, 2021

Wanghuang-Huawei commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

openjdk bot commented Jul 20, 2021

nsjian commented Jul 21, 2021

Wanghuang-Huawei commented Aug 3, 2021

mlbridge bot commented Aug 3, 2021 • edited

Webrevs

theRealAph left a comment

Choose a reason for hiding this comment

wuyan0 commented Aug 6, 2021

theRealAph commented Aug 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuyan0 commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theRealAph commented Aug 10, 2021

Choose a reason for hiding this comment

e1iu commented Oct 14, 2021

nsjian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e1iu left a comment

Choose a reason for hiding this comment

wuyan0 commented Oct 26, 2021

wuyan0 commented Oct 26, 2021

theRealAph left a comment

Choose a reason for hiding this comment

Wanghuang-Huawei commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

nsjian commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

openjdk bot commented Oct 27, 2021

Wanghuang-Huawei commented Jul 20, 2021 •

edited by openjdk bot

mlbridge bot commented Aug 3, 2021 •

edited