RFR: 8350579: Remove Template Assertion Predicates belonging to a loop once it is folded away during IGVN
Christian Hagedorn
chagedorn at openjdk.org
Thu Feb 27 13:12:43 UTC 2025
The patch fixes the issue of creating an Initialized Assertion Predicate at a loop X from a Template Assertion Predicate that was originally created for a loop Y. Using the unrelated loop values from loop Y for the Initialized Assertion Predicate will let it fail during runtime and we execute a `halt` instruction. This was originally reported with [JDK-8305428](https://bugs.openjdk.org/browse/JDK-8305428).
Note that most of the line changes are from new tests.
### The Problem
There are multiple test cases triggering the same problem. In the following, when referring to "the test case", I'm referring to `testTemplateAssertionPredicateNotRemovedHalt()` which was written from scratch and contains more detailed comments explaining how we end up with executing a `Halt` node in more details.
#### An Inner Loop without Parse Predicates
The graph in `testTemplateAssertionPredicateNotRemovedHalt()` looks like this after creating `LoopNodes` for the outer `for` and inner `while (true)` loop:

We only have Parse Predicates for the outer loop. Why?
Before beautify loop, we have the following region which merges multiple backedges - the one from the `for` loop and the one from the `while (true)` loop:

In `IdealLoopTree::merge_many_backedges()`, we notice that the hottest backedge is hot enough such that it is worth to have a separate merge point region for the inner and outer loop. We set everything up and eventually in `IdealLoopTree::split_outer_loop()`, we create a second `LoopNode`.
For this inner `LoopNode`, we cannot set up `Parse Predicates` with the same UCTs as used for the outer loop. It would be incorrect when taking the trap to re-execute the inner and outer loop again while having already executed some of the outer loop's iterations. Thus, we get the graph shape with back-to-back `LoopNodes` as shown above.
#### Predicates from a Folded Loop End up at Another Loop
As described in the previous section, we have an inner and outer `LoopNode` while the inner does not have Parse Predicates. In a series of events (see test case comments for more details), we first hoist a range check out of the outer loop during Loop Predication with a Template Assertion Predicate. Then, we fold the outer loop away because we find that it is only running for a single iteration and the backedge is never taken. The Template Assertion Predicate together with the Parse Predicates end up at the inner loop running from `i = 80`:

#### Creating Initialized Assertion Predicate with Wrong Loop Values
We now split the inner loop by creating pre-main-post loops. In this process, we create new Template Assertion Predicates with the new init value of the main and post loop. We also create Initialized Assertion Predicates from the new templates. But these now use the init value from the inner loop, even though the Assertion Predicates were created with the loop values from the outer loop:

`iArrShort` has only a size of `10` but `512 Phi` takes value `80`. During runtime, this Initialized Assertion Predicate fails and we crash by executing a halt instruction.
### Proposed Solution
We should remove any Template Assertion Predicate when a `CountedLoopNode` is folded away. This is implemented in `CountedLoopNode::Ideal()` to do that right during IGVN when a loop node is folded. This ensures that we do not miss any dying loop.
#### Implementation Details
- I introduced a new `KillTemplateAssertionPredicates` visitor to do that. This required a new `TemplateAssertionPredicate::kill_during_igvn()` method to directly operate on `PhaseIterGVN` instead of `PhaseIdealLoop`.
- Regular Predicates (i.e. Runtime or Assertion Predicates) all use `If` nodes with some specific inputs (i.e. flavors of `Opaque*` nodes) or outputs (i.e. `Halt` or UCTs). Since we now use `PredicateIterator` during IGVN, we need to be more careful when a Regular Predicate is being folded away to still recognize it as a Regular Predicate. When we fail to do so, we could stop the iteration and miss predicates above. The existing checks are not strong enough and required the following tweaks for some situations:
- An Assertion Predicate `If` has a `ConI` as input because the `Opaque*` node was already folded.
- -> Also check that we have a `Halt` node on the false path (done with `AssertionPredicate::may_be_assertion_predicate_if()`).
- A Regular Predicate `If` already lost one of its output.
- -> Treat such a predicate as Runtime Predicate and assume that an Assertion Predicate `If` always has two outputs (done with `RuntimePredicate::is_being_folded_without_uncommon_proj()` and `AssertionPredicate::may_be_assertion_predicate_if()`).
- Added some comments at the predicate classes to reflect these changes.
### Tests
- Hand written tests together with some tests that triggered issues during the implementation in the initial full Assertion Predicate prototype patch.
- Tests from the report of JDK-8305428 and its duplicates.
Thanks,
Christian
-------------
Commit messages:
- 8350579: Remove Template Assertion Predicates belonging to a
Changes: https://git.openjdk.org/jdk/pull/23823/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23823&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8350579
Stats: 571 lines in 4 files changed: 545 ins; 2 del; 24 mod
Patch: https://git.openjdk.org/jdk/pull/23823.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23823/head:pull/23823
PR: https://git.openjdk.org/jdk/pull/23823
More information about the hotspot-compiler-dev
mailing list