RFR: 8291599: Assertion in PhaseIdealLoop::skeleton_predicate_has_opaque after JDK-8289127 [v3]

[Christian Hagedorn]

On Fri, 9 Sep 2022 12:05:03 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.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Christian's review
>  - Merge branch 'master' into JDK-8291599
>  - Update test/hotspot/jtreg/compiler/loopopts/TestPhiInSkeletonPredicateExpression.java
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopTransform.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - fix
>  - test

That looks good to me, thanks for doing the changes!

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

Marked as reviewed by chagedorn (Reviewer).

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