8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert #1489

merykitty · 2025-06-13T22:53:32Z

Hi,

The issue here is that the IGVN cannot deduce that the IsBuffered input is a constant 1. This is because after the InlineTypeNode is pushed down through a Phi cluster, the IsBuffered input is a Phi cluster with loop phis, which makes it difficult for the IGVN to see through. A solution is to do another round of CCP after loop opts, but it is rather a big hammer. In this PR, I propose to make it possible for a PhiNode to look through its Phi inputs to discover the unique non-phi input of a Phi cluster. The test still fails but it seems more like an expected behaviour rather than an issue this time, I have modified the test a little bit for it to pass.

Please take a look and share your thoughts, thanks very much.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert (Bug - P3)

Reviewers

Tobias Hartmann (@TobiHartmann - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1489

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1489.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-13T22:54:30Z

👋 Welcome back qamai! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-06-13T22:55:25Z

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

8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert

Reviewed-by: 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 8 new commits pushed to the lworld branch:

cab7c4b: 8359377: [lworld] Fix cp indexes in larval stackmaps in redefinition
9c01e84: 8331551: [lworld] compiler/gcbarriers/TestZGCUnrolling.java fails after merging jdk-23+6
bcee145: 8358093: [lworld] Crash when passing invalid constant pool index to early_larval frame
... and 5 more: https://git.openjdk.org/valhalla/compare/c9a58a84345d27c1bb4b09ade8a1669ec8c3c3c0...lworld

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-06-13T22:55:28Z

@merykitty Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2025-06-13T22:58:53Z

Webrevs

TobiHartmann

Thanks for working on this Quan-Anh!

compiler/c2/TestDeadLoopSplitIfLoop.java now fails with -XX:-EnableValhalla:

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/open/src/hotspot/share/opto/phaseX.cpp:784), pid=1729076, tid=1729093
#  assert(no_dead_loop) failed: dead loop detected
#
# JRE version: Java(TM) SE Runtime Environment (25.0) (fastdebug build 25-lworld5ea-LTS-2025-06-16-1157190.tobias.hartmann.valhalla2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 25-lworld5ea-LTS-2025-06-16-1157190.tobias.hartmann.valhalla2, compiled mode, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x184ec45]  PhaseGVN::dead_loop_check(Node*) [clone .part.0]+0x1d5

Current CompileTask:
C2:309   14    b        compiler.c2.TestDeadLoopSplitIfLoop::test (88 bytes)

Stack: [0x00007f1323dff000,0x00007f1323eff000],  sp=0x00007f1323efa0e0,  free space=1004k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x184ec45]  PhaseGVN::dead_loop_check(Node*) [clone .part.0]+0x1d5  (phaseX.cpp:784)
V  [libjvm.so+0x185fe39]  PhaseIterGVN::transform_old(Node*)+0x529  (phaseX.cpp:767)
V  [libjvm.so+0x1854c54]  PhaseIterGVN::optimize()+0xb4  (phaseX.cpp:1054)
V  [libjvm.so+0xb54796]  Compile::Optimize()+0x326  (compile.cpp:2779)
V  [libjvm.so+0xb583bf]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1fef  (compile.cpp:874)
V  [libjvm.so+0x96de34]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x484  (c2compiler.cpp:142)
V  [libjvm.so+0xb66078]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0xb58  (compileBroker.cpp:2307)
V  [libjvm.so+0xb67238]  CompileBroker::compiler_thread_loop()+0x568  (compileBroker.cpp:1951)
V  [libjvm.so+0x10e17fb]  JavaThread::thread_main_inner()+0x13b  (javaThread.cpp:774)
V  [libjvm.so+0x1b5b146]  Thread::call_run()+0xb6  (thread.cpp:231)
V  [libjvm.so+0x17c6e38]  thread_native_entry(Thread*)+0x128  (os_linux.cpp:875)

TobiHartmann · 2025-06-16T14:46:54Z

src/hotspot/share/opto/cfgnode.cpp

+
+      Node* phi_in = current->in(i);
+      if (phi_in->is_Phi()) {
+        visited.append_if_missing(phi_in->as_Phi());


Would a Unique_Node_List be more appropriate here?

Unique_Node_List cannot work with const Node* so GrowableArray is the only option here.

Update: Changed it to a Unique_Node_List.

merykitty · 2025-06-16T18:04:13Z

Thanks for your testing, the failure is due to the fact that when traversing the Phi inputs, I decided to skip dead inputs. This may lead to a case like an iv with a dead backedge: Phi = Phi + 1. However, since the backedge is dead, the function only finds the entry node, leading to entry = entry + 1. I fixed this by traversing all the inputs instead of only the live ones.

TobiHartmann

Looks good to me!

Optimize the case when all inputs into a Phi cluster is the same node

422301b

merykitty force-pushed the phiuniqueinput branch from 562a16f to 422301b Compare June 13, 2025 22:53

openjdk bot added ready rfr labels Jun 13, 2025

TobiHartmann reviewed Jun 16, 2025

View reviewed changes

fix dead loop

67f5c15

use Unique_Node_List

c5071cd

TobiHartmann approved these changes Jun 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert #1489

8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert #1489

Uh oh!

merykitty commented Jun 13, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jun 13, 2025

Uh oh!

openjdk bot commented Jun 13, 2025 •

edited

Loading

Uh oh!

openjdk bot commented Jun 13, 2025

Uh oh!

mlbridge bot commented Jun 13, 2025 •

edited

Loading

Uh oh!

TobiHartmann left a comment

Uh oh!

TobiHartmann Jun 16, 2025

Uh oh!

merykitty Jun 16, 2025

Uh oh!

merykitty Jun 17, 2025

Uh oh!

merykitty commented Jun 16, 2025

Uh oh!

TobiHartmann left a comment

Uh oh!

Uh oh!

8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert #1489

Are you sure you want to change the base?

8336003: [lworld] TestLWorld::test151 triggers "Should have been buffered" assert #1489

Uh oh!

Conversation

merykitty commented Jun 13, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 13, 2025

Uh oh!

openjdk bot commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 13, 2025

Uh oh!

mlbridge bot commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

merykitty commented Jun 16, 2025

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

merykitty commented Jun 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 13, 2025 •

edited

Loading

mlbridge bot commented Jun 13, 2025 •

edited

Loading