RFR: 8370220: C2: rename methods and improve documentation around get_ctrl and idom lazy updating/forwarding of ctrl and idom via dead ctrl nodes [v5]
Tobias Hartmann
thartmann at openjdk.org
Tue Oct 21 08:11:11 UTC 2025
On Tue, 21 Oct 2025 08:06:27 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> 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) {
>>
>> 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!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2447155999
More information about the shenandoah-dev
mailing list