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

[Vladimir Kozlov]

On Mon, 11 Mar 2024 15:56:38 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 we get, and assert with)
> 
> OR
> 
> 
> 415 pre orange
> 200 main orange
> 398 post orange
> 298 pre PURPLE
> 
> (assert woud not tr...

I agree with fix.

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

Marked as reviewed by kvn (Reviewer).

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