RFR: 8338100: C2: assert(!n_loop->is_member(get_loop(lca))) failed: control must not be back in the loop [v3]

[Tobias Hartmann]

On Thu, 5 Sep 2024 13:21:19 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The crash occurs because a `Store` is sunk out of a loop that's an
>> inner loop of an infinite loop. The infinite loop was just found to be
>> infinite in the current round of loop opts. When that happens the
>> infinite loop is not properly attached to the rest of the loop tree. As
>> a consequence, the `IdealLoopTree` instance for the infinite loop and
>> its children are only partially initialized (`_nest` is not set) and
>> the structure is an inconsistent state.
>> 
>> When the `Store` is sunk it's reported as belonging to a loop but the
>> `IdealLoopTree` for that loop is only half populated. As a consequence
>> a call to `is_dominator` for that loop hits an inconsistency, returns
>> an incorrect result and the assert fires.
>> 
>> A possible fix would be a point fix that skips that optimization for a
>> loop that's part of an infinite loop nest. But given basic methods of
>> loop opts can't be trusted to work in the infinite loop nest, I
>> suppose similar issues can surface elsewhere.
>> 
>> It's not the first time, we have issues with an infinite loop that's
>> not properly attached to the loop tree the first time it is
>> encountered (a NeverBranch is then added and on the next loop passes,
>> the infinite loop is properly attached to the loop tree). For instance
>> on a loop opts round, C2 can see that it has no loops and on the next
>> that it has some.
>> 
>> I propose fixing this by properly attaching the infinite loop to the
>> loop tree when it's first discovered. A comment in the code seems to
>> hint that it requires going over the graph again after the
>> `NeverBranch` is added but I don't think that's case.
>> 
>> I changed the assert in `loopnode.cpp` because it was there to work
>> around the inconsistency I mentioned above (no loop in a round, some
>> loops on the next one).
>> 
>> The change in `parse1.cpp` fixes an issue I ran into when testing the
>> fix. The existing logic doesn't properly detect an exception backedge.
>> 
>> I added the test case from 8336478 to this. The problem there is that
>> an infinite loop contains a long counted loop. The long counted loop
>> is transformed into a loop nest which is a 2 step process that
>> requires 2 rounds of loop opts. But c2 finds an infinite loop in the
>> middle of the process which causes it to see no more loops and to not
>> attempt another round of loop opts. The assert fires because it finds
>> a long counted loop nest that's half transformed. The change I propose
>> here fixes this too. If we go with this fix, I'll c...
>
> 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 nine additional commits since the last revision:
> 
>  - remove useless PhaseIdealLoop::only_has_infinite_loops()
>  - Merge branch 'master' into JDK-8338100
>  - Update src/hotspot/share/opto/loopnode.hpp
>    
>    Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>
>  - Update src/hotspot/share/opto/loopnode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopnode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - comment
>  - test fix
>  - remove verification code
>  - test & fix

Looks good.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20797#pullrequestreview-2283085094