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

[Tobias Hartmann]

On Fri, 30 Aug 2024 15:58:30 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 close 8336478 as
> duplicate of this one.

Thanks for the detailed summary. The fix looks reasonable to me.

src/hotspot/share/opto/loopnode.cpp line 4591:

> 4589:   // Verify that the has_loops() flag set at parse time is consistent with the just built loop tree. When the back edge
> 4590:   // is an exception edge, parsing doesn't set has_loops().
> 4591:   assert(_ltree_root->_child == nullptr || C->has_loops() || C->has_exception_backedge(), "parsing found no loops but there are some");

`PhaseIdealLoop::only_has_infinite_loops` is dead now and should be removed.

src/hotspot/share/opto/loopnode.hpp line 1081:

> 1079:   // Place 'n' in some loop nest, where 'n' is a CFG node
> 1080:   void build_loop_tree();
> 1081:   int build_loop_tree_impl(Node *n, int pre_order);

Suggestion:

  int build_loop_tree_impl(Node* n, int pre_order);

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20797#pullrequestreview-2276967473
PR Review Comment: https://git.openjdk.org/jdk/pull/20797#discussion_r1741779954
PR Review Comment: https://git.openjdk.org/jdk/pull/20797#discussion_r1741782737