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]
Emanuel Peter
epeter at openjdk.org
Tue Oct 21 08:11:10 UTC 2025
On Tue, 21 Oct 2025 07:51:04 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:
>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Apply suggestions from code review
>>
>> Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>
> 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?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2447148523
More information about the shenandoah-dev
mailing list