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

8261394: [vector] Crash with "assert(Matcher::vector_size_supported(elem_bt, length)) failed: length in range" #38

Closed

Conversation

@XiaohongGong
Copy link
Collaborator

@XiaohongGong XiaohongGong commented Feb 10, 2021

The crash is introduced by [1] and happens when the compiler is making an unspported vector type:

  assert(Matcher::vector_size_supported(elem_bt, length)) failed: length in range

Before vectorization for each vector intrinsic, the hotspot will check whether current arch supports the vector operation from several aspects like the backend implementation, the vector type and length. Once one of them is not matched, the compiler will stop the vectorization and go back to the default java implementation.

The changes in [1] missed the check for "Op_CallLeafVector". I think the double 64-bits vector is not supported to be vectorized. However, due to the missing check, the compiler continues the progress and then the crash happens.

This patch fixes it by making sure the check contains all opcodes.

[1] https://bugs.openjdk.java.net/browse/JDK-8261267


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • JDK-8261394: [vector] Crash with "assert(Matcher::vector_size_supported(elem_bt, length)) failed: length in range"

Reviewers

Download

$ git fetch https://git.openjdk.java.net/panama-vector pull/38/head:pull/38
$ git checkout pull/38

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 10, 2021

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

Loading

@openjdk openjdk bot added the rfr label Feb 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 10, 2021

Webrevs

Loading

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented Feb 18, 2021

Hi, could anyone please take a look at this PR? Thanks so much!

Loading

@XiaohongGong XiaohongGong changed the title 8261484: Double64VectorTests.java crashes on x86 8261394: [vector] Crash with "assert(Matcher::vector_size_supported(elem_bt, length)) failed: length in range" Feb 18, 2021
nsjian
nsjian approved these changes Feb 18, 2021
Copy link
Collaborator

@nsjian nsjian left a comment

Looks good to me, but perhaps @sviswa7 may take a look as well?

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 18, 2021

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

8261394: [vector] Crash with "assert(Matcher::vector_size_supported(elem_bt, length)) failed: length in range"

Reviewed-by: njian, jiefu, sviswanathan

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 no new commits pushed to the vectorIntrinsics branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 (@nsjian, @sviswa7) 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 Feb 18, 2021
@DamonFool
Copy link
Member

@DamonFool DamonFool commented Feb 18, 2021

For ACOSDouble64VectorTests, I think @sviswa7 would like to generate code like this:

098     B6: #   out( B14 B7 ) <- in( B5 )  Freq: 0.999977
098     # checkcastPP of RBX
098     movl    R11, [RBX + #12 (8-bit)]        # compressed ptr ! Field: jdk/internal/vm/vector/VectorSupport$VectorPayload.payload (constant)
09c     load_vector XMM0,[R12 + R11 << 3 + #16] (compressed oop addressing)
0a3     call_leaf,vector  vector_acos_double64
        No JVM State Info
        #

However, the fix will generate code like this.

095     B6: #   out( B18 B7 ) <- in( B5 )  Freq: 0.999977
095     movq    R9, RAX # spill
098     # checkcastPP of R9
098     movq    RDI, jdk/incubator/vector/DoubleVector$$Lambda$27+0x0000000801052e28:exact *    # ptr
0a2     movl    RSI, #108       # int
0a7     movq    RDX, java/lang/Class:exact *    # ptr
0b1     movq    RCX, java/lang/Class:exact *    # ptr
0bb     movl    R8, #1  # int
        nop     # 3 bytes pad for loops and calls
0c4     call,static  jdk.internal.vm.vector.VectorSupport::unaryOp
        # jdk.incubator.vector.DoubleVector::lanewiseTemplate @ bci:71 (line 554) L[0]=_ L[1]=_ L[2]=_
        # jdk.incubator.vector.Double64Vector::lanewise @ bci:2 (line 273) L[0]=_ L[1]=_
        # jdk.incubator.vector.Double64Vector::lanewise @ bci:2 (line 41) L[0]=_ L[1]=_
        # Test::ACOSDouble64VectorTests @ bci:15 (line 10) L[0]=_
        # OopMap {rbp=Oop off=204/0xcc}

Maybe, it's OK to call vector_acos_double64 for Double64Vector on x86.
Thanks.

Loading

@sviswa7
Copy link
Collaborator

@sviswa7 sviswa7 commented Feb 18, 2021

@DamonFool is correct, with this change the vector stubs are not being called at all.
@XiaohongGong @nsjian The correct fix would be as follows:
diff --git a/src/hotspot/share/opto/vectorIntrinsics.cpp b/src/hotspot/share/opto/vectorIntrinsics.cpp
index 056c415..b0ac90f 100644
--- a/src/hotspot/share/opto/vectorIntrinsics.cpp
+++ b/src/hotspot/share/opto/vectorIntrinsics.cpp
@@ -253,6 +253,9 @@ bool LibraryCallKit::inline_vector_nary_operation(int n) {
}
return false;
}

  • if (!Matcher::vector_size_supported(elem_bt, num_elem)) {
  •  return false;
    
  • }
    }

// TODO When mask usage is supported, VecMaskNotUsed needs to be VecMaskUseLoad.

Loading

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Feb 18, 2021

@DamonFool is correct, with this change the vector stubs are not being called at all.
@XiaohongGong @nsjian The correct fix would be as follows:
diff --git a/src/hotspot/share/opto/vectorIntrinsics.cpp b/src/hotspot/share/opto/vectorIntrinsics.cpp
index 056c415..b0ac90f 100644
--- a/src/hotspot/share/opto/vectorIntrinsics.cpp
+++ b/src/hotspot/share/opto/vectorIntrinsics.cpp
@@ -253,6 +253,9 @@ bool LibraryCallKit::inline_vector_nary_operation(int n) {
}
return false;
}

  • if (!Matcher::vector_size_supported(elem_bt, num_elem)) {
  •  return false;
    
  • }
    }

// TODO When mask usage is supported, VecMaskNotUsed needs to be VecMaskUseLoad.

This change doesn't call vector_acos_double64 for Double64Vector on x86 either.
But it seems reasonable and better to understand.

Let's push it to fix the bug ASAP.
And I'll check whether we can generate vector_acos_double64 for Double64Vector later.
Thanks.

Loading

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented Feb 19, 2021

Hi @sviswa7 @DamonFool, the suggested solution makes sense to me. Thanks for looking at this change. I will update the patch ASAP.

Loading

Copy link
Member

@DamonFool DamonFool left a comment

Looks good to me.
Thanks for fixing it.

Loading

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented Feb 20, 2021

Thanks for the review @DamonFool @sviswa7 @nsjian

Loading

@XiaohongGong
Copy link
Collaborator Author

@XiaohongGong XiaohongGong commented Feb 20, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Feb 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

@XiaohongGong
Your change (at version 2d47110) is now ready to be sponsored by a Committer.

Loading

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Feb 20, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

@DamonFool Only Committers are allowed to sponsor changes.

Loading

@nsjian
Copy link
Collaborator

@nsjian nsjian commented Feb 20, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

@nsjian @XiaohongGong Pushed as commit e901c4d.

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

Loading

@XiaohongGong XiaohongGong deleted the JDK-8261484 branch Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants