8359344: C2: Malformed control flow after intrinsic bailout #25936

marc-chevalier · 2025-06-23T13:09:43Z

When intrinsic bailout, we assume that the control in the LibraryCallKit did not change:

jdk/src/hotspot/share/opto/library_call.cpp

Line 137 in c4fb00a

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

This is enforced by restoring the old state, like in

jdk/src/hotspot/share/opto/library_call.cpp

Lines 1722 to 1732 in c4fb00a

    
           uint old_sp = sp(); 
        
           SafePointNode* old_map = clone_map(); 
        
           value = must_be_not_null(value, true); 
        
           Node* adr = array_element_address(value, index, T_CHAR); 
        
           if (adr->is_top()) { 
        
             set_map(old_map); 
        
             set_sp(old_sp); 
        
             return false; 
        
           }

That is good, but not sufficient. First, the most obvious, one could have already built some structure without moving the control. For instance, we can obtain something such as:

Here, during late inlining, the call 323 is candidate to be inline, but that bails out. Yet, a call to make_unsafe_address was made, which built nodes 354 If and everything under. This is needed as tests are made on the resulting nodes (especially 366 AddP) to know whether we should bail out or not. At the end, we get 2 control successor to 346 IfFalse: the call that is not removed and the leftover of the intrinsic that will be cleanup much later, but not by RemoveUseless.

Another situation is somewhat worse, when happening during parsing. It can lead to such cases:

The nodes 31 OpaqueNotNull, 31 If, 36 IfTrue, 33 IfFalse, 35 Halt, 44 If, 45 IfTrue, 46 IfFalse are leftover from a bailing out intrinsic. The replacement call 49 CallStaticJava should come just under 5 Parm, but the control was updated and the call is actually built under 36 If. Then, why does the previous assert doesn't complain?

This is because there is more than one control, or one map. In intrinsics that need to restore their state, the initial SafePoint map is cloned, the clone is kept aside, and if needed (bailing out), we set the current map to this saved clone. But there is another map from which the one of the LibraryCallKit comes, and that survives longer, it's the one that is contained in the JVMState:

jdk/src/hotspot/share/opto/library_call.cpp

Lines 101 to 102 in c4fb00a

    
           JVMState* LibraryIntrinsic::generate(JVMState* jvms) { 
        
             LibraryCallKit kit(jvms, this);

And here there is the challenge:

the JVMState jvms contains a SafePoint map, this map must have jvms as jvms (pointer comparison)
we can't really change the pointer, just the content
after bailing out, we need the map of the jvms to be where it was, so that the graph construction can continue where it was.
if the intrinsic tried building some control flow, but we don't need it, we should remove it.

So... let's do that!

When a intrinsic bails out and regret its choice, we need to have remembered the old JVMState, set the map of it correctly, set the jvms of the map of it correctly, restore the map and sp of the LibraryCallKit as it was done before. On top of that, we remember control nodes that existed under our control() before trying to intrinsify: new control nodes that is not the (new) current map(= the clone of the map before) are disconnected to leave a nice CFG.

This has 2 interesting consequences:

in the case of compiler/intrinsics/VectorIntoArrayInvalidControlFlow.java, compilation used to bailout because of malformed CFG on Aarch64. I'm adding a test that reproduced on x64 and Aarch64 and make the compilation bailout into a crash. The graph is now correctly shape on intrinsic bailout.
in the case of compiler/unsafe/OpaqueAccesses.java, the whole (useless) structure introduced by the bailing out intrinsic is now removed. The call is now connected to where the intrinsic started, not where it ended (as shown under). I've adapted this test to check we don't have both a call and intrinsic leftover. Some of these cases are being intrinsiced, some are left as a call, and I don't want to be too strict about which must be which, as long as they are not both at the same time.

Thanks,
Marc

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-8359344: C2: Malformed control flow after intrinsic bailout (Bug - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25936

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-23T13:10:31Z

👋 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-06-23T13:11:14Z

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

8359344: C2: Malformed control flow after intrinsic bailout

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

f5afbbd: 8361717: Refactor Collections.emptyList() in Locale related classes
cbc7090: 8359809: AttributeList, RoleList and UnresolvedRoleList should never accept other types of Object
f735275: 8361839: Problemlist BogusFocusableWindowState due to failures in the CI pipeline
... and 49 more: https://git.openjdk.org/jdk/compare/7b255b8a625ce1eda1ec6242b8e438691f6cc845...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 · 2025-06-23T13:12:01Z

@marc-chevalier The following labels will be automatically applied to this pull request:

graal
hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-06-24T07:38:20Z

Webrevs

TobiHartmann

Nice analysis! In general, the fix looks good to me. I added a few comments / suggestions.

src/hotspot/share/opto/library_call.hpp

TobiHartmann · 2025-07-07T07:18:11Z

src/hotspot/share/opto/library_call.cpp

    return false;
  }
-  destruct_map_clone(old_map);
+  destruct_map_clone(old_state.map);


I think destruct_map_clone could be refactored to take a SavedState.

I've made an override of destruct_map_clone taking a SavedState (and delegating to the existing one) rather than changing the existing one for the following reasons:

destruct_map_clone is in GraphKit so doesn't know about SavedState. Either I'd need to bring SavedState to the base class (useless visibility) (or something with forward declarations...) or move destruct_map_clone to the derived class LibraryCallKit

destruct_map_clone makes sense to have next to clone_map. But clone_map is used also in GraphKit, so not possible to move to the derived class

The existing destruct_map_clone doesn't need a SavedState and makes sense without. Requiring more information just make it less usable, but it's fine to have a thin adapter that one can by-pass if one has a SafePointNode and not a whole SavedState.

src/hotspot/share/opto/library_call.cpp

TobiHartmann · 2025-07-07T07:28:37Z

src/hotspot/share/opto/library_call.cpp

+  state.jvms = jvms();
+  state.map = clone_map();
+  for (DUIterator_Fast imax, i = control()->fast_outs(imax); i < imax; i++) {
+    Node* out = control()->fast_out(i);


Could we have a similar issue with non-control users? For example, couldn't we also have stray memory users after bailout?

We could, but it should be relatively harmless. Control is more annoying to have more than one successor.

test/hotspot/jtreg/compiler/intrinsics/VectorIntoArrayInvalidControlFlow.java

test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java

openjdk · 2025-07-07T07:43:46Z

@marc-chevalier this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fix/too-many-ctrl-successor-after-intrinsic
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

…uccessor-after-intrinsic

marc-chevalier · 2025-07-08T13:54:22Z

I've addressed the comments, ready for a second pass!

src/hotspot/share/opto/library_call.cpp

vnkozlov · 2025-07-08T20:23:08Z

src/hotspot/share/opto/library_call.hpp

+  struct SavedState {
+    uint sp;
+    JVMState* jvms;
+    SafePointNode* map;
+    Unique_Node_List ctrl_succ;
+  };
+  SavedState clone_map_and_save_state();
+  void restore_state(const SavedState&);
+  void destruct_map_clone(const SavedState& sfp);
+


Can this be a class instead of struct? These methods could be members. Initialization can be done through constructor. The destructor can do restoration by default unless destruct_map_clone() was called before.
I don't like name destruct_map_clone() for this. How about SavedState::remove() or something.

I like it. Since intrinsic implementations have mostly bailing out returns, and few success paths, it's nice to say when we are good, rather than every path that ends with bailing out.

I've called the member function discard. It gives as old_state.discard(), which reads well, I think.

TobiHartmann

Nice! Looks good to me.

src/hotspot/share/opto/library_call.hpp

marc-chevalier · 2025-07-10T06:10:14Z

Turns out in this SavedState tiny refactoring, I removed the underlying call to destruct_map_clone. It's probably benign up to memory consumption, and it made no test fail. Nevertheless, it's back.

vnkozlov

Looks good.

marc-chevalier · 2025-07-11T07:06:36Z

/integrate

Thanks @vnkozlov and @TobiHartmann for reviews!

openjdk · 2025-07-11T07:07:30Z

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

529049b: 8356137: GifImageDecode can produce opaque image when disposal method changes
eddfc64: 8361383: LogFileStreamOutput::write_decorations uses wrong type for format precisions
2e7e272: 8361829: [TESTBUG] RISC-V: compiler/vectorization/runner/BasicIntOpTest.java fails with RVV but not Zvbb
... and 55 more: https://git.openjdk.org/jdk/compare/7b255b8a625ce1eda1ec6242b8e438691f6cc845...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-11T07:07:43Z

@marc-chevalier Pushed as commit 3ffc5b9.

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

marc-chevalier added 6 commits June 20, 2025 12:25

Trying something

b1a06fc

Fix test

867df72

Re-try

0796e25

Adapt test

e51716d

Urgh

4e8ce18

whoops Forgot to remove a bit, and restore sp

54b07e9

marc-chevalier changed the title ~~Fix/too many ctrl successor after intrinsic~~ 8359344 Jun 23, 2025

openjdk bot changed the title ~~8359344~~ 8359344: C2: Malformed control flow after intrinsic bailout Jun 23, 2025

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jun 23, 2025

marc-chevalier marked this pull request as ready for review June 24, 2025 07:33

openjdk bot added the rfr Pull request is ready for review label Jun 24, 2025

Remove useless loop

d51853c

TobiHartmann approved these changes Jul 7, 2025

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jul 7, 2025

marc-chevalier added 2 commits July 8, 2025 11:31

Address comments

f0574bf

Merge remote-tracking branch 'origin/master' into fix/too-many-ctrl-s…

e57cf47

…uccessor-after-intrinsic

openjdk bot removed merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels Jul 8, 2025

Somehow intellij doesn't remove empty indented line

09b24ec

openjdk bot added the rfr Pull request is ready for review label Jul 8, 2025

TobiHartmann reviewed Jul 8, 2025

View reviewed changes

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

Re-insert

e098535

vnkozlov reviewed Jul 8, 2025

View reviewed changes

Tentative addressing Vladimir's comments

5913377

TobiHartmann approved these changes Jul 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 9, 2025

vnkozlov reviewed Jul 9, 2025

View reviewed changes

src/hotspot/share/opto/library_call.hpp Outdated Show resolved Hide resolved

marc-chevalier added 2 commits July 9, 2025 23:50

+'_' and ctor init

31ba6a3

Forgot to destruct_map_clone

be5c024

openjdk bot removed the ready Pull request is ready to be integrated label Jul 10, 2025

TobiHartmann approved these changes Jul 10, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 10, 2025

vnkozlov approved these changes Jul 10, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 11, 2025

openjdk bot closed this Jul 11, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 11, 2025

	uint old_sp = sp();
	SafePointNode* old_map = clone_map();

	value = must_be_not_null(value, true);

	Node* adr = array_element_address(value, index, T_CHAR);
	if (adr->is_top()) {
	set_map(old_map);
	set_sp(old_sp);
	return false;
	}

	JVMState* LibraryIntrinsic::generate(JVMState* jvms) {
	LibraryCallKit kit(jvms, this);

8359344: C2: Malformed control flow after intrinsic bailout #25936

8359344: C2: Malformed control flow after intrinsic bailout #25936

Uh oh!

Conversation

marc-chevalier commented Jun 23, 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 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Jun 23, 2025

Uh oh!

mlbridge bot commented Jun 24, 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!

Uh oh!

TobiHartmann Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

marc-chevalier Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TobiHartmann Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

marc-chevalier Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Jul 7, 2025

Uh oh!

marc-chevalier commented Jul 8, 2025

Uh oh!

Uh oh!

vnkozlov Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

marc-chevalier Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-chevalier commented Jul 10, 2025

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

marc-chevalier commented Jul 11, 2025

Uh oh!

openjdk bot commented Jul 11, 2025

Uh oh!

openjdk bot commented Jul 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

marc-chevalier commented Jun 23, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 23, 2025 •

edited

Loading

mlbridge bot commented Jun 24, 2025 •

edited

Loading