RFR: 8253644: C2: assert(skeleton_predicate_has_opaque(iff)) failed: unexpected [v3]

[Vladimir Kozlov]

On Tue, 1 Dec 2020 10:20:18 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> `test1()` fails while creating pre/main/post loops when copying skeleton predicates to the main loop. The problem is that we find phi nodes when following a skeleton `Opaque4` bool node, when trying to find the `OpaqueLoopInit` and `OpaqueLoopStride` nodes. This is unexpected and lets the assertion fail. This happens due to the following reason:
>> 
>> An inner loop of a nested loop is first unswitched and the skeleton predicates are copied to the slow and fast loop by just creating a new `If` node that share the same `Opaque4` node:
>> https://github.com/openjdk/jdk/blob/973255c469d794afe8ee74b24ddb5048bfcaadf7/src/hotspot/share/opto/loopPredicate.cpp#L268-L273
>> 
>> The loop tree building algorithm recognizes both loops as children of the parent loop:
>> Loop: N0/N0  has_sfpt
>>   Loop: N338/N314  limit_check profile_predicated predicated counted [0,100),+1 (2 iters)  has_sfpt
>>      Loop: N459/N458  profile_predicated predicated counted [0,10000),+1 (5271 iters)  has_sfpt (slow loop)
>>      Loop: N343/N267  profile_predicated predicated counted [0,10000),+1 (5271 iters)  has_sfpt (fast loop)
>> 
>> Some additional optimizations are then applied such that the fast loop no longer has any backedge/path to the parent loop while the slow loop still has. As a result, the loop tree building algorithm only recognizes the slow loop as child while the fast loop is not. The fast loop is treated as a separate loop on the same level as the parent loop:
>> Loop: N0/N0  has_sfpt
>>   Loop: N338/N314  limit_check profile_predicated predicated counted [0,100),+1 (2 iters)  has_sfpt
>>     [N459, but the loop could actually be removed in the meantime but the skeleton predicates are still there]
>>   Loop: N343/N267  profile_predicated predicated counted [0,10000),+1 (5274 iters)  has_sfpt
>> 
>> Now the original parent loop (N338) gets peeled. The fast and the slow loop still both share skeleton `Opaque4` bool nodes with all their inputs nodes up to and including the `OpaqueLoopInit/Stride` nodes. Let's look at one of the skeleton `If` nodes for the fast loop that uses such a `Opaque4` node. The skeleton `If` is no longer part of the original parent loop and is therefore not peeled. But now we need some phi nodes to select the correct nodes either from the peeled iteration or from N338 for this skeleton `If` of the fast loop. This is done in `PhaseIdealLoop::clone_iff()` which creates a new `Opaque4` node together with new `Bool` and `Cmp` nodes and then inserts some phi nodes to do the selection. 
>> 
>> When afterwards creating pre/main/post loops for the fast loop (N343) that is no child anymore, we find these phi nodes on the path to the `OpaqueLoopInit/Stride` nodes which lets the assertion fail. A more detailed explanation why this happens can be find at `test1()` in [TestUnswitchCloneSkeletonPredicates](https://github.com/chhagedorn/jdk/blob/cfc7c941ddefd716c9f07bf7e8f4824f00bb7e9b/test/hotspot/jtreg/compiler/loopopts/TestUnswitchCloneSkeletonPredicates.java#L51).
>> 
>> I propose to copy the skeleton predicates to the unswitched loops in the same way as we copy the skeleton predicates to the main loop by cloning all the nodes on the path to the` OpaqueLoopInit/Stride` nodes with a small adaptation: We should also copy the `OpaqueLoopInit/Stride` nodes and we should keep the uncommon traps because we only want to replace them by `Halt` nodes once we create pre/main/post loops.
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn 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 four additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8253644
>  - Add back accidentally removed execution of test2-5.
>  - Fixing whitespaces
>  - 8253644: C2: assert(skeleton_predicate_has_opaque(iff)) failed: unexpected

Marked as reviewed by kvn (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/1448