RFR: 8318826: C2: "Bad graph detected in build_loop_late" with incremental inlining [v2]

[Christian Hagedorn]

On Tue, 14 Nov 2023 12:15:52 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In test case, `test1()`, `inlined1()` returns null which is used as
>> receiver for the `notInlined()` virtual call. On the fallthrough
>> projection of the call, the receiver is casted to not null. In the
>> case of `test1()` the fallthrough should be unreachable:
>> `CatchNode::Value()` makes that path dead if it is a projection of a
>> virtual call with a null receiver. When `test1()` is compiled with
>> incremental inlining, before `inlined1()` is inlined, the receiver of
>> the virtual call is not known to be null so the fallthrough path is
>> not killed. When it is inlined, the first input argument to the
>> virtual call node is updated to be null. So that node is pushed on the
>> igvn work list. But the `CatchNode` that's not change in the the
>> process is not. As a result, the fallthrough path of the virtual call
>> is not killed. The input to the cast node in the fallthrough path is
>> changed to null which causes that node to become top. That causes the
>> crash.
>> 
>> IGVN has logic to enqueue nodes that are related to an edge change but
>> are not directly modified. We would need similar logic to be called
>> from late inlining code. This change refactors the igvn logic so it
>> can be called from elsewhere.
>> 
>> `test2()` is similar to `test1()` except the return of the inlined
>> method `inlined2()` is not set to null. Instead, a replaced node is
>> recorded from `a` to null (because class `D` is unrelated to `A` so
>> casting a `A` to `D` successfully only happens for null) and applied
>> when `inlined2()` is late inlined.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   -XX:+IgnoreUnrecognizedVMOptions

Looks good to me, too!

src/hotspot/share/opto/phaseX.hpp line 533:

> 531:   static void add_users_to_worklist0(Node* n, Unique_Node_List& worklist);
> 532:   static void add_users_of_use_to_worklist(Node* n, Node* use, Unique_Node_List& worklist);
> 533:   void add_users_to_worklist (Node *n);

While at it:
Suggestion:

  void add_users_to_worklist(Node* n);

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

> 223: 
> 224:     // Clone nodes and record mapping from current to cloned nodes
> 225:     uint old_nodes = C->unique();

I suggest the following name instead to better describe what the uint is:
Suggestion:

    uint index_before_clone = C->unique();

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16581#pullrequestreview-1724838164
PR Review Comment: https://git.openjdk.org/jdk/pull/16581#discussion_r1393813871
PR Review Comment: https://git.openjdk.org/jdk/pull/16581#discussion_r1393814533