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

Mon Oct 20 13:59:36 UTC 2025

On Mon, 20 Oct 2025 08:57:33 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> When working on https://github.com/openjdk/jdk/pull/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:
> https://github.com/openjdk/jdk/pull/27889
> https://github.com/openjdk/jdk/pull/15720
> 
> TODO: improve `VerifyLoopOptimizations` to check that we can call `get_ctrl` on all live nodes after loop-opts.

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 line 1080:

> 1078:   }
> 1079: 
> 1080:   // Retreives the ctrl for a data node i.

Suggestion:

  // Retrieves the ctrl for a data node i.

src/hotspot/share/opto/loopnode.hpp line 1125:

> 1123:       // forwarding installed, using "install_lazy_ctrl_and_idom_forwarding".
> 1124:       // We now have to jump from the old (dead) ctrl node to the new (live)
> 1125:       // ctrl node, in possibly multiple ctrl/idom forwarding steps.

I guess in this context of ctrl, you can omit the idom?

Suggestion:

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

src/hotspot/share/opto/loopnode.hpp line 1154:

> 1152:   //   forwarding in the future.
> 1153:   // - When querying "idom": from some node get its old idom, which
> 1154:   //   may be dead but has a ctrl forwarding to the new and live

Maybe add this:
Suggestion:

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

src/hotspot/share/opto/loopnode.hpp line 1160:

> 1158:   // the entry for the old dead node now, and we do not have to update all
> 1159:   // the nodes that had the old_node as their "get_ctrl" or "idom". We
> 1160:   // clean up the forwarding links when we query "get_ctrl" or "idom".

Suggestion:

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

src/hotspot/share/opto/loopnode.hpp line 1161:

> 1159:   // the nodes that had the old_node as their "get_ctrl" or "idom". We
> 1160:   // clean up the forwarding links when we query "get_ctrl" or "idom".
> 1161:   void install_lazy_ctrl_and_idom_forwarding(Node* old_node, Node* new_node) {

Maybe we don't need lazy sice "install forwarding" is already expressive enough:

Suggestion:

  void install_ctrl_and_idom_forwarding(Node* old_node, Node* new_node) {

src/hotspot/share/opto/loopnode.hpp line 1176:

> 1174:   // - Update the node inputs of all uses.
> 1175:   // - Lazily update the ctrl and idom info of all uses, via a ctrl/idom forwarding.
> 1176:   void replace_ctrl_node_and_forward_ctrl_and_idom(Node *old_node, Node *new_node) {

Maybe add here and/or in `install_lazy_ctrl_and_idom_forwarding()` an assert that we have a CFG nodes (i.e. `is_CFG()`) additionally to the `!has_ctrl()` asserts.

src/hotspot/share/opto/loopnode.hpp line 1259:

> 1257:     while (n->in(0) == nullptr) { // Skip dead CFG nodes
> 1258:       // We encountered a dead CFG node.
> 1259:       // If everything went right, this dead CFG node should have had a idom/ctrl

In the context of idom, you might be able to remove "ctrl":
Suggestion:

      // If everything went right, this dead CFG node should have had an idom

src/hotspot/share/opto/loopnode.hpp line 1261:

> 1259:       // If everything went right, this dead CFG node should have had a idom/ctrl
> 1260:       // forwarding installed, using "install_lazy_ctrl_and_idom_forwarding".
> 1261:       // We now have to jump from the old (dead) ctrl node to the new (live)

Suggestion:

      // We now have to jump from the old (dead) idom node to the new (live)

src/hotspot/share/opto/loopnode.hpp line 1262:

> 1260:       // forwarding installed, using "install_lazy_ctrl_and_idom_forwarding".
> 1261:       // We now have to jump from the old (dead) ctrl node to the new (live)
> 1262:       // 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`:
Suggestion:

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

src/hotspot/share/opto/loopnode.hpp line 1272:

> 1270:   Node* idom(Node* d) const {
> 1271:     return idom(d->_idx);
> 1272:   }

While touching it, we might also want to name it `n` for node? `d` could suggest dominator but it's actually the "dominatee".
Suggestion:

  Node* idom(Node* n) const {
    return idom(n->_idx);
  }

src/hotspot/share/opto/loopnode.hpp line 1274:

> 1272:   }
> 1273: 
> 1274:   Node* idom(uint didx) const {

While at it: Maybe also name this `node_index`?

src/hotspot/share/opto/loopnode.hpp line 1278:

> 1276:     // We store the found idom in the side-table again. In most cases,
> 1277:     // this is a no-op, since we just read from _idom. But in cases where
> 1278:     // there was a ctrl forwarding via dead ctrl nodes, this shortens the path.

Suggestion:

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

-------------

PR Review: https://git.openjdk.org/jdk/pull/27892#pullrequestreview-3356437939
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445002972
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445012015
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445018079
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445021875
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445023706
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2444994098
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445049053
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445055275
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445083835
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445090585
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445094889
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2445093928