RFR: 8315920: C2: "control input must dominate current control" assert failure [v3]

[Tobias Hartmann]

On Mon, 25 Sep 2023 13:46:12 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The assert fires during range check eliminationin code that was added
>> recently. It catches a bug that's been going unnoticed. The `array[0]`
>> load of the test case ends up on the exit of the loop above it. After
>> pre/main/post loops are created for this loop (the inner loop is
>> optimized out), the load is at a region merging control from the just
>> created pre/main/post loops but its control (as returned by
>> `PhaseIdealLoop::get_ctrl()`) is not updated by pre/main/post loops
>> creation code so it's still the exit projection of one of the
>> loops. This causes the assert to fire (the load has bad control) when
>> RC runs next in the same loop opts pass.
>> 
>> The current logic for pre/main/post loops creation does update the
>> control of nodes on an exit projection but only if the node is
>> referenced from some other node in the loop body.
>> 
>> I think a similar bug with a node assigned control at the exit but not
>> pinned is likely to exist. So I doubt simply going over nodes pinned
>> at exits during pre/main/post and updating their control is good
>> enough. What I propose instead is to `lazy_replace()` the projection
>> by the region so that, for all nodes that are assigned the exit as
>> control, `get_ctrl()` will then return the region. It's not entirely
>> straightforward to implement because the `lazy_replace()` mechanism
>> assumes the control being replaced is dead which is not the case
>> here. That's why in the patch the exit projection is cloned so the
>> existing one is dead and can be used in `lazy_replace()`. This also
>> makes the calls to `sink_use()` redundant.
>
> 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 seven additional commits since the last revision:
> 
>  - comments
>  - Merge branch 'master' into JDK-8315920
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
>  - fix & test

Looks good to me, thanks for adding the comments!

src/hotspot/share/opto/loopopts.cpp line 2621:

> 2619:         // - have control set to the loop exit
> 2620:         // below the post-loop merge point. lazy_replace() takes a dead control as first input. To make it
> 2621:         // possible to use it, the loop exit projection is cloned and become the new exit projection. The initial one

Suggestion:

        // possible to use it, the loop exit projection is cloned and becomes the new exit projection. The initial one

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15720#pullrequestreview-1642343116
PR Review Comment: https://git.openjdk.org/jdk/pull/15720#discussion_r1335995878