RFR: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late [v5]

[Christian Hagedorn]

On Fri, 27 Aug 2021 09:25:51 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Since [JDK-8252372](https://bugs.openjdk.java.net/browse/JDK-8252372), data nodes are sunk out of a loop with the help of pinned `CastXX` nodes to ensure that the data nodes do not float back into the loop. Such a chain of sunk data nodes  is now pinned to the uncommon projection of a loop predicate in the testcase. Loop unswitching is then applied and this loop predicate is cloned down to the fast and the slow loop in `PhaseIdealLoop::clone_predicates_to_unswitched_loop()`. Internally, it uses `PhaseIdealLoop::create_new_if_for_predicate()` to create the new `If` node and the uncommon projection to the uncommon trap region. The UCT region has a phi node from an earlier split-if optimization. The code in `create_new_if_for_predicate()` updates this phi by just setting the data node of the old uncommon projection path as input for the new uncommon projection path:
>> https://github.com/openjdk/jdk/blob/e8f1219d6f471c89fe15b19c56e3062dd668466f/src/hotspot/share/opto/loopPredicate.cpp#L196
>> 
>> It does this for both the slow and the fast loop. The graph looks like this afterwards (for method `testNoDiamond()`):
>> 
>> ![wrong_update](https://user-images.githubusercontent.com/17833009/129887401-05ea8311-c420-444d-b7bb-e2480b7570b6.png)
>> 
>> The data node `1226 AddI` is used for the new uncommon projection paths of the slow loop and the fast loop while still also being used for the old uncommon projection path of `986 IfFalse`. When later determining the earliest and latest control, we find that the early control is `986 IfFalse` for `1226 Addl` while the LCA is `376 IfTrue` (dominates `1347 IfFalse`, `1352 IfFalse` and `986 IfFalse`) which leads to the bad graph assertion failure.
>> 
>> An easy way to fix this is to just set the control to the LCA when cloning the predicates down in loop unswitching but this would revert the work of JDK-8252372 as the nodes could float back into the loop. Therefore, I suggest to improve `create_new_if_for_predicate()` to clone the sunk data node chain to the slow loop and the fast loop. Since the old predicate will die anyways once IGVN is applied after the useless predicates are eliminated in `PhaseIdealLoop::eliminate_useless_predicates()`, I propose to only clone the sunk data node chain for the fast loop once and reuse the old data nodes for the slow loop. I replace the old uncommon projection data input into the phi by TOP which should be fine because the control will die. The graph looks like this after applying these changes:
>> 
>> ![fixed_update](https://user-images.githubusercontent.com/17833009/129887382-5724745e-4b7a-49c2-ae06-5be559ae294f.png)
>> 
>> The algorithm also handles diamonds in the sunk data node chains. 
>> 
>> Thanks,
>> Christian
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Changing to enum

Thanks Vladimir for your review!

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

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