RFR: 8312980: C2: "malformed control flow" created during incremental inlining [v2]

[Tobias Hartmann]

On Tue, 3 Oct 2023 12:01:42 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> When `testHelper()` is late inlined, `o` is recorded in the replaced
>> nodes because of the cast to `B` in that method. When late inlining
>> finishes, c2 tries to replace `o` with the resulting `CheckCastPP` to
>> `B` for uses dominated by the call. ``test` and `testHelper2` have 2
>> type checks for A: there are 2 `CheckCastPP` nodes pinned at 2
>> different controls (one is dominated by the `testHelper()` call, one
>> is not), a single `CmpP` that's shared by the 2 type checks (one of
>> the type check is dominated, the other is not). To check if a
>> replacement is legal, for each use of the node to be replaced, the
>> code in `ReplacedNodes::apply()` follow uses and uses of uses until it
>> finds a node that's a CFG node or is pinned. It then uses the IGVN
>> heuristics to figure out if the CFG is dominated by the call or
>> not. If it finds a CFG node that is not dominated, then that use is
>> skipped.
>> 
>> What happens here is that `ReplacedNodes::apply()` checks the control
>> input for each `CheckCastPP` and finds one to be dominated and the
>> other not. So it performs the replacement only for the one that's
>> dominated. For the shared `CmpP`, it follows uses until it reaches the
>> `If` nodes of the type checks. One is dominated. The other is not. So
>> it declares `CmpP` to not be dominated and skips it.
>> 
>> When IGVN runs next, the `CheckCastPP` that had its input replaced now
>> casts a `B` to a `A` which results to top but the check that `o` is of
>> type `A` that guards the type check still tests that `o` of type
>> `Object` is a `B`. That check doesn't constant fold. Replaced nodes
>> introduced an inconsistency. What we would have needed, is for both
>> the `CmpP` and `CheckCastPP` inputs to be changed. But that wasn't
>> possible because the `CmpP` is shared.
>> 
>> The fix I propose is a tweak to `ReplacedNodes::apply()` so it goes
>> depth first over the use of the node to be replaced (let's call it N)
>> and the use of its uses. When it hits a node that's pinned or a CFG,
>> it checks if it is dominated. If that is the case, the chain of use
>> that leads from N is cloned and the clone of the use of N gets the
>> improved input. That way if a node on that path is shared and used in
>> some CFG path that's not dominated, it is unaffected. We then let IGVN
>> cleans up extraneous clones.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into JDK-8312980
>  - fix & test

Thanks for the detailed explanation, Roland. I was a bit worried that there might be an explosion of cloned nodes but I convinced myself that there is no better option. The fix looks good to me.

Testing is still running but looks good as well.

src/hotspot/share/opto/replacednodes.cpp line 127:

> 125:     }
> 126: 
> 127:     // Try to find some use of initial that are dominated by ctl so, initial can be replaced by improved.

Suggestion:

    // Try to find some use of initial that is dominated by ctl so, initial can be replaced by improved.

src/hotspot/share/opto/replacednodes.cpp line 193:

> 191: }
> 192: 
> 193: void ReplacedNodes::clone_stack(Compile* C, Node* initial, Node* improved, const Node_Stack& stack, int i) const {

What about using a more descriptive name here like `clone_uses_and_replace` and maybe add a brief comment?

src/hotspot/share/opto/replacednodes.cpp line 199:

> 197:     Node* clone = n->clone();
> 198:     bool is_in_table = C->initial_gvn()->hash_delete(prev);
> 199:     if (i == -1){

Suggestion:

    if (i == -1) {

src/hotspot/share/opto/replacednodes.cpp line 203:

> 201:       assert(replaced > 0, "expected some use");
> 202:     } else {
> 203:       assert(k == stack.size() -2 && prev->is_Phi(), "");

Suggestion:

      assert(k == (stack.size() - 2) && prev->is_Phi(), "");

src/hotspot/share/opto/replacednodes.cpp line 216:

> 214:   }
> 215:   bool is_in_table = C->initial_gvn()->hash_delete(prev);
> 216:   if (i == -1){

Suggestion:

  if (i == -1) {

src/hotspot/share/opto/replacednodes.cpp line 233:

> 231:   assert(n->is_CFG(), "should be CFG now");
> 232:   int depth = 0;
> 233:   while(n != ctl) {

Suggestion:

  while (n != ctl) {

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15905#pullrequestreview-1656701840
PR Review Comment: https://git.openjdk.org/jdk/pull/15905#discussion_r1345224073
PR Review Comment: https://git.openjdk.org/jdk/pull/15905#discussion_r1345231136
PR Review Comment: https://git.openjdk.org/jdk/pull/15905#discussion_r1345227435
PR Review Comment: https://git.openjdk.org/jdk/pull/15905#discussion_r1345227954
PR Review Comment: https://git.openjdk.org/jdk/pull/15905#discussion_r1345227242
PR Review Comment: https://git.openjdk.org/jdk/pull/15905#discussion_r1345225619