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 07:57:43 UTC 2025


On Tue, 21 Oct 2025 07:35:20 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
>
> 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/loopUnswitch.cpp line 523:

> 521:   register_control(region, lp, new_multiversion_slow_proj);
> 522: 
> 523:   // Hook region into slow_path, in stead of the multiversion_slow_proj.

Suggestion:

  // Hook region into slow_path, instead of the multiversion_slow_proj.

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2447056340
PR Review Comment: https://git.openjdk.org/jdk/pull/27892#discussion_r2447097742


More information about the shenandoah-dev mailing list