RFR: 8353290: C2: Refactor PhaseIdealLoop::is_counted_loop() [v2]

[Christian Hagedorn]

On Thu, 10 Apr 2025 15:26:02 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> src/hotspot/share/opto/loopnode.cpp line 313:
>> 
>>> 311: // clone the inner loop etc. No optimizations need to change the outer
>>> 312: // strip mined loop as it is only a skeleton.
>>> 313: IdealLoopTree* PhaseIdealLoop::create_outer_strip_mined_loop(Node *init_control,
>> 
>> Generally, for the touched code, you can also fix the `*` placement to be at the type.
>> Suggestion:
>> 
>> IdealLoopTree* PhaseIdealLoop::create_outer_strip_mined_loop(Node* init_control,
>
> Good point. I tried to fix them as I go but missed this one.

With the `x` -> `head` change, there are some more opportunities :-)

>> src/hotspot/share/opto/loopnode.hpp line 1310:
>> 
>>> 1308:                                                Node*& entry_control, Node*& iffalse);
>>> 1309: 
>>> 1310:   class CountedLoopConverter {
>> 
>> Probably quite a subjective matter, but what about just naming it `CountedLoop`? Then you have `counted_loop.convert()`.
>
> I disagree: I don't want to confuse it with `CountedLoopNode` or making it looks like extending from `IdealLoopTree`. Besides it wouldn't not necessarily guarantee a counted loop until confirmed by `is_counted_loop()`

What I had in mind was something like that (I'm also fine with `CountedLoopConverter`:

CountedLoop counted_loop(...);
counted_loop.convert();
return counted_loop.is_valid();

But maybe you can explain in more detail what the follow-up work will be and how you use this class again later?

>> src/hotspot/share/opto/loopnode.hpp line 1311:
>> 
>>> 1309: 
>>> 1310:   class CountedLoopConverter {
>>> 1311:     PhaseIdealLoop* _phase;
>> 
>> I suggest to add `const` whenever you can. For example, here you will probably not change the `_phase` pointer anymore:
>> Suggestion:
>> 
>>     PhaseIdealLoop* const _phase;
>
> That was my initial thought, too; however, `PhaseIdealLoop::insert_loop_limit_check_predicate()`, `::lazy_replace()`, `::set_subtree_ctrl()` and many other mutations make this impossible.

You might be confusing it with having:

const PhaseIdealLoop* _phase

where we cannot call any non-const methods. That's indeed not possible. But what I mean was to make the pointer const:

PhaseIdealLoop* const _phase;

such that you cannot do `_phase = xyz` later. You would probably not do that anyway but it's an easy addition and safety for all fields not being reassigned again. It also helps to see which fields are going to be updated as part of the mutable state and which fields are not.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2039041842
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2039041754
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2039041966