RFR: 8359344: C2: Malformed control flow after intrinsic bailout [v4]

[Tobias Hartmann]

On Tue, 8 Jul 2025 13:53:25 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> 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:
>> 
>> ![1 after-intrinsic-bailout-during-late-inlining](https://github.com/user-attachments/assets/2fd255cc-0bfc-4841-8dd1-f64d502e0ee1)
>> 
>> 
>> 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:
>> 
>> ![2 after-intrinsic-bailout-during-parsing](https://github.com/user-attachments/assets/4524c615-6521-4f0d-8f61-c426f9179035)
>> 
>> 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)
>> ...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Somehow intellij doesn't remove empty indented line

src/hotspot/share/opto/library_call.cpp line 2507:

> 2505: 
> 2506:   if (alias_type->adr_type() == TypeInstPtr::KLASS ||
> 2507:   alias_type->adr_type() == TypeAryPtr::RANGE) {

The indentation is off here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25936#discussion_r2192803477