8305189: C2 failed "assert(_outcnt==1) failed: not unique" #13901

rwestrel · 2023-05-10T11:27:06Z

pre/main/post loops are created for an inner loop of a loop nest but
assert predicates cause the main and post loops to be removed. The
OpaqueZeroTripGuard nodes for the loops are not removed: there's no
logic to trigger removal of the opaque nodes once the loops are no
longer there. With the inner loops gone, the outer loop becomes
candidate for optimizations and is unrolled which causes the zero trip
guards of the now removed loops to be duplicated and the opaque nodes
to have more than one use.

The fix I propose is, using logic similar to
PhaseIdealLoop::eliminate_useless_predicates(), to check during loop
opts if every OpaqueZeroTripGuard node guards a loop and if not,
remove it.

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-8305189: C2 failed "assert(_outcnt==1) failed: not unique"

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13901

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-10T11:27:54Z

👋 Welcome back roland! 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 · 2023-05-10T11:29:34Z

@rwestrel 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 · 2023-05-10T11:33:28Z

Webrevs

chhagedorn

That looks reasonable.

chhagedorn · 2023-05-10T15:37:18Z

src/hotspot/share/opto/opaquenode.cpp

@@ -54,6 +55,37 @@ Node* Opaque3Node::Identity(PhaseGVN* phase) {
  return this;
 }

+CountedLoopNode* OpaqueZeroTripGuardNode::guarded_loop() const {


Could be guarded with ifdef ASSERT since you are only using it for an assertion.

I made that change.

chhagedorn · 2023-05-10T16:09:13Z

src/hotspot/share/opto/loopnode.cpp

@@ -6145,6 +6175,11 @@ void PhaseIdealLoop::build_loop_late_post_work(Node *n, bool pinned) {
  IdealLoopTree *chosen_loop = get_loop(least);
  if( !chosen_loop->_child )   // Inner loop?
    chosen_loop->_body.push(n);// Collect inner loops
+
+  if (!_verify_only && n->Opcode() == Op_OpaqueZeroTripGuard) {
+    _zero_trip_guard_opaque_nodes.push(n);


That's a good idea to collect them newly here for each loop opts pass.

I'm not sure if you're expecting me to comment on this or not.

No action required. I was first trying to suggest to move it to Compile to the other predicate opaque node lists but then I've thought that this solution here is cleaner. So it was more of a 👍 (I will refactor these predicate lists in the assertion predicate changes - otherwise, I would have suggested to move them here as well at some point).

Thanks for the clarification.

openjdk · 2023-05-10T16:14:37Z

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

8305189: C2 failed "assert(_outcnt==1) failed: not unique"

Reviewed-by: chagedorn, thartmann

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

f09345b: 8308931: Problemlist compiler/jvmci/TestUncaughtErrorInCompileMethod.java
7c072db: 8308844: ProblemList gc/z/TestHighUsage.java with Generational ZGC on windows x64
4becb7b: 8306137: Open source several AWT ScrollPane related tests
199b1bf: 8308583: SIGSEGV in GraphKit::gen_checkcast
46c4da7: 8159023: Engineering notation of DecimalFormat does not work as documented
ee321c7: 8308907: ProblemList java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java on linux-x64
dc7683a: 8308073: ClassLoaderExt::append_boot_classpath should handle dynamic archive
4870234: 8304375: jdk/jfr/api/consumer/filestream/TestOrdered.java failed with "Expected at least some events to be out of order! Reuse = false"
7d2a7ce: 8308672: Add version number in the replay file generated by DumpInline
27ba8bd: 8308108: Support Unicode extension for collation settings
... and 84 more: https://git.openjdk.org/jdk/compare/05e99db466e7ef5c26f089db772a21cb2ca62e93...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.

chhagedorn

Update looks good!

rwestrel · 2023-05-11T08:11:28Z

That looks reasonable.

Thanks for reviewing this @chhagedorn

TobiHartmann · 2023-05-15T13:39:12Z

I see the following failure with TestMissingMulLOptimization from JDK-8299546 and -XX:StressLongCountedLoop=2000000:

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (c:\sb\prod\1684151031\workspace\open\src\hotspot\share\opto\loopnode.cpp:4157), pid=5368, tid=836
#  Error: assert(loop == nullptr) failed

Current CompileTask:
C2:    267   15    b  4       compiler.ccp.TestMissingMulLOptimization::test (101 bytes)

Stack: [0x0000002f4f600000,0x0000002f4f700000]
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [jvm.dll+0xc53091]  os::win32::platform_print_native_stack+0xf1  (os_windows_x86.cpp:236)
V  [jvm.dll+0xee2a99]  VMError::report+0x1019  (vmError.cpp:815)
V  [jvm.dll+0xee4775]  VMError::report_and_die+0x645  (vmError.cpp:1596)
V  [jvm.dll+0xee4e84]  VMError::report_and_die+0x64  (vmError.cpp:1361)
V  [jvm.dll+0x55053b]  report_vm_error+0x5b  (debug.cpp:191)
V  [jvm.dll+0xadcab2]  PhaseIdealLoop::eliminate_useless_zero_trip_guard+0x2f2  (loopnode.cpp:4157)
V  [jvm.dll+0xad0fb1]  PhaseIdealLoop::build_and_optimize+0x971  (loopnode.cpp:4455)
V  [jvm.dll+0x4ebc51]  Compile::optimize_loops+0x1d1  (compile.cpp:2155)
V  [jvm.dll+0x4de2e8]  Compile::Optimize+0xef8  (compile.cpp:2391)
V  [jvm.dll+0x4db378]  Compile::Compile+0x1458  (compile.cpp:840)
V  [jvm.dll+0x3f05ba]  C2Compiler::compile_method+0x11a  (c2compiler.cpp:121)
V  [jvm.dll+0x4f6a81]  CompileBroker::invoke_compiler_on_method+0x881  (compileBroker.cpp:2268)
V  [jvm.dll+0x4f3ea6]  CompileBroker::compiler_thread_loop+0x396  (compileBroker.cpp:1945)
V  [jvm.dll+0x7f2ff9]  JavaThread::thread_main_inner+0x279  (javaThread.cpp:720)
V  [jvm.dll+0xe5434d]  Thread::call_run+0x1cd  (thread.cpp:222)
V  [jvm.dll+0xc519c2]  os::win32::thread_native_entry+0xa2  (os_windows.cpp:551)

rwestrel · 2023-05-22T15:30:47Z

I see the following failure with TestMissingMulLOptimization from JDK-8299546 and -XX:StressLongCountedLoop=2000000:

What happens here is that a counted loop ends up in an infinite loop. So the IdealLoopTree tree object is unreachable from the root loop (for this round of loop opts as a NeverBranch is added so the IdealLoopTree should become reachable again at next pass of loop opts). So the new logic removes an opaque node for a loop that still exists. I suppose it's rare and mostly harmless and I tweaked the assert to cover that corner case.

chhagedorn · 2023-05-23T06:33:37Z

Would it still be required/useful to keep an OpaqueZeroTripGuardNode for such a counted loop inside an infinite loop?

rwestrel · 2023-05-23T09:23:41Z

Would it still be required/useful to keep an OpaqueZeroTripGuardNode for such a counted loop inside an infinite loop?

You're right that it's probably safer to be conservative and rather than remove the opaque node anyway, keep it if we can't tell if it's useful or not. I updated the change with a new commit that does that.

chhagedorn

I agree that it is better to be safe here - update looks good!

I'll resubmit some testing.

src/hotspot/share/opto/loopnode.cpp

chhagedorn

I agree that it is better to be safe here - update looks good!

I'll resubmit some testing.

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

chhagedorn · 2023-05-24T11:14:21Z

All testing passed!

rwestrel · 2023-05-24T11:33:35Z

All testing passed!

Thanks for the review and testing!

TobiHartmann

Looks reasonable to me.

rwestrel · 2023-05-26T09:03:31Z

@TobiHartmann thanks for the review

rwestrel · 2023-05-26T09:03:39Z

/integrate

openjdk · 2023-05-26T09:04:43Z

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

62537d2: 8299974: Replace NULL with nullptr in share/adlc/
f09345b: 8308931: Problemlist compiler/jvmci/TestUncaughtErrorInCompileMethod.java
7c072db: 8308844: ProblemList gc/z/TestHighUsage.java with Generational ZGC on windows x64
4becb7b: 8306137: Open source several AWT ScrollPane related tests
199b1bf: 8308583: SIGSEGV in GraphKit::gen_checkcast
46c4da7: 8159023: Engineering notation of DecimalFormat does not work as documented
ee321c7: 8308907: ProblemList java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java on linux-x64
dc7683a: 8308073: ClassLoaderExt::append_boot_classpath should handle dynamic archive
4870234: 8304375: jdk/jfr/api/consumer/filestream/TestOrdered.java failed with "Expected at least some events to be out of order! Reuse = false"
7d2a7ce: 8308672: Add version number in the replay file generated by DumpInline
... and 85 more: https://git.openjdk.org/jdk/compare/05e99db466e7ef5c26f089db772a21cb2ca62e93...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-26T09:06:14Z

@rwestrel Pushed as commit bac02b6.

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

fix & test

49b2a2f

openjdk bot added the rfr Pull request is ready for review label May 10, 2023

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 10, 2023

chhagedorn approved these changes May 10, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 10, 2023

review

d360f92

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels May 11, 2023

chhagedorn approved these changes May 11, 2023

View reviewed changes

rwestrel added 2 commits May 22, 2023 13:10

Merge branch 'master' into JDK-8305189

849610b

test failure

8f37817

review

426b2a4

chhagedorn approved these changes May 23, 2023

View reviewed changes

src/hotspot/share/opto/loopnode.cpp Outdated Show resolved Hide resolved

chhagedorn approved these changes May 23, 2023

View reviewed changes

Update src/hotspot/share/opto/loopnode.cpp

5e26e81

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

chhagedorn approved these changes May 24, 2023

View reviewed changes

TobiHartmann approved these changes May 26, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 26, 2023

openjdk bot closed this May 26, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8305189: C2 failed "assert(_outcnt==1) failed: not unique" #13901

8305189: C2 failed "assert(_outcnt==1) failed: not unique" #13901

rwestrel commented May 10, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 10, 2023

openjdk bot commented May 10, 2023

mlbridge bot commented May 10, 2023 •

edited

Loading

chhagedorn left a comment

chhagedorn May 10, 2023

rwestrel May 11, 2023

chhagedorn May 10, 2023

rwestrel May 11, 2023 •

edited

Loading

chhagedorn May 11, 2023

rwestrel May 11, 2023

openjdk bot commented May 10, 2023 •

edited

Loading

chhagedorn left a comment

rwestrel commented May 11, 2023 •

edited

Loading

TobiHartmann commented May 15, 2023

rwestrel commented May 22, 2023

chhagedorn commented May 23, 2023

rwestrel commented May 23, 2023

chhagedorn left a comment

chhagedorn left a comment

chhagedorn commented May 24, 2023

rwestrel commented May 24, 2023

TobiHartmann left a comment

rwestrel commented May 26, 2023

rwestrel commented May 26, 2023

openjdk bot commented May 26, 2023

openjdk bot commented May 26, 2023

8305189: C2 failed "assert(_outcnt==1) failed: not unique" #13901

8305189: C2 failed "assert(_outcnt==1) failed: not unique" #13901

Conversation

rwestrel commented May 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 10, 2023

openjdk bot commented May 10, 2023

mlbridge bot commented May 10, 2023 • edited Loading

Webrevs

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn May 10, 2023

Choose a reason for hiding this comment

rwestrel May 11, 2023

Choose a reason for hiding this comment

chhagedorn May 10, 2023

Choose a reason for hiding this comment

rwestrel May 11, 2023 • edited Loading

Choose a reason for hiding this comment

chhagedorn May 11, 2023

Choose a reason for hiding this comment

rwestrel May 11, 2023

Choose a reason for hiding this comment

openjdk bot commented May 10, 2023 • edited Loading

chhagedorn left a comment

Choose a reason for hiding this comment

rwestrel commented May 11, 2023 • edited Loading

TobiHartmann commented May 15, 2023

rwestrel commented May 22, 2023

chhagedorn commented May 23, 2023

rwestrel commented May 23, 2023

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn commented May 24, 2023

rwestrel commented May 24, 2023

TobiHartmann left a comment

Choose a reason for hiding this comment

rwestrel commented May 26, 2023

rwestrel commented May 26, 2023

openjdk bot commented May 26, 2023

openjdk bot commented May 26, 2023

rwestrel commented May 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 10, 2023 •

edited

Loading

rwestrel May 11, 2023 •

edited

Loading

openjdk bot commented May 10, 2023 •

edited

Loading

rwestrel commented May 11, 2023 •

edited

Loading