RFR: 8268019: C2: assert(no_dead_loop) failed: dead loop detected [v2]

[Vladimir Kozlov]

On Mon, 26 Jul 2021 12:09:32 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> In the testcase, a path containing a loop is found to be dead. These nodes are then removed during IGVN after parsing (before loop opts). One of these nodes is a loop header region through which an `If` is split. Splitting an `If` through a loop header region is not intended and was disabled by [JDK-8232539](https://bugs.openjdk.java.net/browse/JDK-8232539). However, the bailouts in the current code are not enough to prevent the split if optimization for this dying loop header node due to the following reasons:
>> - The region is not a `LoopNode`, yet, and we do not bail out at:
>> https://github.com/openjdk/jdk/blob/cd8783c08ee18167f15df621e997015b971bfb01/src/hotspot/share/opto/ifnode.cpp#L119
>> - The loop header region has only one phi left (the other phis were already killed) and we do not bail out at:
>> https://github.com/openjdk/jdk/blob/cd8783c08ee18167f15df621e997015b971bfb01/src/hotspot/share/opto/ifnode.cpp#L143-L148
>> - The phi merges a `ConP` constant (dead path) and a `CastPP` node which is also allowed as constant:
>> https://github.com/openjdk/jdk/blob/cd8783c08ee18167f15df621e997015b971bfb01/src/hotspot/share/opto/ifnode.cpp#L98-L101
>> - The loop entry was already replaced by top and thus no predicates can be found to bail out:
>> https://github.com/openjdk/jdk/blob/cd8783c08ee18167f15df621e997015b971bfb01/src/hotspot/share/opto/ifnode.cpp#L244-L247
>> 
>> These conditions let the split if optimization to be applied without a bailout. The `CastPP` node is a use of the phi and is thus rewired in the process. The problem now is that the `CastPP` node is an input *and* output of the loop phi while being a constant to be merged:
>> ![before_split_if](https://user-images.githubusercontent.com/17833009/126507548-62583105-b746-4551-8037-aba1b15b1d5a.png)
>> 
>> This lets the split if optimization to create a data loop by setting the input of the `CastPP` to itself.
>> 
>> This scenario is rare and highly depends on the order in which IGVN processes the nodes. As a fix, I propose to extend the bailout checks for loop header regions to catch this edge case. The new check looks at all inputs of the region and bail out if only one path is non-top. This also avoids unnecessary splits through regions which are dying anyways.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   new fix with worklist check

Good.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4860