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

[Kangcheng Xu]

On Thu, 10 Apr 2025 07:03:20 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   reviewer suggested changes
>
> 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.

> src/hotspot/share/opto/loopnode.cpp line 367:
> 
>> 365: }
>> 366: 
>> 367: Node* PhaseIdealLoop::loop_exit_control(const Node* x, const IdealLoopTree* loop) const {
> 
> There are quite some places where we use `x` to denote a `loop_head`. Maybe you can also fix that with this patch.

I would prefer a simple `head` for function named `.*loop.*`. Updated all relevant functions I touched.

> src/hotspot/share/opto/loopnode.cpp line 1465:
> 
>> 1463:   assert(back_control != nullptr, "no back control");
>> 1464: 
>> 1465:   LoopExitTest exit_test = loop_exit_test(back_control, loop);
> 
> Just an idea: Would it make sense to have a single `LoopStructure` (or another name) class that contains all the information stored now with separate structs `LoopExitTest`, `LoopIvIncr` and `LoopIvStride`? Then we could offer query methods for validation like `is_valid()`, `is_stride_valid()` etc., or check for existence like `has_stride()`, or access specific nodes like `stride()`, `incr()` etc.

Let me see what I can do! :)

> 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()`

> 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 makes this impossible.

> src/hotspot/share/opto/loopnode.hpp line 1331:
> 
>> 1329:     bool _includes_limit;
>> 1330:     BoolTest::mask _mask;
>> 1331:     Node* _increment;
> 
> Here you have `(_phi_)incr` and `increment`. I suggest to make the naming consistent. I personally prefer full names whenever we can if it's not a universal abbreviation like `cmp` or `iv`. But what we count as well-known abbreviation is debatable and also depends on personal taste.

Updated `s/_phi_incr/_phi_increment`

> src/hotspot/share/opto/loopnode.hpp line 1336:
> 
>> 1334:     Node* _sfpt;
>> 1335:     jlong _final_correction;
>> 1336:     Node* _trunc1;
> 
> `trunc1` is a little hard to understand. Can we find a better name?

Droped `Node* _trunc1` in favour of `TypeInteger* _increment_truncation_type`. Added a note in `should_stress_long_counted_loop()`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2037682520
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2037682664
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2037682831
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2037683322
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2037683588
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2037683863
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2037684011