8344251: C2: remove blackholes with dead control input #24663

marc-chevalier · 2025-04-15T13:39:52Z

When a BlackholeNode's control input becomes dead, the node is not removed causing the crash

 assert(!in->is_CFG()) failed: CFG Node with no controlling input?

In the case reported in the issue, after a round of peeling, a condition becomes constant, and the branch containing the blackhole becomes dead:

I simply use Node::remove_dead_region(PhaseGVN*, bool) to remove the blackhole, as many other node types do.

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-8344251: C2: remove blackholes with dead control input (Bug - P3)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to a71abe7d
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24663

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-15T13:40:23Z

👋 Welcome back mchevalier! 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 · 2025-04-15T13:40:34Z

@marc-chevalier 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:

8344251: C2: remove blackholes with dead control input

Reviewed-by: shade, thartmann, 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 46 new commits pushed to the master branch:

a4e9da3: 8354191: GTK LaF should use pre-multiplied alpha same as cairo
e00355a: 8353958: Open source several AWT ScrollPane tests - Batch 2
3d0feba: 8352865: Open source several AWT TextComponent tests - Batch 2
... and 43 more: https://git.openjdk.org/jdk/compare/81d4c80742305b72c73a59cf6a596b49bc68bab9...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.

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 (@shipilev, @vnkozlov, @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-04-15T13:41:18Z

@marc-chevalier 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 · 2025-04-15T14:41:19Z

Webrevs

shipilev

Right. I should have done the similar thing from the day 1 :)

src/hotspot/share/opto/cfgnode.cpp

shipilev · 2025-04-15T15:12:39Z

test/hotspot/jtreg/compiler/blackhole/DeadBhElimination.java

+public class DeadBhElimination {
+    public static void main(String[] args) {
+        TestFramework.runWithFlags(
+                "-Xcomp",


-Xcomp is likely too heavy-weight here. Other tests use -XX:CompileThreshold=100. I think that is enough for IR tests to compile the method.

Or you can di separate runs with different flags, including default.

I've added the -Xcomp flag because the dead if branch was compiled into an uncommon trap (reason = unstable_if), and I had no BlackholeNode. Looking at the code emitting the trap, it seems to come from some profiling information, but that the generation of this uncommon trap is explicitly disabled if -Xcomp is given. I am not entirely sure why, but -XX:CompileThreshold=100 does the job to make the compilation of the dead if branch happen as well. But if I give no such flag, the test fails for lack of a BlackholeNode after parsing (also meaning that the test doesn't exercise the deletion of the BlackholeNode).

I'm currently testing further with -XX:CompileThreshold=100 as suggested (it works on my laptop so far), but if you have a more canonical trick to inhibit the uncommon trap in the dead branch, I'd take happily!

Last time I had this problem, I did this:

jdk/test/hotspot/jtreg/compiler/c2/irTests/gc/ReferenceClearTests.java

Lines 48 to 49 in cec48ed

// Use PerMethodTrapLimit=0 to compile all branches in the intrinsics.

args.add("-XX:PerMethodTrapLimit=0");

jdk/test/hotspot/jtreg/testlibrary/ctw/src/sun/hotspot/tools/ctw/CtwRunner.java

Lines 303 to 307 in cec48ed

// CTW does not have good execution profile info, which would uncommon-trap

// a lot of branches/calls that are presumed to be never executed.

// Expand the optimization scope by disallowing most traps.

"-XX:PerMethodTrapLimit=0",

"-XX:PerMethodSpecTrapLimit=0",

That looks exactly like what I'm trying to achieve. I'll re-test with that. Thanks!

-Xcomp is likely too heavy-weight here.

FTR another alternative is to keep -Xcomp, but limit compilation to test methods (-XX:CompileCommand=compileonly,<Test::test>).

vnkozlov

C2 fix code is good.

TobiHartmann

Great work extracting a standalone reproducer for this and narrowing it down! The fix looks good to me.

marc-chevalier · 2025-04-16T11:45:50Z

/integrate

Thanks @shipilev, @vnkozlov and @TobiHartmann for reviews!

openjdk · 2025-04-16T11:47:08Z

@marc-chevalier
Your change (at version 487b1a6) is now ready to be sponsored by a Committer.

test/hotspot/jtreg/compiler/blackhole/DeadBhElimination.java

shipilev

Still good.

vnkozlov · 2025-04-16T20:47:41Z

/sponsor

openjdk · 2025-04-16T20:48:25Z

@vnkozlov The PR has been updated since the change author (@marc-chevalier) issued the integrate command - the author must perform this command again.

marc-chevalier · 2025-04-17T07:09:40Z

/integrate

openjdk · 2025-04-17T07:10:26Z

@marc-chevalier
Your change (at version 9f00099) is now ready to be sponsored by a Committer.

TobiHartmann · 2025-04-17T07:23:11Z

/sponsor

openjdk · 2025-04-17T07:24:15Z

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

fabf67c: 8354625: Compile::igv_print_graph_to_network doesn't use its second parameter
a4e9da3: 8354191: GTK LaF should use pre-multiplied alpha same as cairo
e00355a: 8353958: Open source several AWT ScrollPane tests - Batch 2
... and 44 more: https://git.openjdk.org/jdk/compare/81d4c80742305b72c73a59cf6a596b49bc68bab9...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-17T07:24:29Z

@TobiHartmann @marc-chevalier Pushed as commit 1138a18.

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

marc-chevalier added 3 commits April 15, 2025 11:16

remove_dead_region in Blackhole::Ideal

cb1d1d6

Add test

f636a4c

Format

a71abe7

openjdk bot added the hotspot-compiler [email protected] label Apr 15, 2025

marc-chevalier marked this pull request as ready for review April 15, 2025 14:36

openjdk bot added the rfr Pull request is ready for review label Apr 15, 2025

shipilev reviewed Apr 15, 2025

View reviewed changes

Address review comments

97aac30

vnkozlov approved these changes Apr 15, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 15, 2025

Using PerMethod(Spec)?TrapLimit

487b1a6

openjdk bot removed the ready Pull request is ready to be integrated label Apr 16, 2025

shipilev approved these changes Apr 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2025

TobiHartmann approved these changes Apr 16, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 16, 2025

ky0n reviewed Apr 16, 2025

View reviewed changes

test/hotspot/jtreg/compiler/blackhole/DeadBhElimination.java Outdated Show resolved Hide resolved

cosmetic

9f00099

openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Apr 16, 2025

shipilev approved these changes Apr 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2025

TobiHartmann approved these changes Apr 17, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 17, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 17, 2025

openjdk bot closed this Apr 17, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 17, 2025

	// Use PerMethodTrapLimit=0 to compile all branches in the intrinsics.
	args.add("-XX:PerMethodTrapLimit=0");

	// CTW does not have good execution profile info, which would uncommon-trap
	// a lot of branches/calls that are presumed to be never executed.
	// Expand the optimization scope by disallowing most traps.
	"-XX:PerMethodTrapLimit=0",
	"-XX:PerMethodSpecTrapLimit=0",

8344251: C2: remove blackholes with dead control input #24663

8344251: C2: remove blackholes with dead control input #24663

Uh oh!

Conversation

marc-chevalier commented Apr 15, 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 Apr 15, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Apr 15, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shipilev Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

vnkozlov Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

marc-chevalier Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

shipilev Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-chevalier Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

iwanowww Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

marc-chevalier commented Apr 16, 2025

Uh oh!

openjdk bot commented Apr 16, 2025

Uh oh!

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Apr 16, 2025

Uh oh!

openjdk bot commented Apr 16, 2025

Uh oh!

marc-chevalier commented Apr 17, 2025

Uh oh!

openjdk bot commented Apr 17, 2025

Uh oh!

TobiHartmann commented Apr 17, 2025

Uh oh!

openjdk bot commented Apr 17, 2025

Uh oh!

openjdk bot commented Apr 17, 2025

Uh oh!

Uh oh!

marc-chevalier commented Apr 15, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 15, 2025 •

edited

Loading

mlbridge bot commented Apr 15, 2025 •

edited

Loading

shipilev Apr 15, 2025 •

edited

Loading