8356084: C2: Data is wrongly rewired to Initialized Assertion Predicates instead of Template Assertion Predicates

[chhagedorn]

Before the Assertion Predicate refactorings, we rewired data dependencies either to the newly created Initialized Assertion Predicates (for Loop Peeling) or to the zero trip guard (for main and post loops). Both was incomplete when we further split a loop - we missed to update these data dependencies accordingly.

Now that the (almost) complete Assertion Predicate fix is in with JDK-8350577, we are now finally able to fix this by always rewiring the data dependencies to the Template Assertion Predicates which will be kept until either no more loop splitting can be done for a loop or until loop opts are over.

We could have already fixed that with JDK-8350577 but it was simply missed. As an intermediate solution, we always rewired the data dependencies to the Initialized Assertion Predicates which only worked in some cases when the Initialized Assertion Predicates were folded away: They ended up at the Template Assertion Predicates above and from there we could update the data dependencies further. But if that did not happen, we could not find these data dependencies at the Template Assertion Predicates and failed to further update them when the loop was split again. As a result, we could perform some loads too early and crash (not observable, though).

How we could end up with such a crash is described in the newly added regression test testPeelingThreeTimesDataUpdate(). Here is a snippet from the graph after applying Loop Peeling several times without the patch:

All LoadN data dependencies are piled up at an Initialized Assertion Predicate from where we can no longer update them in further loop splitting optimizations because we only look at Template Assertion Predicates for that. By correctly rewiring the data dependencies to Template Assertion Predicates, we fix this which is proposed with this patch.

This was found by a new stress peeling mode (JDK-8355488) @marc-chevalier
is currently working on. I was able to come up with a reproducer that does not use the new stressing but it shows that the new stressing is useful in finding hard to discover bugs.

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-8356084: C2: Data is wrongly rewired to Initialized Assertion Predicates instead of Template Assertion Predicates (Bug - P3)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to 31edbb8f
• Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25007

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[chhagedorn]

Wrongly used the Initialized Assertion Predicate tail instead of the Template Assertion Predicate tail.

[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]

@chhagedorn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8356084: C2: Data is wrongly rewired to Initialized Assertion Predicates instead of Template Assertion Predicates

Reviewed-by: epeter, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 114 new commits pushed to the master branch:

• b21b3a3: 8355528: Update HarfBuzz to 11.2.0
• a6995a3: 8354473: Incorrect results for compress/expand tests with -XX:+EnableX86ECoreOpts
• 9477c42: 8356276: JavaScript error in script.js after JDK-8348282
• ... and 111 more: https://git.openjdk.org/jdk/compare/108078a6813f49fa82b6f97a8a6665d200d95e28...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[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

• 01: Full - Incremental (19ede0a9)
• 00: Full (31edbb8f)

[vnkozlov]

Looks good.

[chhagedorn]

Thanks Vladimir for your review!

[eme64]

Looks reasonable, thanks for fixing this :)

[chhagedorn]

Thanks Emanuel for your review!