[jdk20] RFR: 8298520: C2: assert(found_opaque == res) failed: wrong pattern

[Christian Hagedorn]

On Tue, 13 Dec 2022 14:49:31 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> The assert fires because CountedLoopNode::is_canonical_loop_entry()
> finds an Opaque1 where it expects an OpaqueZeroTripGuard. That Opaque1
> guards the CountedLoopEnd of the pre loop: is_canonical_loop_entry()
> should have stopped at the zero trip guard but walked past it. The
> reason for that is the call for skip_predicates(): when
> CountedLoopNode::skip_predicates_from_entry() encounters an If node
> that has lost a projection, it assumes it's a predicate and moves to
> the next one. In this case, the zero trip guard only has one
> projection (the one that connects it through predicates to the loop
> head), because igvn is in the process of updating a dead part of the
> graph.
> 
> To fix this, I propose that:
> 
> 1- CountedLoopNode::is_canonical_loop_entry() fails when after
> predicates it's at a CountedLoopEnd.
> 
> 2- CountedLoopNode::skip_predicates_from_entry() stops at a zero trip
> guard (identified by a condition that uses a OpaqueZeroTripGuard)
> 
> Either 1- or 2- are good enough to fix this particular graph but I
> propose doing both as making this logic more robust feels safer.
> 
> I don't provide a test case as this seems to only happen when igvn
> processes a dying subgraph in a very specific order.

Fix 2) seems right as the name of the method suggests to only skip actual predicates. But I agree that it makes sense to add fix 1) as well to make it more robust.

`skip_predicates_from_entry()` has become quite hard to read but it should be replaced in the redesign of the skeleton predicates anyways. 

Looks good to me!

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk20/pull/24