8360510: C2: Template Assertion Predicates are not cloned to the inner counted loop with -XX:+StressDuplicateBackedge

[chhagedorn]

Strong Connection between Template Assertion Predicate and Counted Loop

In JDK-8350579, we fixed the issue that a Template Assertion Predicate for a folded loop A could end up at another loop B. We then created an Initialized Assertion Predicate at loop B from the template of loop A and used the values from the already folded, completely unrelated loop A . As a result, we crashed with a halt because loop B violated the predicate with the wrong values. As a fix, we established a strong connection between Template Assertion Predicates and their associated loop node by adding a direct link from OpaqueTemplateAssertionPredicate -> CountedLoop.

Maintaining this Property

In PhaseIdealLoop::eliminate_useless_predicates(), we walk through all counted loops and only keep those OpaqueTemplateAssertionPredicate nodes that can be found from the loop heads and are actually meant for this loop (using the strong connection):

jdk/src/hotspot/share/opto/predicates.cpp

Lines 1245 to 1249 in d2926df

	OpaqueTemplateAssertionPredicateNode* opaque_node = template_assertion_predicate.opaque_node();
	if (opaque_node->loop_node() == _loop_node) {
	// We actually mark the associated OpaqueTemplateAssertionPredicate node useful.
	template_assertion_predicate.opaque_node()->mark_useful();
	}

All other opaque nodes are removed.

Additional Verification for Useless OpaqueTemplateAssertionPredicate Nodes

As an additional verification for OpaqueTemplateAssertionPredicate nodes that are found to be useless in eliminate_useless_predicates(), we check that in this case the CountedLoop is really dead (otherwise, we should have found the OpaqueTemplateAssertionPredicate in our walks through all loop):

jdk/src/hotspot/share/opto/predicates.cpp

Lines 1294 to 1301 in d2926df

	for (LoopTreeIterator iterator(_ltree_root); !iterator.done(); iterator.next()) {
	IdealLoopTree* loop = iterator.current();
	Node* loop_head = loop->head();
	if (loop_head->is_CountedLoop()) {
	assert(!loop_nodes_of_useless_template_assertion_predicates.member(loop_head),
	"CountedLoopNode should be dead when found in OpaqueTemplateAssertionPredicateNode being marked useless");
	}
	}

Violating the Additional Verification with -XX:+StressLoopBackedge

In PhaseIdealLoop::duplicate_loop_backedge(), we convert a loop with a merge point into two loops which should enable us to transform the new inner loop into a counted loop. This only makes sense for a Loop that is not a counted loop, yet. However, to stress the transformation, we can also run with -XX:+StressDuplicateBackedge that also transforms a counted loop into an inner and an outer loop. This is a problem when we have Template Assertion Predicates above a counted loop to be stressed:

After duplicate backedge, the Template Assertion Predicates are now at the outer non-counted Loop:

In eliminate_useless_predicates(), we then no longer find these Template Assertion Predicates when walking up from 275 CountedLoop. But since the counted loop is still in the graph, the additional verification above fails when checking that a useless Template Assertion Predicate is associated with a dead counted loop - which is not the case.

Solution

The solution I propose is to clone the Template Assertion Predicates to the inner counted loop. This can be guarded with an ifdef ASSERT because it can only happen with StressLoopBackedge which is a develop flag. This is straight forward and solves this "opaque <-> counted loop" mismatching problem.

Additional Changes

• When working on this change, I noticed that the regression test for checking that data control dependencies are correctly updated with Template Assertion Predicates in TestAssertionPredicates.java was no longer working (i.e. disabling TemplateAssertionPredicate::rewire_loop_data_dependencies() did not crash). It only triggers when running with ZGC (i.e. produce a crash when disabling rewire_loop_data_dependencies()). I added an additional jtreg block with that flag setup.
• I added an ifdef ASSERT block for some code that is only executed for StressDuplicateBackedge.
• I ran this patch through t1-4 + hs-precheckin-comp + hs-comp-stress, once without -XX:-StressDuplicateBackedge and once with. In the latter run, I found that TestVerifyLoopOptimizationsHitsMemLimit.java hit the memory limit. This seems expected since we create more loop nodes which results in more verification work. The test already uses quite some memory when run with VerifyLoopOptimizations. We now add some more on top which will reach the limit of 100M set for the test. I propose to just disable this test with StressDuplicateBackedge. Note that this also fails before this patch.

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-8360510: C2: Template Assertion Predicates are not cloned to the inner counted loop with -XX:+StressDuplicateBackedge (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28389

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28389.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 (36fa175f)

[rwestrel]

Do we need to clone the template predicates for correctness as well? Maybe pre/main/post loops are created next then unrolling happens?
Otherwise another way to fix this may have been to replace the CountedLoop with a Loop and let the next round of loop opts create a new CountedLoop
Where do the template predicates branch to when they fail? A predicate uncommon trap?

[chhagedorn]

Thanks for having a look!

Do we need to clone the template predicates for correctness as well? Maybe pre/main/post loops are created next then unrolling happens?

Yes, I think so. We could probably come up with some case where we would also hit correctness issues down the road with more loop opts to be applied similar to the cases we have in TestAssertionPredicates.java.

Otherwise another way to fix this may have been to replace the CountedLoop with a Loop and let the next round of loop opts create a new CountedLoop

Which would mean that this is not really an option: When we convert back to a Loop, we will remove the Template Assertion Predicates.

Where do the template predicates branch to when they fail? A predicate uncommon trap?

Not sure if I understand your question. The Template Assertion Predicates themselves are never executed and just serve as templates to create Initialized Assertion Predicates from which will result in a halt if they fail at runtime.