RFR: 8291599: Assertion in PhaseIdealLoop::skeleton_predicate_has_opaque after JDK-8289127

[Tobias Hartmann]

On Thu, 25 Aug 2022 12:30:02 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> In TestPhiInSkeletonPredicateExpression.test1():
> 
> - Loop predication adds predicates for the null check of array and the
>   array range check. It also adds skeleton predicates in case of
>   subsequent unrolling.
> 
> - One of the skeleton predicate has the following shape:
> 
> (Opaque4 (Bool (CmpUL (AddL (AddL (ConvI2L (LoadI (Phi ...))) (ConvI2L (CastII (AddI (OpaqueLoopInit OpaqueLoopStride))))) -1) ...)))
> 
> - Split thru phi pushes the null check through the dominating
>   region. The skeleton predicate subgraph is transformed to:
> 
> (Opaque4 (Bool (CmpUL (Phi ...) ...)))
> 
> - Logic that processes skeleton predicate can no longer find the
>   OpaqueLoopInit and OpaqueLoopStride nodes because they are now
>   behind a phi. That causes the assert to fire.
> 
> The fix I propose is to catch cases where part of a skeleton predicate
> expression (a subgraph with a OpaqueLoopInit or OpaqueLoopStride node)
> is being split during split if and to clone the entire skeleton
> predicate subgraph then.
> 
> There's a already logic for that currently but it only triggers if
> PhaseIdealLoop::split_up() tries to split an OpaqueLoopInit or
> OpaqueLoopStride. In the case here, the OpaqueLoopInit and
> OpaqueLoopStride nodes have control above the region at which split if
> occurs. So they are never split by PhaseIdealLoop::split_up(). The
> AddL nodes in subgraph are.

Looks good to me.

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10022