RFR: 8359344: C2: Malformed control flow after intrinsic bailout
Marc Chevalier
mchevalier at openjdk.org
Tue Jun 24 07:38:21 UTC 2025
When intrinsic bailout, we assume that the control in the `LibraryCallKit` did not change:
https://github.com/openjdk/jdk/blob/c4fb00a7be51c7a05a29d3d57d787feb5c698ddf/src/hotspot/share/opto/library_call.cpp#L137
This is enforced by restoring the old state, like in
https://github.com/openjdk/jdk/blob/c4fb00a7be51c7a05a29d3d57d787feb5c698ddf/src/hotspot/share/opto/library_call.cpp#L1722-L1732
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`:
https://github.com/openjdk/jdk/blob/c4fb00a7be51c7a05a29d3d57d787feb5c698ddf/src/hotspot/share/opto/library_call.cpp#L101-L102
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
-------------
Commit messages:
- whoops Forgot to remove a bit, and restore sp
- Urgh
- Adapt test
- Re-try
- Fix test
- Trying something
Changes: https://git.openjdk.org/jdk/pull/25936/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25936&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8359344
Stats: 373 lines in 7 files changed: 277 ins; 50 del; 46 mod
Patch: https://git.openjdk.org/jdk/pull/25936.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/25936/head:pull/25936
PR: https://git.openjdk.org/jdk/pull/25936
More information about the graal-dev
mailing list