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

[Christian Hagedorn]

On Thu, 22 Jul 2021 19:25:33 GMT, Vladimir Kozlov <kvn 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
>
> src/hotspot/share/opto/ifnode.cpp line 118:
> 
>> 116:   // No intervening control, like a simple Call
>> 117:   Node* r = iff->in(0);
>> 118:   if (!r->is_Region() || r->is_Loop() || phi->region() != r) {
> 
> May be also add ` || r->as_Region()->is_copy() || igvn->_worklist.member(r)` checks.

Could we then possibly miss a split if opportunity when bailing out with `igvn->_worklist.member(r)`? Should we readd the `If` node to the worklist in this case? We could also think about adding `igvn->_worklist.member(r)` further down when we are really sure that we could do a split if.

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

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