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

Christian Hagedorn chagedorn at openjdk.org
Fri Sep 5 14:56:18 UTC 2025 

On Tue, 26 Aug 2025 14:47:00 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 the detection and conversion code. This enables us to try different loop configurations easily 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).
>
> Kangcheng Xu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits:
> 
>  - Merge branch 'openjdk:master' into counted-loop-refactor
>  - Merge remote-tracking branch 'origin/master' into counted-loop-refactor
>    
>    # Conflicts:
>    #	src/hotspot/share/opto/loopnode.cpp
>    #	src/hotspot/share/opto/loopnode.hpp
>  - Merge branch 'master' into counted-loop-refactor
>    
>    # Conflicts:
>    #	src/hotspot/share/opto/loopnode.cpp
>    #	src/hotspot/share/opto/loopnode.hpp
>    #	src/hotspot/share/opto/loopopts.cpp
>  - Merge remote-tracking branch 'origin/master' into counted-loop-refactor
>  - further refactor is_counted_loop() by extracting functions
>  - WIP: refactor is_counted_loop()
>  - WIP: refactor is_counted_loop()
>  - WIP: review followups
>  - reviewer suggested changes
>  - line break
>  - ... and 14 more: https://git.openjdk.org/jdk/compare/173dedfb...763adeda

Hi @tabjy, sorry for letting you wait! I was very busy with other things. 

Thanks for coming back with an improved version! This looks much better already. I had a first over-viewing look. Will dive into it more again next week. I've just left some thoughts/comments here and there. Generally, I think we could improve on the classes to not just make them pure data holders with public access but actually allow users to call methods to interact with the data that we could hide to prevent modification. Let me know what you think :-)

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

> 440: }
> 441: 
> 442: PhaseIdealLoop::LoopExitTest PhaseIdealLoop::loop_exit_test(const Node* back_control, const IdealLoopTree* loop) {

Just an idea here: Could this also be part of `LoopExitTest` instead? Then a user could do something like:

LoopExitTest loop_exit_test(...);
 // i.e. = PhaseIdealLoop::loop_exit_test() but with a `_is_valid` flag. Then at the end you 
 // can also check for the right Cmp opcode and that it's not null which the current caller of 
 // loop_exit_test() are all doing. If that's off, you set `_is_valid` to false accordingly.
loop_exit_test.build();
if (loop_exit_test.is_not_valid()) {
...
}


The same also applies for the other classes like `LoopIVIncr`, `LoopIVStride` etc.

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

> 1879:   PhaseIterGVN* igvn = &_phase->igvn();
> 1880: 
> 1881:   LoopStructure structure{};

I think you can remove the `{}`:
Suggestion:

  LoopStructure structure;

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

> 2256: }
> 2257: 
> 2258: bool CountedLoopConverter::build_loop_structure(CountedLoopConverter::LoopStructure& structure) {

Suggestion:

bool CountedLoopConverter::build_loop_structure(LoopStructure& structure) {

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

> 2257: 
> 2258: bool CountedLoopConverter::build_loop_structure(CountedLoopConverter::LoopStructure& structure) {
> 2259:   PhaseIterGVN* igvn = &_phase->igvn();

Not used anymore and can be removed

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

> 2264:   }
> 2265: 
> 2266:   PhaseIdealLoop::LoopExitTest exit_test = _phase->loop_exit_test(back_control, _loop);

Some thoughts/suggestions here:
- The method is still big and you need a moment to figure out what's going on/what checks we do.
- It looks like you are only initializing fields of `LoopStructure`. Couldn't you move the method to this class?
- You could have a separate field `_is_valid` in `LoopStructure`, then you could remove the `bool` return. I.e. this would then look something like this:

LoopStructure loop_structure;
loop_structure.build();
if (loop_structure.is_not_valid()) {
  return false;
}

You might need to pass in some additional info like `phase` to `LoopStructure` but I think that's okay.
- When doing the thing above, then you can just step by step assign the fields as you go and as soon as something is off (i.e. not a counted loop anymore), you set `_is_valid` to false and stop parsing further. This would allow you to further split the method up which also improves documentation and moves field specific things to separate initializer methods:

back_control = _phase->loop_exit_control(_head, _loop);
if (back_control == nullptr) {
  _is_valid = false;
  return false;
}
exit_test = exit_test();
if (exit_test.is_not_valid()) {
  _is_valid = false;
  return;
}
incr = incr();
iv_incr = PhaseIdealLoop::loop_iv_incr(incr, _head, _loop);
....

- Btw, you should use a `_` prefix for the fields.

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

> 2327:   structure.phi = phi;
> 2328: 
> 2329:   structure.sfpt = sfpt;

Are you later really going to use all these fields? I haven't double-checked. Another note here: I think it would be better to hide the fields and provide accessor methods. Otherwise, everyone can update them.

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

> 1325:   static PhiNode* loop_iv_phi(const Node* xphi, const Node* phi_incr, const Node* head);
> 1326: 
> 1327:   bool try_convert_to_counted_loop(Node* head, IdealLoopTree*&loop, const BasicType iv_bt);

Suggestion:

  bool try_convert_to_counted_loop(Node* head, IdealLoopTree*& loop, const BasicType iv_bt);

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

PR Review: https://git.openjdk.org/jdk/pull/24458#pullrequestreview-3189442899
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2325295897
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2325148526
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2325150664
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2325150111
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2325253187
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2325256932
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2325264509

More information about the hotspot-compiler-dev mailing list