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

[Christian Hagedorn]

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.

Otherwise, the fix looks good to me!

src/hotspot/share/opto/loopTransform.cpp line 1454:

> 1452: }
> 1453: 
> 1454: void PhaseIdealLoop::skeleton_predicate_opaque_helper(Node* n, uint& init, uint& stride) {

I suggest to rename this method to make it more clear what its purpose is. Maybe `count_opaque_loop_nodes`?

src/hotspot/share/opto/loopTransform.cpp line 1456:

> 1454: void PhaseIdealLoop::skeleton_predicate_opaque_helper(Node* n, uint& init, uint& stride) {
> 1455:   init= 0;
> 1456:   stride= 0;

Suggestion:

  init = 0;
  stride = 0;

test/hotspot/jtreg/compiler/loopopts/TestPhiInSkeletonPredicateExpression.java line 26:

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

Suggestion:

 * @bug 8291599

test/hotspot/jtreg/compiler/loopopts/TestPhiInSkeletonPredicateExpression.java line 28:

> 26:  * bug 8291599
> 27:  * @summary Assertion in PhaseIdealLoop::skeleton_predicate_has_opaque after JDK-8289127
> 28:  * @run main/othervm -XX:-TieredCompilation -XX:-BackgroundCompilation -XX:-UseOnStackReplacement -XX:LoopMaxUnroll=0 TestPhiInSkeletonPredicateExpression

Since `LoopMaxUnroll` is a C2 flag, we should also add a `@requires vm.compiler2.enabled`.

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

Changes requested by chagedorn (Reviewer).

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