RFR: 8327423: C2 remove_main_post_loops: check if main-loop belongs to pre-loop, not just assert
Emanuel Peter
epeter at openjdk.org
Mon Mar 11 16:42:37 UTC 2024
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:

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 trigger, since we would have "_next == nullptr" and return)
--------
**Solution**
We need to convert the `assert` into a condition. If the condition fails, we have no main-loop, and can just return from `remove_main_post_loops`.
Note: if we have an empty pre-loop that still has a main-loop, then the loop-tree must have the pre-loop and main-loop adjacent, i.e. you can get from the pre-loop to its main-loop via `_next`. That is because the `build_loop_tree` traversal cannot take any other path.
-------------
Commit messages:
- 8327423
Changes: https://git.openjdk.org/jdk/pull/18200/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18200&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8327423
Stats: 64 lines in 2 files changed: 61 ins; 2 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18200.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18200/head:pull/18200
PR: https://git.openjdk.org/jdk/pull/18200
More information about the hotspot-compiler-dev
mailing list