RFR: 8327423: C2 remove_main_post_loops: check if main-loop belongs to pre-loop, not just assert [v2]

[Roland Westrelin]

On Tue, 12 Mar 2024 08:24:29 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> The assert was added in [JDK-8085832](https://bugs.openjdk.org/browse/JDK-8085832) (JDK9), by @rwestrel . And in [JDK-8297724](https://bugs.openjdk.org/browse/JDK-8297724) (JDK21), he made more empty loops be removed, and since then the attached regression test fails.
>> 
>> ----------
>> 
>> **Problem**
>> 
>> By the time we get to the assert, we already have had a series of Pre-Main-Post, unroll and empty-loop removal:
>> the PURPLE main and post loops are already previously removed as empty-loops.
>> 
>> At the time of the assert, the graph looks like this:
>> ![image](https://github.com/openjdk/jdk/assets/32593061/cb36eda4-0684-4b79-8557-0fdd5973ab50)
>> 
>> We are in `IdealLoopTree::remove_main_post_loops` with the PURPLE `298 CountedLoop` as the `cl` pre-loop.
>> 
>> The loop-tree looks essencially like this:
>> 
>> (rr) p _ltree_root->dump()
>> Loop: N0/N0 has_sfpt
>> Loop: N425/N431 limit_check profile_predicated predicated counted [0,int),+1 (4 iters) pre sfpts={ 429 }
>> Loop: N298/N301 profile_predicated predicated counted [0,int),+1 (4 iters) pre
>> Loop: N200/N179 counted [int,100),+1 (2147483648 iters) main sfpts={ 171 }
>> Loop: N398/N404 counted [int,100),+1 (4 iters) post sfpts={ 402 }
>> 
>> 
>> This is basically:
>> 
>> 415 pre orange
>> 298 pre PURPLE
>> 200 main orange
>> 398 post orange
>> 
>> 
>> From `298 pre PURPLE`, we try to find its main-loop, by looking at the `_next` info in the loop-tree.
>> There, we find `200 main orange`, it is a main-loop that still is has a pre-loop...
>> ...but not the same pre-loop as `cl` -> the `assert` fires.
>> 
>> It seems that we assume in the code, that we can check the `_next->_head`, and if:
>> 1) it is a main-loop and
>> 2) that main-loop still has a pre-loop
>> then the current pre-loop "cl" must be the pre-loop of that found main-loop locate `_pre_from_main(main_head)`.
>> But this is NOT generally guaranteed by "PhaseIdealLoop::build_loop_tree".
>> 
>> The loop-tree is correct here, and this is how it was arrived at:
>> "415 CountedLoop" (pre orange) is visited, and its body traversed. "427 If" is traversed. Now the path splits.
>> If we first took the "428 IfFalse" path, then we would visit "200 CountedLoop" (main orange), and "398 CountedLoop" (post orange) first.
>> But we instead take "432 IfTrue" first, and hence visit "298 CountedLoop" (pre PURPLE) first.
>> 
>> So depending on what turn we take at this "427 If", we either get the order:
>> 
>> 
>> 415 pre orange
>> 298 pre PURPLE
>> 200 main orange
>> 398 post orange
>> 
>> (the one w...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update test/hotspot/jtreg/compiler/loopopts/TestEmptyPreLoopForDifferentMainLoop.java
>   
>   Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

Looks good to me.

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

Marked as reviewed by roland (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18200#pullrequestreview-1930405522