RFR: 8343148: C2: Refactor uses of "PhaseValue::*con*() + PhaseIdealLoop::set_ctrl()" into separate method [v4]

[Vladimir Kozlov]

On Fri, 8 Nov 2024 14:55:53 GMT, theoweidmannoracle <duke at openjdk.org> wrote:

>> This patch introduces the methods `PhaseIdealLoop::intcon` and `PhaseIdealLoop::longcon` which are wrappers for:
>> 
>> 
>> ConINode* node = _igvn.intcon(i);
>> set_ctrl(node, C->root());
>> 
>> 
>> and
>> 
>> 
>> ConLNode* node = _igvn.longcon(i);
>> set_ctrl(node, C->root());
>> 
>> 
>> Occurrences of this pattern in loopnode.cpp were replaced with the appropriate call to the new methods.
>
> theoweidmannoracle has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add set_root_as_ctrl

Looks good. I have few additional comments.

src/hotspot/share/opto/loopnode.cpp line 3147:

> 3145:   ConINode* zero = igvn->intcon(0);
> 3146:   if (iloop != nullptr) {
> 3147:     iloop->set_root_as_ctrl(zero);

Please look on history of this code. This is suspicious - constant nodes should be always attached to Root.

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

> 994:   }
> 995:   void set_root_as_ctrl(Node* n) {
> 996:     assert( !has_node(n) || has_ctrl(n), "" );

We don't use spaces after and before `()` in assert(). Ignore old style in previous lines.

src/hotspot/share/opto/loopopts.cpp line 195:

> 193:       set_root_as_ctrl(x);
> 194:       continue;
> 195:     }

This looks like "band-aid" - this should be assert. May be investigate in separate RFE.

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

PR Review: https://git.openjdk.org/jdk/pull/21836#pullrequestreview-2424513220
PR Review Comment: https://git.openjdk.org/jdk/pull/21836#discussion_r1834825217
PR Review Comment: https://git.openjdk.org/jdk/pull/21836#discussion_r1834821279
PR Review Comment: https://git.openjdk.org/jdk/pull/21836#discussion_r1834827466