Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8265907: JVM crashes when matching VectorMaskCmp Node #3670

Closed
wants to merge 5 commits into from

Conversation

@Wanghuang-Huawei
Copy link

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 25, 2021

  • fix the issue JDK-8265907
  • all archs might be effected by this bug. I fixed x86 and aarch64.

Progress

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

Issue

  • JDK-8265907: JVM crashes when matching VectorMaskCmp Node

Reviewers

Contributors

  • Wang Huang <whuang@openjdk.org>
  • Ai Jiaming <aijiaming1@huawei.com>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3670

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 25, 2021

👋 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.

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 25, 2021

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

Loading

@openjdk openjdk bot added the rfr label Apr 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 25, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 25, 2021

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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 25, 2021

@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.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 25, 2021

Loading

case Op_VectorMaskCmp:
if (vlen < 2 || bit_size < 64) {
return false;
}
break;
Copy link

@nsjian nsjian Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, currently min_vector_size for byte type is lower to 4 bytes to support vector api shuffle, but we don't have a per Opcode size check correctly. E.g. no length check for reduce_add8B, do you also see any issue for that? I think we'd better only allow 4 bytes for vector api shuffle related opcodes (and 2 for mask related opcodes) in match_rule_supported_vector, while for other opcodes, we still only support for >=64 bits.

Loading

Copy link

@nsjian nsjian May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, currently min_vector_size for byte type is lower to 4 bytes to support vector api shuffle, but we don't have a per Opcode size check correctly. E.g. no length check for reduce_add8B, do you also see any issue for that?

Since SLP does not support subword reduction, I think current match rules are fine. AArch64 part looks good to me.

Loading

@Wanghuang-Huawei Wanghuang-Huawei changed the title 8265907: JVM crashes when matching VectorMaskCmp Node for Neon backend 8265907: JVM crashes when matching VectorMaskCmp Node Apr 26, 2021
case Op_VectorMaskCmp:
if (vlen < 2 || size_in_bits < 64) {
return false;
}
break;
Copy link
Contributor

@XiaohongGong XiaohongGong Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you also see the crash on x86?

Loading

Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this bug effects x86.

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 28, 2021

@Wanghuang-Huawei This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented May 29, 2021

@iwanowww @mgkwill Could you do me a favor to review this issue? Any suggestion is welcome.

Loading

@mgkwill
Copy link
Contributor

@mgkwill mgkwill commented May 31, 2021

@jatin-bhateja and @sviswa7 could you review this for x86?

Loading

@jatin-bhateja
Copy link
Member

@jatin-bhateja jatin-bhateja commented May 31, 2021

Hi @Wanghuang-Huawei ,
By adding suggested check we may suppress iotaShuffle intrinsification for Long256, Int128, Short64 species.
Since currently shuffles are stored in a byte array so this case seeped through the constraint to prevent vectorization imposed by match rule supported vector (i.e. vlen < 4 for byte vectors).

Please let me know if its ok to integrate following changes in your patch.

diff --git a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
index 310bbbfa150..ca77ee043e2 100644
--- a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
+++ b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
@@ -1496,7 +1496,11 @@ void C2_MacroAssembler::load_vector_mask(XMMRegister dst, XMMRegister src, int v
 
 void C2_MacroAssembler::load_iota_indices(XMMRegister dst, Register scratch, int vlen_in_bytes) {
   ExternalAddress addr(StubRoutines::x86::vector_iota_indices());
-  if (vlen_in_bytes <= 16) {
+  if (vlen_in_bytes == 4) {
+    movdl(dst, addr);
+  } else if (vlen_in_bytes == 8) {
+    movq(dst, addr);
+  } else if (vlen_in_bytes == 16) {
     movdqu(dst, addr, scratch);
   } else if (vlen_in_bytes == 32) {
     vmovdqu(dst, addr, scratch);
@@ -1505,6 +1509,7 @@ void C2_MacroAssembler::load_iota_indices(XMMRegister dst, Register scratch, int
     evmovdqub(dst, k0, addr, false /*merge*/, Assembler::AVX_512bit, scratch);
   }
 }
+
 // Reductions for vectors of bytes, shorts, ints, longs, floats, and doubles.
 
 void C2_MacroAssembler::reduce_operation_128(BasicType typ, int opcode, XMMRegister dst, XMMRegister src) {
diff --git a/src/hotspot/cpu/x86/x86.ad b/src/hotspot/cpu/x86/x86.ad
index 5f6d5bc6fdb..cb23ebdd9db 100644
--- a/src/hotspot/cpu/x86/x86.ad
+++ b/src/hotspot/cpu/x86/x86.ad
@@ -6867,7 +6867,7 @@ instruct evcmpFD(vec dst, vec src1, vec src2, immI8 cond, rRegP scratch, kReg kt
 %}
 
 instruct vcmp(legVec dst, legVec src1, legVec src2, immI8 cond, rRegP scratch) %{
-  predicate(vector_length_in_bytes(n->in(1)->in(1)) >=  8 && // src1
+  predicate(vector_length_in_bytes(n->in(1)->in(1)) >=  4 && // src1
             vector_length_in_bytes(n->in(1)->in(1)) <= 32 && // src1
             is_integral_type(vector_element_basic_type(n->in(1)->in(1)))); // src1
   match(Set dst (VectorMaskCmp (Binary src1 src2) cond));
diff --git a/src/hotspot/share/opto/vectorIntrinsics.cpp b/src/hotspot/share/opto/vectorIntrinsics.cpp
index f613299a1c4..e9657e1136d 100644
--- a/src/hotspot/share/opto/vectorIntrinsics.cpp
+++ b/src/hotspot/share/opto/vectorIntrinsics.cpp
@@ -340,9 +340,6 @@ bool LibraryCallKit::inline_vector_shuffle_iota() {
   int num_elem = vlen->get_con();
   BasicType elem_bt = T_BYTE;
 
-  if (num_elem < 4)
-    return false;
-
   if (!arch_supports_vector(VectorNode::replicate_opcode(elem_bt), num_elem, elem_bt, VecMaskNotUsed)) {
     return false;
   }

Loading

@jatin-bhateja
Copy link
Member

@jatin-bhateja jatin-bhateja commented May 31, 2021

Hi @Wanghuang-Huawei , I have updated the proposed fix.

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Jun 2, 2021

Hi @Wanghuang-Huawei , I have updated the proposed fix.

Thank you for your suggestion. I think your codes can solve the x86's problem here. I will add it to my next push.

Loading

@sviswa7
Copy link

@sviswa7 sviswa7 commented Jun 4, 2021

x86 changes look good to me.

Loading

sviswa7
sviswa7 approved these changes Jun 9, 2021
Copy link

@sviswa7 sviswa7 left a comment

@Wanghuang-Huawei Are you planning to proceed with this? x86 changes are good. You will need an approval from aarch64 side. Please do a rebase to latest.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

@Wanghuang-Huawei this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8265907
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Jun 9, 2021

@Wanghuang-Huawei Are you planning to proceed with this? x86 changes are good. You will need an approval from aarch64 side. Please do a rebase to latest.

OK. I will rebase & solve the conflict. Thank you.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

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

8265907: JVM crashes when matching VectorMaskCmp Node

Co-authored-by: Wang Huang <whuang@openjdk.org>
Co-authored-by: Ai Jiaming <aijiaming1@huawei.com>
Reviewed-by: sviswanathan, njian, jbhateja

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 64 new commits pushed to the master branch:

  • df65237: 8267930: Refine code for loading hsdis library
  • 2e900da: 8268574: ProblemList tests failing due to UseBiasedLocking going away
  • 4fd2a14: 8267556: Enhance class paths check during runtime
  • 8c8422e: 8267893: Improve jtreg test failure handler do get native/mixed stack traces for cores and live processes
  • 1e1039a: 8268223: Problemlist vmTestbase/nsk/jdi/HiddenClass/events/events001.java
  • 78cb677: 8268539: several serviceability/sa tests should be run in driver mode
  • 7267227: 8268361: Fix the infinite loop in next_line
  • b018c45: 8267630: Start of release updates for JDK 18
  • 7400789: 8268542: serviceability/logging/TestFullNames.java tests only 1st test case
  • a95e64c: 8268443: ParallelGC Full GC should use parallel WeakProcessor
  • ... and 54 more: https://git.openjdk.java.net/jdk/compare/2cc1977a9698af9538101a5842c311659521a0aa...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 (@sviswa7, @nsjian, @jatin-bhateja) 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).

Loading

@openjdk openjdk bot added the ready label Jun 9, 2021
@openjdk openjdk bot removed the merge-conflict label Jun 9, 2021
nsjian
nsjian approved these changes Jun 10, 2021
Copy link

@nsjian nsjian left a comment

AArch64 change looks good to me (not a Reviewer).

Loading

@sviswa7
Copy link

@sviswa7 sviswa7 commented Jun 10, 2021

@Wanghuang-Huawei Could you please submit this fix versus JDK17 repo which forked today?

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Jun 11, 2021

@Wanghuang-Huawei Could you please submit this fix versus JDK17 repo which forked today?

OK. I will submit it.

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Jun 11, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Jun 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 11, 2021

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

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Jun 11, 2021

@theRealAph Could you do me a favor to review this patch?

Loading

@nsjian
Copy link

@nsjian nsjian commented Jun 11, 2021

I think this should go to https://github.com/openjdk/jdk17?

Loading

@PaulSandoz
Copy link
Member

@PaulSandoz PaulSandoz commented Jun 11, 2021

@nsjian yes, withdraw this PR and redo it for https://github.com/openjdk/jdk17

Loading

@nsjian
Copy link

@nsjian nsjian commented Jun 18, 2021

@Wanghuang-Huawei Could you please create the PR in jdk17 and withdraw this as suggested by @PaulSandoz ?

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Jun 24, 2021

@nsjian @PaulSandoz I will recommit this patch to jdk17 soon.

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Jun 24, 2021

Loading

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Jul 6, 2021

This PR can be closed now.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment