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

[Marc Chevalier]

On Thu, 10 Jul 2025 06:13:27 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 two additional commits since the last revision:
> 
>  - Forgot to destruct_map_clone
>  - +'_' and ctor init

Thanks @vnkozlov and @TobiHartmann for reviews!

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

PR Comment: https://git.openjdk.org/jdk/pull/25936#issuecomment-3060908007