RFR: 8353290: C2: Refactor PhaseIdealLoop::is_counted_loop()

[Christian Hagedorn]

On Fri, 4 Apr 2025 20:33:47 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

> This PR refactors `PhaseIdealLoop::is_counted_loop()` into (mostly) `CountedLoopConverter::is_counted_loop()` and `CountedLoopConverter::convert()` to decouple to detection and conversion code. This enables us to try different loop configurations easy and finally convert once a counted loop is found. 
> 
> A nested `PhaseIdealLoop::CountedLoopConverter` class is created to handle the context, but I'm not if this is the best name or place for it. Please let me know what you think.
> 
> Blocks [JDK-8336759](https://bugs.openjdk.org/browse/JDK-8336759).

That's a good idea to refactor this, thanks for tackling it! Some initial general comments, I will have a closer look at your patch later again.

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,

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.

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.

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

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;

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.

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?

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

PR Review: https://git.openjdk.org/jdk/pull/24458#pullrequestreview-2755561053
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2036648877
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2036651203
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2036660181
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2036665370
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2036666689
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2036668900
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2036673872