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

8277843: [Vector API] scalar2vector generates incorrect type info for mask operations if Op_MaskAll is unavailable #6562

Closed
wants to merge 2 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Nov 25, 2021

Hi all,

This bug was first observed on x86_32/AVX512.
It caused 62 vector api test failures.

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
>> jtreg:test/jdk/jdk/incubator/vector                  74    12    62     0 <<
==============================

You can easily reproduce this bug on an AVX512 machine with x86_32.
Or you can also reproduce it on an AVX512 machine with x86_64 if you disable Op_MaskAll like this.

diff --git a/src/hotspot/cpu/x86/x86.ad b/src/hotspot/cpu/x86/x86.ad
index 3f6d5a44b0d..d5a751b310d 100644
--- a/src/hotspot/cpu/x86/x86.ad
+++ b/src/hotspot/cpu/x86/x86.ad
@@ -1819,6 +1819,7 @@ const bool Matcher::match_rule_supported_vector(int opcode, int vlen, BasicType
       }
       break;
     case Op_MaskAll:
+      return false;
       if (!is_LP64 || !VM_Version::supports_evex()) {
         return false;
       }

The failure reason is that VectorNode::scalar2vector generate incorrect IR for mask operations if Op_MaskAll is unavailable.
So it shouldn't be used for mask operations if Op_MaskAll is unavailable.

Testing (with two more bug fixes #6535 and #6533):

  • vector api tests on {x86_64, x86_32}/{AVX512, AVX256}, all passed
  • vector api tests on aarch64, all passed

Thanks.
Best regards,
Jie


Progress

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

Issue

  • JDK-8277843: [Vector API] scalar2vector generates incorrect type info for mask operations if Op_MaskAll is unavailable

Reviewers

Contributors

  • Jatin Bhateja <jbhateja@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6562

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 25, 2021

👋 Welcome back jiefu! 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 openjdk bot added the rfr Pull request is ready for review label Nov 25, 2021
@openjdk
Copy link

openjdk bot commented Nov 25, 2021

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

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 25, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 25, 2021

Webrevs

@jatin-bhateja
Copy link
Member

jatin-bhateja commented Nov 26, 2021

Hi @DamonFool ,

MaskAll is only supported for targets having predicate registers, in all other cases a Replicate node should be generated.
Ideal type of Replicate node should be TypeVect, reported problem seems to be occurring because ideal type of Replicate Node is TypeVectMask.

I think following fix should be sufficient

diff --git a/src/hotspot/share/opto/vectornode.cpp b/src/hotspot/share/opto/vectornode.cpp
index 6a5b0b9b014..f6c483cf62c 100644
--- a/src/hotspot/share/opto/vectornode.cpp
+++ b/src/hotspot/share/opto/vectornode.cpp
@@ -588,13 +588,13 @@ VectorNode* VectorNode::make(int opc, Node* n1, Node* n2, Node* n3, uint vlen, B
 // Scalar promotion
 VectorNode* VectorNode::scalar2vector(Node* s, uint vlen, const Type* opd_t, bool is_mask) {
   BasicType bt = opd_t->array_element_basic_type();
-  const TypeVect* vt = opd_t->singleton() ? TypeVect::make(opd_t, vlen, is_mask)
-                                          : TypeVect::make(bt, vlen, is_mask);
-
   if (is_mask && Matcher::match_rule_supported_vector(Op_MaskAll, vlen, bt)) {
+    const TypeVect* vt = TypeVect::make(opd_t, vlen, is_mask);
     return new MaskAllNode(s, vt);
   }

+  const TypeVect* vt = opd_t->singleton() ? TypeVect::make(opd_t, vlen)
+                                          : TypeVect::make(bt, vlen);
   switch (bt) {
   case T_BOOLEAN:
   case T_BYTE:

Best Regards,
Jatin

@@ -837,6 +837,16 @@ bool LibraryCallKit::inline_vector_broadcast_coerced() {
return false; // not supported
}

// Op_MaskAll is required in VectorNode::scalar2vector for mask operations.
// So bail out if Op_MaskAll is unavailable.
if (is_vector_mask(vbox_klass) && !Matcher::match_rule_supported_vector(Op_MaskAll, num_elem, elem_bt)) {
Copy link
Member

@jatin-bhateja jatin-bhateja Nov 26, 2021

Choose a reason for hiding this comment

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

For targets which do not support MaskAll operation, Replicate node should be wrapped into a mask box.

@DamonFool DamonFool changed the title 8277843: [Vector API] scalar2vector shouldn't be used for mask operations if Op_MaskAll is unavailable 8277843: [Vector API] scalar2vector generates incorrect type info for mask operations if Op_MaskAll is unavailable Nov 26, 2021
@DamonFool
Copy link
Member Author

DamonFool commented Nov 26, 2021

/contributor add @jatin-bhateja

@openjdk
Copy link

openjdk bot commented Nov 26, 2021

@DamonFool
Contributor Jatin Bhateja <jbhateja@openjdk.org> successfully added.

@DamonFool
Copy link
Member Author

DamonFool commented Nov 26, 2021

MaskAll is only supported for targets having predicate registers, in all other cases a Replicate node should be generated.

Thanks @jatin-bhateja for your classification.
Fixed.
Thanks.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks reasonable to me.

@openjdk
Copy link

openjdk bot commented Nov 29, 2021

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

8277843: [Vector API] scalar2vector generates incorrect type info for mask operations if Op_MaskAll is unavailable

Co-authored-by: Jatin Bhateja <jbhateja@openjdk.org>
Reviewed-by: thartmann, 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 32 new commits pushed to the master branch:

  • 560f9c9: 8277426: Optimize mask reduction operations on x86
  • 3a4a94e: 8277854: The upper bound of GCCardSizeInBytes should be limited to 512 for 32-bit platforms
  • 825e633: 8277944: JDK 18 - update GA Release Date
  • 3d39f09: 8277654: Shenandoah: Don't produce new memory state in C2 LRB runtime call
  • 05ab176: 8277797: Remove undefined/unused SharedRuntime::trampoline_size()
  • ad51d06: 8277789: G1: G1CardSetConfiguration prefixes num_ and max_ used interchangeably
  • 614c6e6: 8277878: Fix compiler tests after JDK-8275908
  • 960bdde: 8277904: G1: Remove G1CardSetArray::max_entries
  • 45e8973: 8277896: Remove unused BOTConstants member methods
  • e5676f8: 8277450: Record number of references into collection set during gc
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/f788834998eeb9083e971857446321ed173aa916...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.

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 29, 2021
@DamonFool
Copy link
Member Author

DamonFool commented Nov 29, 2021

/reviewer credit @jatin-bhateja

@openjdk
Copy link

openjdk bot commented Nov 29, 2021

@DamonFool
Reviewer jbhateja successfully credited.

@DamonFool
Copy link
Member Author

DamonFool commented Nov 29, 2021

Looks reasonable to me.

Thanks @TobiHartmann for your review.

@jatin-bhateja , I had added you as one of the reviewers for this pr manually.
And will push it today if there is no other comments.
Thanks.

@jatin-bhateja
Copy link
Member

jatin-bhateja commented Nov 30, 2021

Looks reasonable to me.

Thanks @TobiHartmann for your review.

@jatin-bhateja , I had added you as one of the reviewers for this pr manually. And will push it today if there is no other comments. Thanks.

Thanks @DamonFool

@DamonFool
Copy link
Member Author

DamonFool commented Nov 30, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Nov 30, 2021

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

  • 3ee26c6: 8267767: Redundant condition check in SafepointSynchronize::thread_not_running
  • d230fee: 8277931: Parallel: Remove unused PSVirtualSpace::expand_into
  • fde6fe7: 8277824: Remove empty RefProcSubPhasesWorkerTimeTracker destructor
  • 27299ea: 8277803: vmTestbase/nsk/jdi/TypeComponent/isSynthetic/issynthetic001 fails with "Synthetic fields not found"
  • 560f9c9: 8277426: Optimize mask reduction operations on x86
  • 3a4a94e: 8277854: The upper bound of GCCardSizeInBytes should be limited to 512 for 32-bit platforms
  • 825e633: 8277944: JDK 18 - update GA Release Date
  • 3d39f09: 8277654: Shenandoah: Don't produce new memory state in C2 LRB runtime call
  • 05ab176: 8277797: Remove undefined/unused SharedRuntime::trampoline_size()
  • ad51d06: 8277789: G1: G1CardSetConfiguration prefixes num_ and max_ used interchangeably
  • ... and 26 more: https://git.openjdk.java.net/jdk/compare/f788834998eeb9083e971857446321ed173aa916...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 30, 2021
@openjdk 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 labels Nov 30, 2021
@openjdk
Copy link

openjdk bot commented Nov 30, 2021

@DamonFool Pushed as commit ceae380.

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

@DamonFool DamonFool deleted the JDK-8277843 branch Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
3 participants