8370220: C2: rename methods and improve documentation around get_ctrl and idom lazy updating/forwarding of ctrl and idom via dead ctrl nodes #27892

eme64 · 2025-10-20T08:57:33Z

When working on #27889, I was irritated by the lack of documentation and suboptimal naming.

Here, I'm doing the following:

Add more documentation, and improve it in other cases.
Rename "lazy" methods: "lazy" could indicate that we delay it somehow until later, but it is unclear what is delayed.
- lazy_replace -> replace_ctrl_node_and_forward_ctrl_and_idom
- lazy_update -> install_lazy_ctrl_and_idom_forwarding
Made some methods private, and added some additional asserts.

I'd be more than happy for even better names, and suggestions how to improve the documentation further :)

Related issues:
#27889
#15720

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-8370220: C2: rename methods and improve documentation around get_ctrl and idom lazy updating/forwarding of ctrl and idom via dead ctrl nodes (Enhancement - P4)

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27892

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-10-20T08:59:31Z

👋 Welcome back epeter! 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 · 2025-10-20T08:59:53Z

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

8370220: C2: rename methods and improve documentation around get_ctrl and idom lazy updating/forwarding of ctrl and idom via dead ctrl nodes

Reviewed-by: chagedorn, thartmann

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

5ed6c20: 8370681: [BACKOUT] Improve memory ordering in new CPU Time Profiler
7bb490c: 8370318: AES-GCM vector intrinsic may read out of bounds (x86_64, AVX-512)
6f8d07a: 8368500: ContextClassLoader cannot be reset on threads in ForkJoinPool.commonPool()

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2025-10-20T09:00:51Z

@eme64 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 · 2025-10-20T12:58:55Z

Webrevs

chhagedorn

Thanks a lot for following up with a documentation and renaming! Some small suggestions, otherwise, looks good!

Note: There are some build failures in GHA.

src/hotspot/share/opto/loopnode.hpp

chhagedorn · 2025-10-20T13:25:58Z

src/hotspot/share/opto/loopnode.hpp

+  //   reach a live ctrl node. Shorten the path to avoid chasing the
+  //   forwarding in the future.
+  // - When querying "idom": from some node get its old idom, which
+  //   may be dead but has a ctrl forwarding to the new and live


Maybe add this:

Suggested change

// may be dead but has a ctrl forwarding to the new and live

// may be dead but has an idom forwarding (piggy-backing on '_loop_or_ctrl') to the new and live

Thanks for the suggestion. I applied something that is inspired by your suggestion instead :)

chhagedorn · 2025-10-20T13:27:15Z

src/hotspot/share/opto/loopnode.hpp

+  // Using "install_lazy_ctrl_and_idom_forwarding" allows us to only edit
+  // the entry for the old dead node now, and we do not have to update all
+  // the nodes that had the old_node as their "get_ctrl" or "idom". We
+  // clean up the forwarding links when we query "get_ctrl" or "idom".


Suggested change

// clean up the forwarding links when we query "get_ctrl" or "idom".

// clean up the forwarding links when we query "get_ctrl" or "idom" for these nodes the next time.

Applied it but with a line break ;)

src/hotspot/share/opto/loopnode.hpp

chhagedorn · 2025-10-20T13:50:08Z

src/hotspot/share/opto/loopnode.hpp

+      // If everything went right, this dead CFG node should have had a idom/ctrl
+      // forwarding installed, using "install_lazy_ctrl_and_idom_forwarding".
+      // We now have to jump from the old (dead) ctrl node to the new (live)
+      // ctrl/idom node, in possibly multiple ctrl/idom forwarding steps.


Maybe for clarification since it's somehow surprising at first that we reuse _loop_or_ctrl:

Suggested change

// ctrl/idom node, in possibly multiple ctrl/idom forwarding steps.

// idom node, in possibly multiple idom forwarding steps.

// Note that we piggy back on `_loop_or_ctrl` do the the forwarding.

Applied with even more information.

src/hotspot/share/opto/loopnode.hpp

chhagedorn · 2025-10-20T13:53:22Z

src/hotspot/share/opto/loopnode.hpp

+    Node* n = idom_no_update(didx);
+    // We store the found idom in the side-table again. In most cases,
+    // this is a no-op, since we just read from _idom. But in cases where
+    // there was a ctrl forwarding via dead ctrl nodes, this shortens the path.


Suggested change

// there was a ctrl forwarding via dead ctrl nodes, this shortens the path.

// there was an idom forwarding via dead idom nodes, this shortens the path.

chhagedorn · 2025-10-20T13:53:42Z

src/hotspot/share/opto/loopnode.hpp

-  Node *idom(uint didx) const {
-    Node *n = idom_no_update(didx);
-    _idom[didx] = n; // Lazily remove dead CFG nodes from table.
+  Node* idom(uint didx) const {


While at it: Maybe also name this node_index?

I chose to replace it with node_idx, since it comes from n->_idx.

openjdk · 2025-10-21T07:14:08Z

@eme64 hotspot, shenandoah have been added to this pull request based on files touched in new commit(s).

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

src/hotspot/share/opto/loopUnswitch.cpp

TobiHartmann · 2025-10-21T07:51:04Z

src/hotspot/share/opto/loopnode.hpp

+  // Replace the old ctrl node with a new ctrl node.
+  // - Update the node inputs of all uses.
+  // - Lazily update the ctrl and idom info of all uses, via a ctrl/idom forwarding.
+  void replace_ctrl_node_and_forward_ctrl_and_idom(Node *old_node, Node *new_node) {


Drive-by comment (sorry): I think this is way too heavy. Descriptive names are good but for complex methods, not all the semantics can be put into the name but should rather be in a comment.

Couldn't we just name these replace_and_update_ctrl and update_ctrl? I think this entails updating idom which is dependent on ctrl. And the comments explain the details well enough.

@TobiHartmann

update_ctrl: Sounds too much like set_ctrl, but it is not at all similar conceptually.

What about:

install_ctrl_and_idom_forwarding -> forward_ctrl

replace_ctrl_node_and_forward_ctrl_and_idom -> replace_node_and_forward_ctrl

Because it is really a specialization of _igvn.replace_node, so it should have some name similarity.

What do you think?

Yes, that's much better, good with me!

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

eme64 · 2025-10-21T08:21:00Z

@chhagedorn Thanks for all the suggestions / comments, I think I applied them all now :)

@TobiHartmann thanks for the drive-by comment / suggestion.

@rwestrel You are somewhat familiar with this code, would you mind reviewing?

@vnkozlov You may also want to have quick look, just so you are aware of the renamings. But feel free to also review ;)

TobiHartmann

Nice improvement, looks good to me!

chhagedorn

Good naming update! The update looks good to me apart from some last nits.

src/hotspot/share/opto/loopnode.hpp

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

eme64 · 2025-10-21T15:54:30Z

Seems GHA is giving me some failures I did not see in our internal testing:

compiler.lib.ir_framework.flag.FlagVM compiler.gcbarriers.TestShenandoahBarrierExpansion

#  Internal Error (/home/runner/work/jdk/jdk/src/hotspot/share/opto/loopnode.hpp:1168), pid=5884, tid=5904
#  assert(!has_ctrl(old_node) && old_node->is_CFG() && old_node->in(0) == nullptr) failed: must be dead ctrl (CFG) node

This is part of the assert I added. Will have to investigate.

…om/eme64/jdk into JDK-8370220-get-ctrl-documentation

openjdk · 2025-10-22T06:30:00Z

⚠️ @eme64 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

eme64 · 2025-10-22T06:46:21Z

src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp

            fix_memory_uses(u, n, n, c);
          } else if (_phase->C->get_alias_index(u->adr_type()) == _alias) {
-            _phase->lazy_replace(u, n);
+            _phase->igvn().replace_node(u, n);


As far as I can see, the lazy_replace only did igvn.replace_node for non-ctrl nodes anyway. Since we are dealing with PhiNodes here, we might as well only use igvn.replace_node.

I discovered this, because it hit my !old_node->is_CFG() check.

@rwestrel Do you have an opinion on this?

That change looks good to me.

@rwestrel Thank, good to know :)

eme64 · 2025-10-27T10:19:36Z

@chhagedorn @TobiHartmann Would either of you be able to re-approve? I've fixed a little issue for shenandoah, and Roland approved that change. I merged with master and am currently running some last testing.

chhagedorn

Still good!

eme64 · 2025-10-28T06:41:29Z

@chhagedorn @TobiHartmann Thanks for the review / approval!
@rwestrel Thanks for approving the shanandoah changes!

/integrate

openjdk · 2025-10-28T06:42:08Z

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

05ee55e: 8369219: JNI::RegisterNatives causes a memory leak in CodeCache
327b7c3: 8370100: Redundant .png files in Linux app-image cause unnecessary bloat
460a69b: 8017266: Background is painted taller than needed for styled text.
... and 13 more: https://git.openjdk.org/jdk/compare/91e1dcb1083cc8c451d2d169d7f2fdb51c1a158e...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-10-28T06:42:17Z

@eme64 Pushed as commit d5ce666.

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

JDK-8370220

3c03c07

eme64 mentioned this pull request Oct 20, 2025

8369898: C2 SuperWord: assert(has_ctrl(i)) failed: should be control, not loop #27889

Closed

3 tasks

openjdk bot changed the title ~~JDK-8370220~~ 8370220: C2: improve documentation around get_ctrl and _loop_or_ctrl implementation Oct 20, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 20, 2025

eme64 added 4 commits October 20, 2025 14:05

more documentation

d44a9cd

wip documentation and renaming

6e86c12

make helper method private

3bd0435

rename lazy methods

5605adb

eme64 changed the title ~~8370220: C2: improve documentation around get_ctrl and _loop_or_ctrl implementation~~ 8370220: C2: rename methods and improve documentation around get_ctrl and idom lazy updating/forwarding of ctrl and idom via dead ctrl nodes Oct 20, 2025

eme64 added 2 commits October 20, 2025 14:49

missing part

c3b87a8

code style

21911dc

eme64 marked this pull request as ready for review October 20, 2025 12:53

openjdk bot added the rfr Pull request is ready for review label Oct 20, 2025

chhagedorn reviewed Oct 20, 2025

View reviewed changes

eme64 added 2 commits October 21, 2025 08:54

Merge branch 'master' into JDK-8370220-get-ctrl-documentation

c3ab069

fix merge

e0b6ad6

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Oct 21, 2025

eme64 and others added 2 commits October 21, 2025 09:26

for Christian part 1

d9f7926

Apply suggestions from code review

73bb42f

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

TobiHartmann reviewed Oct 21, 2025

View reviewed changes

eme64 and others added 4 commits October 21, 2025 09:52

more for Christian part 2

c5fb615

more for Christian part 3

94950b6

Apply suggestions from code review

a91396b

Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>

renaming for Tobias

aaa91d1

TobiHartmann approved these changes Oct 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 21, 2025

chhagedorn approved these changes Oct 21, 2025

View reviewed changes

src/hotspot/share/opto/loopnode.hpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/loopnode.hpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/loopnode.hpp Outdated Show resolved Hide resolved

Apply suggestions from code review

ac05739

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Oct 21, 2025

eme64 added 2 commits October 22, 2025 08:29

fix shenandoah replace for phis

1488616

Merge branch 'JDK-8370220-get-ctrl-documentation' of https://github.c…

44e808b

…om/eme64/jdk into JDK-8370220-get-ctrl-documentation

eme64 commented Oct 22, 2025

View reviewed changes

Merge branch 'master' into JDK-8370220-get-ctrl-documentation

e4bcb76

chhagedorn approved these changes Oct 27, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 27, 2025

TobiHartmann approved these changes Oct 27, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 28, 2025

openjdk bot closed this Oct 28, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 28, 2025

	// may be dead but has a ctrl forwarding to the new and live
	// may be dead but has an idom forwarding (piggy-backing on '_loop_or_ctrl') to the new and live

	// clean up the forwarding links when we query "get_ctrl" or "idom".
	// clean up the forwarding links when we query "get_ctrl" or "idom" for these nodes the next time.

	// ctrl/idom node, in possibly multiple ctrl/idom forwarding steps.
	// idom node, in possibly multiple idom forwarding steps.
	// Note that we piggy back on `_loop_or_ctrl` do the the forwarding.

	// there was a ctrl forwarding via dead ctrl nodes, this shortens the path.
	// there was an idom forwarding via dead idom nodes, this shortens the path.

Uh oh!

8370220: C2: rename methods and improve documentation around get_ctrl and idom lazy updating/forwarding of ctrl and idom via dead ctrl nodes #27892

8370220: C2: rename methods and improve documentation around get_ctrl and idom lazy updating/forwarding of ctrl and idom via dead ctrl nodes #27892

Uh oh!

Conversation

eme64 commented Oct 20, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 20, 2025

Uh oh!

openjdk bot commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

chhagedorn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chhagedorn Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eme64 commented Oct 21, 2025

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eme64 commented Oct 21, 2025

Uh oh!

openjdk bot commented Oct 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

eme64 commented Oct 20, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 20, 2025 •

edited

Loading

openjdk bot commented Oct 20, 2025 •

edited

Loading

mlbridge bot commented Oct 20, 2025 •

edited

Loading

chhagedorn left a comment •

edited

Loading

chhagedorn Oct 20, 2025 •

edited

Loading