8350579: Remove Template Assertion Predicates belonging to a loop once it is folded away during IGVN

[chhagedorn]

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.

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

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8350579: Remove Template Assertion Predicates belonging to a loop once it is folded away during IGVN (Sub-task - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23823/head:pull/23823
$ git checkout pull/23823

Update a local copy of the PR:
$ git checkout pull/23823
$ git pull https://git.openjdk.org/jdk.git pull/23823/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23823

View PR using the GUI difftool:
$ git pr show -t 23823

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23823.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@chhagedorn The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (63896b3f)

[rwestrel]

That looks reasonable to me but imaking sure that predicates in the process of being removed are properly stepped over feels like something that could be fragile. So I'm wondering if there would be a way to mark predicates as being for a particular loop (maybe storing the loop's node id they apply to in predicate nodes and making sure it's properly updated as loops are cloned etc.) so when there is a mismatch between the loop and predicate it can be detected?

[chhagedorn]

Thanks Roland for having a look. I agree that it is indeed quite fragile. It started out with a quite simple fix but then I found more and more cases with fuzzing where we have some weird in-between states in IGVN while a predicate is being folded where matching failed. I was not super happy with matching predicates during IGVN which is difficult and error-prone to get right.

So I'm wondering if there would be a way to mark predicates as being for a particular loop (maybe storing the loop's node id they apply to in predicate nodes and making sure it's properly updated as loops are cloned etc.) so when there is a mismatch between the loop and predicate it can be detected?

That's an interesting idea that could work more reliably. Let me think about that more.