8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant #19918

vnkozlov · 2024-06-27T02:15:20Z

Problems were found in some C2 intrinsics while testing Leyden Early Release.

inline_array_partition() and inline_array_sort() intrinsics incorrectly assume that elementType argument is always compile-time constant. Which is not true for Leyden (and in general) when constant folding optimization is switched off (or did not executed for particular type).

Note, other intrinsics are very careful about that. For example, see inline_Class_cast() intrinsic.

Add similar checks to inline_array_partition() and inline_array_sort().

An other problem is that null_check() (which adds new control nodes to graph) was executed in these intrinsics before bailouts from intrinsic generation. We hit next assert if bailout happens:

  assert(ctrl == kit.control(), "Control flow was added although the intrinsic bailed out");

I moved null_check() after bailouts.

An other issue with null_check() is it checks for null first argument which is Class. It can't be null because corresponding Java methods are private and each call site passed <primitive_type>.class as first argument.
For example: DualPivotQuicksort.java#L260
On other hand there is no null check for passed array (second argument). I changed null_check() for array.

These 2 intrinsics were added by JDK-8309130 in JDK 22.

Testing tier1-6, hs-stress, hs-xcomp

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-8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant (Bug - P3)

Reviewers

Roland Westrelin (@rwestrel - Reviewer) ⚠️ Review applies to c370b07a
Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to 8ab7034a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19918

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

Using diff file

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

Webrev

Link to Webrev Comment

…compile-time constant

bridgekeeper · 2024-06-27T02:16:02Z

👋 Welcome back kvn! 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-06-27T02:16:55Z

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

8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant

Reviewed-by: roland, chagedorn

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

45c4eaa: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
486aa11: 8335237: ubsan: vtableStubs.hpp is_vtable_stub exclude from ubsan checks
f4d8c00: 8334562: Automate com/sun/security/auth/callback/TextCallbackHandler/Default.java test
49eb00d: 8299813: java/nio/channels/DatagramChannel/Disconnect.java fails with jtreg test timeout due to lost datagram
8ec378a: 8277949: (dc) java/nio/channels/DatagramChannel/AdaptorBasic.java failed in timeout
c798316: 8269657: Test java/nio/channels/DatagramChannel/Loopback.java failed: Unexpected message
99d2bbf: 8334433: jshell.exe runs an executable test.exe on startup
6f4ddc2: 8335142: compiler/c1/TestTraceLinearScanLevel.java occasionally times out with -Xcomp
3b3a19e: 8335314: Problem list compiler/uncommontrap/DeoptReallocFailure.java
d457609: 8319947: Recursive lightweight locking: s390x implementation
... and 26 more: https://git.openjdk.org/jdk/compare/4ebb77120af5a4ccbfde63b24cb50e05a3161f16...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-06-27T02:17:26Z

@vnkozlov 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-06-27T02:20:14Z

Webrevs

rwestrel

Looks good to me.

chhagedorn

Only some style comments, otherwise, it looks good to me, too.

Were you able to reproduce this in mainline as well?

chhagedorn · 2024-06-27T09:21:16Z

src/hotspot/share/opto/library_call.cpp

+  address stubAddr = nullptr;
+  stubAddr = StubRoutines::select_array_partition_function();


Can be merged:

Suggested change

address stubAddr = nullptr;

stubAddr = StubRoutines::select_array_partition_function();

address stubAddr = StubRoutines::select_array_partition_function();

chhagedorn · 2024-06-27T09:21:47Z

src/hotspot/share/opto/library_call.cpp

+
+  null_check(obj);
+  // If obj is dead, only null-path is taken.
+  if (stopped())  return true;


Can you add braces here?

Suggested change

if (stopped()) return true;

if (stopped()) {

return true;

}

chhagedorn · 2024-06-27T09:25:31Z

src/hotspot/share/opto/library_call.cpp

+  const TypeInstPtr* elem_klass = gvn().type(elementType)->isa_instptr();
+  if (elem_klass == nullptr) {
+    return false;  // dead path (elementType->is_top()).
+  }
+  if (obj == nullptr || obj->is_top()) {
+    return false;  // dead path
+  }
+  // java_mirror_type() returns non-null for compile-time Class constants only.
+  ciType* elem_type = elem_klass->java_mirror_type();
+  if (elem_type == nullptr) {
+    return false;
+  }
+  BasicType bt = elem_type->basic_type();
+  // Disable the intrinsic if the CPU does not support SIMD sort
+  if (!Matcher::supports_simd_sort(bt)) {
+    return false;
+  }
+  address stubAddr = nullptr;
+  stubAddr = StubRoutines::select_array_partition_function();
+  // stub not loaded
+  if (stubAddr == nullptr) {
+    return false;
+  }
+  // get the address of the array
+  const TypeAryPtr* obj_t = _gvn.type(obj)->isa_aryptr();
+  if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM ) {
+    return false; // failed input validation
+  }


The bailout code looks almost identical to the one in inline_array_sort(). Can it somehow be shared?

Good idea. I will try.

chhagedorn · 2024-06-27T09:27:07Z

src/hotspot/share/opto/library_call.cpp

+
+  null_check(obj);
+  // If obj is dead, only null-path is taken.
+  if (stopped())  return true;


Suggested change

if (stopped()) return true;

if (stopped()) {

return true;

}

chhagedorn · 2024-06-27T09:28:13Z

src/hotspot/share/opto/library_call.cpp

+  }
+  // get the address of the array
+  const TypeAryPtr* obj_t = _gvn.type(obj)->isa_aryptr();
+  if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM ) {


Suggested change

if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM ) {

if (obj_t == nullptr || obj_t->elem() == Type::BOTTOM) {

vnkozlov · 2024-06-27T23:28:32Z

Moved common checks into new check_array_sort_arguments() method.
Moved intrinsic's stub presence check to the beginning of methods and added assert to check signature size (number of stack slots) similar to other intrinsics.
I kept null_check() together with stopped() check for clarity.

Tested locally java/util/Arrays/Sorting.java on AVX512 machine to make sure intrinsics are generated and working.
Running tier1-3,stress,xcomp again for this update.

chhagedorn · 2024-06-28T09:21:25Z

src/hotspot/share/opto/library_call.cpp

-
+// Common checks for array sorting intrinsics arguments.
+// Returns `true` if checks passed.
+bool LibraryCallKit::check_array_sort_arguments(Node* elementType, Node* obj, BasicType* bt) {


Thanks for doing the update, looks good! Just one small detail: You could use a reference here instead of a pointer. Then you can pass bt in and modify it directly.

vnkozlov · 2024-06-28T19:34:26Z

/integrate

openjdk · 2024-06-28T19:36:01Z

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

3e23e9c: 8335344: test/jdk/sun/security/tools/keytool/NssTest.java fails to compile
79a3554: 8335124: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java failed with CPU time out of expected range
45c4eaa: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
486aa11: 8335237: ubsan: vtableStubs.hpp is_vtable_stub exclude from ubsan checks
f4d8c00: 8334562: Automate com/sun/security/auth/callback/TextCallbackHandler/Default.java test
49eb00d: 8299813: java/nio/channels/DatagramChannel/Disconnect.java fails with jtreg test timeout due to lost datagram
8ec378a: 8277949: (dc) java/nio/channels/DatagramChannel/AdaptorBasic.java failed in timeout
c798316: 8269657: Test java/nio/channels/DatagramChannel/Loopback.java failed: Unexpected message
99d2bbf: 8334433: jshell.exe runs an executable test.exe on startup
6f4ddc2: 8335142: compiler/c1/TestTraceLinearScanLevel.java occasionally times out with -Xcomp
... and 28 more: https://git.openjdk.org/jdk/compare/4ebb77120af5a4ccbfde63b24cb50e05a3161f16...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-28T19:36:07Z

@vnkozlov Pushed as commit 166f9d9.

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

8335221: Some C2 intrinsics incorrectly assume that type argument is …

c370b07

…compile-time constant

openjdk bot added the rfr Pull request is ready for review label Jun 27, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 27, 2024

rwestrel approved these changes Jun 27, 2024

View reviewed changes

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

chhagedorn approved these changes Jun 27, 2024

View reviewed changes

Move common checks into shared method

8ab7034

chhagedorn approved these changes Jun 28, 2024

View reviewed changes

Pass bt argument as reference instead of pointer

a35b3ee

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

openjdk bot closed this Jun 28, 2024

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

vnkozlov deleted the 8335221 branch June 28, 2024 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant #19918

8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant #19918

vnkozlov commented Jun 27, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 27, 2024

openjdk bot commented Jun 27, 2024 •

edited

Loading

openjdk bot commented Jun 27, 2024

mlbridge bot commented Jun 27, 2024 •

edited

Loading

rwestrel left a comment

chhagedorn left a comment

chhagedorn Jun 27, 2024

vnkozlov Jun 27, 2024

chhagedorn Jun 27, 2024

vnkozlov Jun 27, 2024

chhagedorn Jun 27, 2024

vnkozlov Jun 27, 2024

chhagedorn Jun 27, 2024

vnkozlov Jun 27, 2024

chhagedorn Jun 27, 2024

vnkozlov Jun 27, 2024

vnkozlov commented Jun 27, 2024

chhagedorn Jun 28, 2024

vnkozlov Jun 28, 2024

vnkozlov commented Jun 28, 2024

openjdk bot commented Jun 28, 2024

openjdk bot commented Jun 28, 2024

		address stubAddr = nullptr;
		stubAddr = StubRoutines::select_array_partition_function();

	address stubAddr = nullptr;
	stubAddr = StubRoutines::select_array_partition_function();
	address stubAddr = StubRoutines::select_array_partition_function();

	if (obj_t == nullptr \|\| obj_t->elem() == Type::BOTTOM ) {
	if (obj_t == nullptr \|\| obj_t->elem() == Type::BOTTOM) {

8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant #19918

8335221: Some C2 intrinsics incorrectly assume that type argument is compile-time constant #19918

Conversation

vnkozlov commented Jun 27, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 27, 2024

openjdk bot commented Jun 27, 2024 • edited Loading

openjdk bot commented Jun 27, 2024

mlbridge bot commented Jun 27, 2024 • edited Loading

Webrevs

rwestrel left a comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov commented Jun 28, 2024

openjdk bot commented Jun 28, 2024

openjdk bot commented Jun 28, 2024

vnkozlov commented Jun 27, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 27, 2024 •

edited

Loading

mlbridge bot commented Jun 27, 2024 •

edited

Loading