8264469: Add Insert float nodes implementation for Arm SVE #56

Wanghuang-Huawei · 2021-03-31T02:50:03Z

Add Insert float nodes implementation for Arm SVE, like insertD & insertF
add fast path by using cmpeq (SVE compare vector with immediate). For the range limit of imm5 is [-16, 15], I shift the index range from [0, 31] to [-16, 15].

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed

Issue

JDK-8264469: Add Insert float nodes implementation for Arm SVE

Reviewers

Ningsheng Jian (@nsjian - Committer)
Xiaohong Gong (@XiaohongGong - Committer)

Contributors

Wang Huang <whuang@openjdk.org>
Ai Jiaming <aijiaming1@huawei.com>
He Xuejin <hexuejin2@huawei.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-vector pull/56/head:pull/56
$ git checkout pull/56

Update a local copy of the PR:
$ git checkout pull/56
$ git pull https://git.openjdk.java.net/panama-vector pull/56/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 56

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-vector/pull/56.diff

Wanghuang-Huawei · 2021-03-31T02:50:44Z

/contributor add Wang Huang whuang@openjdk.org
/contributor add Ai Jiaming aijiaming1@huawei.com
/contributor add He Xuejin hexuejin2@huawei.com

bridgekeeper · 2021-03-31T02:50:47Z

👋 Welcome back whuang! A progress list of the required criteria for merging this PR into vectorIntrinsics 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-03-31T02:52:44Z

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

openjdk · 2021-03-31T02:54:14Z

@Wanghuang-Huawei
Contributor Ai Jiaming <aijiaming1@huawei.com> successfully added.

openjdk · 2021-03-31T02:55:51Z

@Wanghuang-Huawei
Contributor He Xuejin <hexuejin2@huawei.com> successfully added.

mlbridge · 2021-03-31T02:59:42Z

Webrevs

openjdk · 2021-03-31T07:22:45Z

@Wanghuang-Huawei 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:

8264469: Add Insert float nodes implementation for Arm SVE

Co-authored-by: Wang Huang <whuang@openjdk.org>
Co-authored-by: Ai Jiaming <aijiaming1@huawei.com>
Co-authored-by: He Xuejin <hexuejin2@huawei.com>
Reviewed-by: njian, xgong

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 453 new commits pushed to the vectorIntrinsics branch:

cc3f2ea: 8264811: Fix inconsistence of m4 and ad files after merge
a1be5e4: Merge panama-vector:master
b7baca7: 8264288: Performance issue with MethodHandle.asCollector
9201899: 8264729: Random check-in failing header checks.
d920f85: 8264540: WhiteBox.metaspaceReserveAlignment should return shared region alignment
104e925: 8264512: jdk/test/jdk/java/util/prefs/ExportNode.java relies on default platform encoding
a0ec2cb: 8248862: Implement Enhanced Pseudo-Random Number Generators
39719da: 8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI
a8005ef: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
7f9ece2: 8264650: Cross-compilation to macos/aarch64
... and 443 more: https://git.openjdk.java.net/panama-vector/compare/84bc8373a300c431e304f81b360e3f904ddfa678...vectorIntrinsics

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.

➡️ To integrate this PR with the above commit message to the vectorIntrinsics branch, type /integrate in a new comment.

XiaohongGong · 2021-03-31T08:50:45Z

src/hotspot/cpu/aarch64/aarch64_sve.ad

+    Assembler::SIMD_RegVariant size =
+              elemType_to_regVariant(vector_element_basic_type(this));
+    __ sve_index(as_FloatRegister($tmp$$reg), __ S, 0, 1);
+    __ sve_dup(as_FloatRegister($tmp2$$reg), __ S, (int)($idx$$constant));
+    __ sve_cmpeq(as_PRegister($pTmp$$reg), size, ptrue,


Use size instead of __ S all through the codes ? Or remove line 3322, and use __ S.

Good catch. I think this patch has separated rules for different types, so removing line 3322 is better.

Totally agree!

Sure. I will remove line 3322 and use __ S in cmpeq.

XiaohongGong · 2021-03-31T08:57:21Z

src/hotspot/cpu/aarch64/aarch64_sve.ad

+  predicate(UseSVE > 0 &&
+            n->bottom_type()->is_vect()->element_basic_type() == T_FLOAT);
+  match(Set dst (VectorInsert (Binary src val) idx));
+  effect(TEMP tmp, TEMP tmp2, TEMP pTmp, KILL cr);


Use tmp1, tmp2 instead of tmp, tmp2 ?

XiaohongGong · 2021-03-31T09:02:52Z

src/hotspot/cpu/aarch64/aarch64_sve.ad

+    __ sve_dup(as_FloatRegister($tmp2$$reg), __ S, (int)($idx$$constant));
+    __ sve_cmpeq(as_PRegister($pTmp$$reg), size, ptrue,


SVE support immedicate CMP: CMPEQ <Pd>.<T>, <Pg>/Z, <Zn>.<T>, #<imm>. Could you please use the imm version here? We can save one dup instruction here.

SVE support immedicate CMP: CMPEQ <Pd>.<T>, <Pg>/Z, <Zn>.<T>, #<imm>. Could you please use the imm version here? We can save one dup instruction here.

I have remembered why I choose this cmpeq. If you use immedicate CMP, the # <imm> is in range [-16,15]. I think we should use cmpeq in my codes.

OK, so a vector CMP is needed here. Thanks for the explanation!

Maybe you can check whether idx is in CMPEQ imm range to generate different code?

Maybe you can check whether idx is in CMPEQ imm range to generate different code?

I add a fast path by using CMPEQ imm.

I shift the range of idx from [0,31] to [-16,15] to suit the range limit of imm5.

XiaohongGong · 2021-03-31T09:07:51Z

src/hotspot/cpu/aarch64/aarch64_sve.ad

+  format %{ "sve_index $tmp, S, 0, 1\n\t"
+            "sve_dup $tmp2, S, $idx\n\t"


Since there is (F) at the end of the format, is the type info S really needed here? I just think it's inconsistent with the following formats which do not have the type info inside.

XiaohongGong · 2021-03-31T09:22:14Z

src/hotspot/cpu/aarch64/aarch64_sve_ad.m4

 VECTOR_INSERT(S, iRegIorL2I, H, Register)
 VECTOR_INSERT(I, iRegIorL2I, S, Register)


I'm sorry that I didn't look at the implementation PR for integer type. Is it better to merge the rules for "B/H/S" as a single one?

I'm sorry that I didn't look at the implementation PR for integer type. Is it better to merge the rules for "B/H/S" as a single one?

Agree. I plan to do some cleanup work before integrating to jdk mainline, which will include such kind of merging. If @Wanghuang-Huawei can do it in this patch, that would help to reduce future refactoring work.

It's a good idea. However, we can not comment the type info after merging, so I choose this implementation.

nsjian · 2021-04-02T08:59:44Z

src/hotspot/cpu/aarch64/aarch64_sve.ad

 %{
-  predicate(UseSVE > 0 &&
+  predicate(UseSVE > 0 && n->as_Vector()->length() > 32 &&


Is it possible to have more than 32 doubles/longs?

Yes. It is useless rule now. I'll remove it.

XiaohongGong · 2021-04-06T01:59:37Z

src/hotspot/cpu/aarch64/aarch64_sve.ad

+  ins_pipe(pipe_slow);
+%}
+
+instruct insertL_fast(vReg dst, vReg src, iRegL val, immI idx, vReg tmp, pRegGov pTmp, rFlagsReg cr)


Since the n->as_Vector()->length() >32 doesn't exist, is insertL better here?

It is considered that all concise rules (which is named fast now ) should have the common macro , otherwise we may use too many macros.

Yes, maybe a separate macro for L/D is better.

XiaohongGong · 2021-04-06T02:00:13Z

src/hotspot/cpu/aarch64/aarch64_sve.ad

+
+instruct insertL_fast(vReg dst, vReg src, iRegL val, immI idx, vReg tmp, pRegGov pTmp, rFlagsReg cr)
+%{
+  predicate(UseSVE > 0 && n->as_Vector()->length() <= 32 &&


Can n->as_Vector()->length() <= 32 be removed? Or maybe an assertion is better.

Yes . The reason is the same as last comment. #56 (comment)
Or we COULD split insertL and insertD from other ones ?

This seems reasonable to me. Agree to split L/D from others.

XiaohongGong · 2021-04-06T03:24:31Z

Looks good to me, thanks!

Wanghuang-Huawei · 2021-04-06T03:26:02Z

/integrate

Wanghuang-Huawei · 2021-04-06T03:26:20Z

Looks good to me, thanks!

Thank you for your review.

openjdk · 2021-04-06T03:27:49Z

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

nsjian · 2021-04-06T03:38:41Z

Looks good to me, but I would like to defer the integration, until @sviswa7 integrates #58, in case of any conflict.

nsjian · 2021-04-07T04:09:07Z

/sponsor

openjdk · 2021-04-07T04:10:35Z

@nsjian This change does not need sponsoring - the author is allowed to integrate it.

Wanghuang-Huawei · 2021-04-07T06:03:55Z

/integrate

openjdk · 2021-04-07T06:05:15Z

@Wanghuang-Huawei Since your change was applied there have been 453 commits pushed to the vectorIntrinsics branch:

cc3f2ea: 8264811: Fix inconsistence of m4 and ad files after merge
a1be5e4: Merge panama-vector:master
b7baca7: 8264288: Performance issue with MethodHandle.asCollector
9201899: 8264729: Random check-in failing header checks.
d920f85: 8264540: WhiteBox.metaspaceReserveAlignment should return shared region alignment
104e925: 8264512: jdk/test/jdk/java/util/prefs/ExportNode.java relies on default platform encoding
a0ec2cb: 8248862: Implement Enhanced Pseudo-Random Number Generators
39719da: 8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI
a8005ef: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
7f9ece2: 8264650: Cross-compilation to macos/aarch64
... and 443 more: https://git.openjdk.java.net/panama-vector/compare/84bc8373a300c431e304f81b360e3f904ddfa678...vectorIntrinsics

Your commit was automatically rebased without conflicts.

Pushed as commit b6a5e16.

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

8264469: Add Insert float nodes implementation for Arm SVE

cf86ee0

openjdk bot added the rfr label Mar 31, 2021

nsjian approved these changes Mar 31, 2021

View reviewed changes

openjdk bot added the ready label Mar 31, 2021

XiaohongGong reviewed Mar 31, 2021

View reviewed changes

add fast path

dafe3ed

nsjian reviewed Apr 2, 2021

View reviewed changes

remove useless rule

57fa497

XiaohongGong reviewed Apr 6, 2021

View reviewed changes

Wanghuang-Huawei added 2 commits April 6, 2021 10:43

refact codes

9698350

rename macro

b471c94

XiaohongGong approved these changes Apr 6, 2021

View reviewed changes

openjdk bot added the sponsor label Apr 6, 2021

nsjian approved these changes Apr 7, 2021

View reviewed changes

openjdk bot closed this Apr 7, 2021

openjdk bot added integrated and removed ready rfr labels Apr 7, 2021

		__ sve_dup(as_FloatRegister($tmp2$$reg), __ S, (int)($idx$$constant));
		__ sve_cmpeq(as_PRegister($pTmp$$reg), size, ptrue,

		format %{ "sve_index $tmp, S, 0, 1\n\t"
		"sve_dup $tmp2, S, $idx\n\t"

		VECTOR_INSERT(S, iRegIorL2I, H, Register)
		VECTOR_INSERT(I, iRegIorL2I, S, Register)

8264469: Add Insert float nodes implementation for Arm SVE #56

8264469: Add Insert float nodes implementation for Arm SVE #56

Conversation

Wanghuang-Huawei commented Mar 31, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Contributors

Reviewing

Wanghuang-Huawei commented Mar 31, 2021

bridgekeeper bot commented Mar 31, 2021

openjdk bot commented Mar 31, 2021

openjdk bot commented Mar 31, 2021

openjdk bot commented Mar 31, 2021

mlbridge bot commented Mar 31, 2021 • edited

Webrevs

openjdk bot commented Mar 31, 2021 • edited

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiaohongGong commented Apr 6, 2021

Wanghuang-Huawei commented Apr 6, 2021

Wanghuang-Huawei commented Apr 6, 2021

openjdk bot commented Apr 6, 2021

nsjian commented Apr 6, 2021

nsjian commented Apr 7, 2021

openjdk bot commented Apr 7, 2021

Wanghuang-Huawei commented Apr 7, 2021

openjdk bot commented Apr 7, 2021

Wanghuang-Huawei commented Mar 31, 2021 •

edited by openjdk bot

mlbridge bot commented Mar 31, 2021 •

edited

openjdk bot commented Mar 31, 2021 •

edited