RFR: 8315088: C2: assert(wq.size() - before == EMPTY_LOOP_SIZE) failed: expect the EMPTY_LOOP_SIZE nodes of this body if empty

[Tobias Hartmann]

On Thu, 31 Aug 2023 15:54:50 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> The assert fires in code that looks for counted loops that would be
> empty if not part of a loop strip mining nest (that is some nodes in
> the the loop body are kept alive because of the safepoint in the outer
> loop, the loop would otherwise be empty). That logic starts by finding
> the set of `EMPTY_LOOP_SIZE` nodes that are core to all counted loops
> (exit test, iv phi etc) and store them in the `empty_loop_nodes`
> list.
> 
> Finding the "core" nodes is performed by
> `IdealLoopTree::collect_loop_core_nodes`. It enqueues the backedge of
> the loop in `empty_loop_nodes` and then iteratively pushes inputs of
> nodes in `empty_loop_nodes` that belongs to the loop until it can't
> make progress. There should be `EMPTY_LOOP_SIZE` of those nodes.
> 
>  It's possible that that 2 or more loops are involved in the process:
> a node n in loop 1 could be kept alive by the safepoint of loop 2 so
> loop 2 needs to be empty for n to become dead and possibly loop 1 to
> be empty. When that happens while the logic is in the process of
> determining if loop 1 is empty, it needs to also collect the
> `EMPTY_LOOP_SIZE` nodes that make the "core" of loop 2. They are
> enqueued to the `empty_loop_nodes` list too.
> 
> The failure happens when `empty_loop_nodes` already has the "core"
> nodes of loop 1 and "core" nodes for loop 2 are collected. In the
> process`IdealLoopTree::collect_loop_core_nodes` iterates on all
> `empty_loop_nodes` (loop 1 nodes included, not just starting from the
> loop 2 backedge). That's inefficient (loop 1 nodes can't help find
> nodes that belong to loop 2) and wrong because there could be an edge
> from a node in loop 1 to a node in the body of loop 2. That node in
> loop 2 is them pushed to the `empty_loop_nodes` list. 
> 
> The fix I propose is to only iterate over loop 2 nodes when loop 2 is
> processed.

Thanks for the detailed description, the fix looks good to me.

test/hotspot/jtreg/compiler/loopstripmining/TestBrokenEmptyLoopLogic.java line 26:

> 24: /**
> 25:  * @test
> 26:  * @bug 8315088

Please add a `@requires vm.compiler2.enabled`

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15520#pullrequestreview-1608776531
PR Review Comment: https://git.openjdk.org/jdk/pull/15520#discussion_r1314448085