8332119: Incorrect IllegalArgumentException for C2 compiled permute kernel #19442

jatin-bhateja · 2024-05-29T06:10:53Z

Currently inline expansion of vector to shuffle conversion simply type casts the vector holding indexes to byte vector[1] where as fallback implementation[2] also wraps the indexes to a valid index range [0, VEC_LEN-1) or generates a -ve index for exceptional / OOB indices.

This patch extends the conversion inline expander to match the fall back implementation. This imposes around 20% performance tax on Vector.toShuffle() intrinsic but fixes this functional bug.

Kindly review and share your feedback.

Best Regards,
Jatin

PS: Patch also fixes an incorrectness issue reported with JDK-8332118

[1] https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/FloatVector.java#L2352
[2] https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractShuffle.java#L58

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-8332119: Incorrect IllegalArgumentException for C2 compiled permute kernel (Bug - P3)

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to 102b78ae
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 16996e57

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19442

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

Using diff file

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

Webrev

Link to Webrev Comment

…ernel

bridgekeeper · 2024-05-29T06:11:23Z

👋 Welcome back jbhateja! 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 · 2024-05-29T06:12:19Z

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

8332119: Incorrect IllegalArgumentException for C2 compiled permute kernel

Reviewed-by: sviswanathan, kvn

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

f7dbb98: 8333086: Using Console.println is unnecessarily slow due to JLine initalization
9b3694c: 8319822: Use a linear-time algorithm for assert_different_registers()
f73922b: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
5dcb7a6: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F
438121b: 8332785: Replace naked uses of UseSharedSpaces with CDSConfig::is_using_archive
d7d1afb: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
7acfba2: 8327650: Test java/nio/channels/DatagramChannel/StressNativeSignal.java timed out
c5c0867: 8333252: C2: assert(assertion_predicate_has_loop_opaque_node(iff)) failed: must find OpaqueLoop* nodes
d85b0ca: 8332457: Examine startup overheads from JDK-8294961
326dbb1: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
... and 131 more: https://git.openjdk.org/jdk/compare/da6aa2a86c86ba5fce747b36dcb2d6001cfcc44e...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 · 2024-05-29T06:12:42Z

@jatin-bhateja 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 · 2024-05-29T06:15:20Z

Webrevs

sviswa7 · 2024-05-31T21:01:35Z

src/hotspot/share/opto/vectorIntrinsics.cpp

+  if (is_vector_shuffle(vbox_klass_to)) {
+     op = wrap_indexes(op, num_elem_to, elem_bt_to);
+  }
+


The wrap_indexes is needed only for two vector rearrange. It looks to me that doing a wrap_indexes here at convert would force it for single vector rearrange (or selectFrom) and thereby reduce the performance for that case as well. Please note that the single vector rearrange throws "IndexOutOfBoundsException" and doesn't need to do a wrap.

Please ignore the above comment. I verified that each index is partially wrapped as part of toShuffle(). We should name the wrap_indexes() to partially_wrap_indexes() for clarity.

sviswa7

Other than these two minor comments, the PR looks good to me.

sviswa7 · 2024-05-31T23:38:35Z

test/hotspot/jtreg/compiler/vectorapi/TestTwoVectorPermute.java

+       for (int i = 0; i < res.length; i++) {
+           float expected = Float.NaN;
+           // Exceptional index.
+           if (shuf[i] < 0 || shuf[i] >= FSP.length()) {


To better match the specs, this could be:
if ( (int)shuf[i] < 0 || (int)shuf[i] >= FSP.length()) {

sviswa7

Looks good to me.

jatin-bhateja · 2024-06-03T17:21:21Z

Hi @vnkozlov / @TobiHartmann / @iwanowww / @eme64 , please let me know if it's good for integration.

jatin-bhateja · 2024-06-04T16:38:42Z

Hi @TobiHartmann , @vnkozlov , please let me know if it's good to integrate.

vnkozlov · 2024-06-04T16:49:40Z

Please, wait our review and testing.

vnkozlov

I have some comments.

vnkozlov · 2024-06-04T17:44:49Z

src/hotspot/share/opto/vectorIntrinsics.cpp

+  const TypeVect * vt  = TypeVect::make(elem_bt, num_elem);
+  const Type * type_bt = Type::get_const_basic_type(elem_bt);


Please remove space between a type and *.

vnkozlov · 2024-06-04T17:48:40Z

src/hotspot/share/opto/vectorIntrinsics.cpp

  return true;
 }

+Node* LibraryCallKit::partially_wrap_indexes(Node* index_vec, int num_elem, BasicType elem_bt) {


Can you add comment with pseudo code to show what this method do?

vnkozlov · 2024-06-04T17:51:26Z

src/hotspot/share/opto/vectorIntrinsics.cpp

+       !arch_supports_vector(Op_VectorMaskCmp, num_elem_to, elem_bt_to, VecMaskNotUsed)   ||
+       !arch_supports_vector(Op_AndV, num_elem_to, elem_bt_to, VecMaskNotUsed)            ||
+       !arch_supports_vector(Op_Replicate, num_elem_to, elem_bt_to, VecMaskNotUsed))) {
+    return false;


Please add log_if_needed(" here too.

vnkozlov · 2024-06-04T17:52:30Z

src/hotspot/share/opto/vectorIntrinsics.cpp

-    res = gvn().transform(VectorNode::make(Op_AndV, res, bcast_mod, vt));
-    Node * biased_val = gvn().transform(VectorNode::make(Op_SubVB, res, bcast_lane_cnt, vt));
-    res = gvn().transform(new VectorBlendNode(biased_val, res, mask));
+     res = partially_wrap_indexes(res, num_elem, elem_bt);


Original spacing here was correct. One at the line 621 is wrong and have to be fixed.

eme64 · 2024-06-05T06:24:25Z

src/hotspot/share/opto/vectorIntrinsics.cpp

+// Note: Unsigned greater than comparison treat both <0 and >VEC_LENGTH indices as out-of-bound
+// indexes.
+Node* LibraryCallKit::partially_wrap_indexes(Node* index_vec, int num_elem, BasicType elem_bt) {
+  assert(elem_bt == T_BYTE, "");


Write message in assert: why is it limited to byte?

eme64 · 2024-06-05T06:25:41Z

src/hotspot/share/opto/vectorIntrinsics.cpp

+  const Type* type_bt = Type::get_const_basic_type(elem_bt);
+
+  Node* mod_val = gvn().makecon(TypeInt::make(num_elem-1));
+  Node* bcast_mod  = gvn().transform(VectorNode::scalar2vector(mod_val, num_elem, type_bt));


Naming issue: this is not the result of the mod, so "mod" is a bit misleading. I would use mask, as it is used as a mask in the AndV below.

Also: it seems to me that you are duplicating these 4 lines above from its call-site. I wonder if this means that you are slicing the boundary of your new method right, or if maybe the whole if-else block from the call-site should be a new method?

Also: it seems to me that you are duplicating these 4 lines above from its call-site. I wonder if this means that you are slicing the boundary of your new method right, or if maybe the whole if-else block from the call-site should be a new method?

The duplication you are pointing in code may not translate into IR since GVN implicitly promotes sharing based on nodes hash value which is a function of node's opcode and inputs.

eme64 · 2024-06-05T06:36:27Z

test/hotspot/jtreg/compiler/vectorapi/TestTwoVectorPermute.java

+* @bug 8332119
+* @summary Incorrect IllegalArgumentException for C2 compiled permute kernel
+* @modules jdk.incubator.vector
+* @requires vm.compiler2.enabled


Is this necessary to restrict to C2? Maybe this test tickles something for other compilers as well.

eme64 · 2024-06-05T06:37:21Z

test/hotspot/jtreg/compiler/vectorapi/TestTwoVectorPermute.java

+* @requires vm.compiler2.enabled
+* @library /test/lib /
+* @run main/othervm -XX:+UnlockDiagnosticVMOptions -Xbatch -XX:-TieredCompilation -XX:CompileOnly=TestTwoVectorPermute::micro compiler.vectorapi.TestTwoVectorPermute
+* @run main/othervm -XX:+UnlockDiagnosticVMOptions -Xbatch -XX:-TieredCompilation compiler.vectorapi.TestTwoVectorPermute


I would also add a run without -XX:-TieredCompilation, that could lead to different compilation patterns, and increase our test coverage.

eme64 · 2024-06-05T06:38:11Z

test/hotspot/jtreg/compiler/vectorapi/TestTwoVectorPermute.java

+public class TestTwoVectorPermute {
+   public static final VectorSpecies<Float> FSP = FloatVector.SPECIES_256;
+
+   public static void validate(float [] res, float [] shuf, float [] src1, float [] src2) {


Suggested change

public static void validate(float [] res, float [] shuf, float [] src1, float [] src2) {

public static void validate(float[] res, float[] shuf, float[] src1, float[] src2) {

Similar issues below.

vnkozlov

Good. Tobias ran testing for v01 and it passed.

vnkozlov · 2024-06-05T16:12:15Z

Please, answer Emanuel's questions/suggestions before integration.

jatin-bhateja · 2024-06-05T18:36:18Z

Thanks @sviswa7 , @vnkozlov , @eme64 , your comments have been addressed. Integrating the patch.

jatin-bhateja · 2024-06-05T18:36:30Z

/integrate

openjdk · 2024-06-05T18:36:51Z

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

f7dbb98: 8333086: Using Console.println is unnecessarily slow due to JLine initalization
9b3694c: 8319822: Use a linear-time algorithm for assert_different_registers()
f73922b: 8333235: vmTestbase/nsk/jdb/kill/kill001/kill001.java fails with C1
5dcb7a6: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F
438121b: 8332785: Replace naked uses of UseSharedSpaces with CDSConfig::is_using_archive
d7d1afb: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
7acfba2: 8327650: Test java/nio/channels/DatagramChannel/StressNativeSignal.java timed out
c5c0867: 8333252: C2: assert(assertion_predicate_has_loop_opaque_node(iff)) failed: must find OpaqueLoop* nodes
d85b0ca: 8332457: Examine startup overheads from JDK-8294961
326dbb1: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
... and 131 more: https://git.openjdk.org/jdk/compare/da6aa2a86c86ba5fce747b36dcb2d6001cfcc44e...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-05T18:36:56Z

@jatin-bhateja Pushed as commit 4c09d9f.

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

8332119: Incorrect IllegalArgumentException for C2 compiled permute k…

12cc2b4

…ernel

openjdk bot added the rfr Pull request is ready for review label May 29, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 29, 2024

sviswa7 reviewed May 31, 2024

View reviewed changes

Review Comments Incorporated.

102b78a

sviswa7 approved these changes Jun 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2024

vnkozlov reviewed Jun 4, 2024

View reviewed changes

Review comments resolutions.

16996e5

eme64 suggested changes Jun 5, 2024

View reviewed changes

vnkozlov approved these changes Jun 5, 2024

View reviewed changes

Review comments addressed.

cb2877b

openjdk bot added the integrated Pull request has been integrated label Jun 5, 2024

openjdk bot closed this Jun 5, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 5, 2024

jatin-bhateja deleted the JDK-8332119 branch August 1, 2024 08:52

		const TypeVect * vt = TypeVect::make(elem_bt, num_elem);
		const Type * type_bt = Type::get_const_basic_type(elem_bt);

	public static void validate(float [] res, float [] shuf, float [] src1, float [] src2) {
	public static void validate(float[] res, float[] shuf, float[] src1, float[] src2) {

8332119: Incorrect IllegalArgumentException for C2 compiled permute kernel #19442

8332119: Incorrect IllegalArgumentException for C2 compiled permute kernel #19442

Uh oh!

Conversation

jatin-bhateja commented May 29, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 29, 2024

Uh oh!

openjdk bot commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 29, 2024

Uh oh!

mlbridge bot commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jatin-bhateja commented Jun 4, 2024

Uh oh!

vnkozlov commented Jun 4, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jun 5, 2024

Uh oh!

jatin-bhateja commented Jun 5, 2024

Uh oh!

jatin-bhateja commented Jun 5, 2024

Uh oh!

openjdk bot commented Jun 5, 2024

Uh oh!

jatin-bhateja commented May 29, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 29, 2024 •

edited

Loading

mlbridge bot commented May 29, 2024 •

edited

Loading

jatin-bhateja commented Jun 3, 2024 •

edited

Loading