8370794: C2 SuperWord: Long/Integer.compareUnsigned return wrong value for EQ/NE in SLP

[Hamlin-Li]

Hi,
Can you help to review this patch?

JDK-8370481 introduces this regression for unsigned I/L EQ/NE in SLP.

====================

In JDK-8370481, we fixed an issue related to transformation from (Bool + CmpU + CMove) to (VectorMaskCmp + VectorBlend), and added tests for unsigned ones.
As discussion in [1], we should also add more tests for transformation from (Bool + Cmp + CMove) to (VectorMaskCmp + VectorBlend) for the signed ones.

[1] #27942 (comment)

Thanks!

Tests running...

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8370794: C2 SuperWord: Long/Integer.compareUnsigned return wrong value for EQ/NE in SLP (Bug - P2)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28047/head:pull/28047
$ git checkout pull/28047

Update a local copy of the PR:
$ git checkout pull/28047
$ git pull https://git.openjdk.org/jdk.git pull/28047/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28047

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28047.diff

Using Webrev

Link to Webrev Comment

[Hamlin-Li]

@eme64 When I develop this test pr, found out that, after #27942 there could be some failing scenarios for unsigned EQ/NE (found by the newly developed unsigned EQ/NE test). We could fix it with the following patch, I can put the fix patch and related EQ/NE signed/unsigned tests in this pr, but seems it's better to put them in another separate pr? Please let me know what do you think. Thanks

diff --git a/src/hotspot/share/opto/subnode.cpp b/src/hotspot/share/opto/subnode.cpp
index 9c6c7498dd0..a10cd2a8d5b 100644
--- a/src/hotspot/share/opto/subnode.cpp
+++ b/src/hotspot/share/opto/subnode.cpp
@@ -1398,6 +1398,21 @@ const Type *BoolTest::cc2logical( const Type *CC ) const {
   return TypeInt::BOOL;
 }
 
+BoolTest::mask BoolTest::unsigned_mask(BoolTest::mask btm) {
+  switch(btm) {
+    case eq:
+    case ne:
+      return btm;
+    case lt:
+    case le:
+    case gt:
+    case ge:
+      return mask(btm | unsigned_compare);
+    default:
+      ShouldNotReachHere();
+  }
+}
+
 //------------------------------dump_spec-------------------------------------
 // Print special per-node info
 void BoolTest::dump_on(outputStream *st) const {
diff --git a/src/hotspot/share/opto/subnode.hpp b/src/hotspot/share/opto/subnode.hpp
index 2c3d9cfd35e..463d9e020cb 100644
--- a/src/hotspot/share/opto/subnode.hpp
+++ b/src/hotspot/share/opto/subnode.hpp
@@ -331,7 +331,7 @@ struct BoolTest {
   mask negate( ) const { return negate_mask(_test); }
   // Return the negative mask for the given mask, for both signed and unsigned comparison.
   static mask negate_mask(mask btm) { return mask(btm ^ 4); }
-  static mask unsigned_mask(mask btm) { return mask(btm | unsigned_compare); }
+  static mask unsigned_mask(mask btm);
   bool is_canonical( ) const { return (_test == BoolTest::ne || _test == BoolTest::lt || _test == BoolTest::le || _test == BoolTest::overflow); }
   bool is_less( )  const { return _test == BoolTest::lt || _test == BoolTest::le; }
   bool is_greater( ) const { return _test == BoolTest::gt || _test == BoolTest::ge; }

[bridgekeeper]

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

[eme64]

@Hamlin-Li By failing you mean they now produce a wrong result? If yes, maybe we can convert this issue here to a bugfix? And just add all the additional tests to ensure we don't have any other bugs.

Is it just a regression from the last patch, or an older issue? Probably just a regression from #27942, because before we just did nothing with the mask, and that is what you want to go back to, right?

BTW: thanks for writing all the tests, it seems to be the only way to ensure we get it all right :)

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[Hamlin-Li]

@Hamlin-Li By failing you mean they now produce a wrong result? If yes, maybe we can convert this issue here to a bugfix? And just add all the additional tests to ensure we don't have any other bugs.

Is it just a regression from the last patch, or an older issue? Probably just a regression from #27942, because before we just did nothing with the mask, and that is what you want to go back to, right?

Yes, it's a regression from #27942.
I'll update the bug and pr, also add fix and tests for unsigned EQ/NE.

BTW: thanks for writing all the tests, it seems to be the only way to ensure we get it all right :)

Thanks for the quick response! :)

[openjdk]

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

[mlbridge]

Webrevs

• 00: Full (696ae0d7)

[eme64]

That looks really good, thanks for writing all the tests, that's amazing :)

I'll run some internal tests now...