8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences

[dlunde]

Issue Summary

The current documentation for PhaseCFG::insert_anti_dependences is difficult to follow and sometimes even misleading. We should ensure the method is appropriately documented.

Changeset

• Rename PhaseCFG::insert_anti_dependences to PhaseCFG::raise_above_anti_dependences. The purpose of PhaseCFG::raise_above_anti_dependences is twofold: raise the load's LCA so that the load is scheduled before anti-dependent stores, and if necessary add anti-dependence edges between the load and certain anti-dependent stores (to ensure we later "raise" the load before anti-dependent stores in LCM). The name PhaseCFG::insert_anti_dependences suggests that we only add anti-dependence edges. The name PhaseCFG::raise_above_anti_dependences, therefore, seems more appropriate.
• Significantly add to and revise the source code documentation of PhaseCFG::raise_above_anti_dependences.
• Add, move, and revise asserts in PhaseCFG::raise_above_anti_dependences, including improved assert messages in a few places.
• In the main worklist loop of PhaseCFG::raise_above_anti_dependences:
  • Clean up how we identify the search root (avoid mutation).
  • Add a missing early exit for Phi nodes when LCA == early.

Testing

• GitHub Actions
• tier1 to tier4 (and additional Oracle-internal testing) on Windows x64, Linux x64, Linux aarch64, macOS x64, and macOS aarch64.

---

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-8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences (Enhancement - P4)

Reviewers

• Roberto Castañeda Lozano (@robcasloz - Reviewer)
• Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24926

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back dlunden! 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]

@dlunde 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:

8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences

Reviewed-by: rcastanedalo, chagedorn

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 315 new commits pushed to the master branch:

• 883e52a: 8353237: [AArch64] Incorrect result of VectorizedHashCode intrinsic on Cortex-A53
• 13b3541: 8356866: Cleanup hotspot/jtreg/ProblemList.txt
• b8d2bdb: 8355743: G1: Collection set clearing is not recorded as part of "Free Collection Set Time"
• ... and 312 more: https://git.openjdk.org/jdk/compare/b0c3485d6c911898b1ac07eccfe7d2b2668144cc...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]

@dlunde 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

• 06: Full - Incremental (aec59a15)
• 05: Full - Incremental (11e37390)
• 04: Full - Incremental (a4a6a778)
• 03: Full - Incremental (e4fb8a0d)
• 02: Full - Incremental (1ba0dcff)
• 01: Full - Incremental (3d11b554)
• 00: Full (eceb18e8)

[galderz]

Thanks @dlunde for the PR. Some small comments.

[robcasloz]

Thanks for doing this, Daniel! insert_anti_dependences is indeed easier to understand after your proposed cleanups and additional comments. I have a few questions and suggestions.

Please update also the reference to insert_anti_dependencies in src/hotspot/share/adlc/output_h.cpp.

[dlunde]

Thanks for doing this, Daniel! insert_anti_dependences is indeed easier to understand after your proposed cleanups and additional comments. I have a few questions and suggestions.

Thanks for the review @robcasloz!

Please update also the reference to insert_anti_dependencies in src/hotspot/share/adlc/output_h.cpp.

Good catch, I only grepped for insert_anti_dependences 🙂

[robcasloz]

Thanks for addressing my comments, looks good! I just have an (optional) suggestion (https://github.com/openjdk/jdk/pull/24926/files#r2081235028).

[robcasloz]

Looks good (modulo the grammar glitch), thanks!

[chhagedorn]

Nice documentation! A lot of minor comments/suggestions while reading through the comments in the entire method.

[chhagedorn]

Thanks for doing all the updates, looks good to me now!

[dlunde]

Thanks for the reviews @robcasloz and @chhagedorn! Only documentation changes since the initial PR commit, and GHA is clean after aec59a1. Integrating now.

/integrate

[openjdk]

Going to push as commit 5cb2317.
Since your change was applied there have been 316 commits pushed to the master branch:

• 1d36f17: 8351232: NPE: Cannot invoke "getDeclarationAttributes" because "sym" is null
• 883e52a: 8353237: [AArch64] Incorrect result of VectorizedHashCode intrinsic on Cortex-A53
• 13b3541: 8356866: Cleanup hotspot/jtreg/ProblemList.txt
• ... and 313 more: https://git.openjdk.org/jdk/compare/b0c3485d6c911898b1ac07eccfe7d2b2668144cc...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dlunde Pushed as commit 5cb2317.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.