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

Mon Jul 7 07:45:46 UTC 2025

On Tue, 1 Jul 2025 16:14:00 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:
> 
>   Remove useless loop

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

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

> 1730:     return false;
> 1731:   }
> 1732:   destruct_map_clone(old_state.map);

I think `destruct_map_clone` could be refactored to take a `SavedState`.

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

> 2374:   state.map = clone_map();
> 2375:   for (DUIterator_Fast imax, i = control()->fast_outs(imax); i < imax; i++) {
> 2376:     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?

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

> 2391:     Node* out = control()->fast_out(i);
> 2392:     if (out->is_CFG() && out->in(0) == control() && out != map() && !state.ctrl_succ.member(out)) {
> 2393:       out->set_req(0, C->top());

Could `out` already be in the GVN hash ("remove node from hash table before modifying it")?

src/hotspot/share/opto/library_call.hpp line 129:

> 127:   virtual int reexecute_sp() { return _reexecute_sp; }
> 128: 
> 129:   struct SavedState {

Please add a comment describing what it's used for.

test/hotspot/jtreg/compiler/intrinsics/VectorIntoArrayInvalidControlFlow.java line 2:

> 1: /*
> 2:  * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.

Suggestion:

test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 104:

> 102:     public static final String START = "(\\d+(\\s){2}(";
> 103:     public static final String MID = ".*)+(\\s){2}===.*";
> 104:     public static final String END = ")";

I don't like exposing these outside the IR framework but then again I don't really have an idea on how to check the "graph should not have both nodes" invariant. Maybe we should extend the `counts` annotation to support something like `@IR(counts = {IRNode.CallStaticJava, IRNode.OpaqueNotNull, "<= 1"} [...]`?

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25936#pullrequestreview-2992473824
PR Review Comment: https://git.openjdk.org/jdk/pull/25936#discussion_r2189175998
PR Review Comment: https://git.openjdk.org/jdk/pull/25936#discussion_r2189211960
PR Review Comment: https://git.openjdk.org/jdk/pull/25936#discussion_r2189198041
PR Review Comment: https://git.openjdk.org/jdk/pull/25936#discussion_r2189172691
PR Review Comment: https://git.openjdk.org/jdk/pull/25936#discussion_r2189212910
PR Review Comment: https://git.openjdk.org/jdk/pull/25936#discussion_r2189244934