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

Christian Hagedorn chagedorn at openjdk.org
Tue Nov 25 15:31:49 UTC 2025 

On Fri, 21 Nov 2025 15:54:08 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 incrementally with one additional commit since the last revision:
> 
>   fix trip counter loop-variant detection

Thanks for the updates! I first did some skimming first but ended up doing another complete pass. It's already in a good state but I still found some more small things which I left as comments. But then I think it's good to go from my side (regarding the code) :-)

About testing: 

Thanks for doing an extended testing with directly inserting the old code again to have a proper comparison. I first thought it's going to be too tricky which is why I proposed a logging - but I already feared that it's not going to be stable enough. So, I'm glad that you managed to do a old vs. new state!

For next steps, I suggest I'll give your patch a spin in our standard testing once you addressed the last comments in this badge. Then I'm also happy to run some more extended testing with your old vs. new counted loop transformation state (would be nice if you can update your branch with the latest review and also merge in latest master).

Let me know if you need some help :-)

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

> 1654:   assert(phi != nullptr && phi->in(LoopNode::LoopBackControl) == iv_incr.incr(), "No phi");
> 1655: 
> 1656:   assert(stride.compute_non_zero_stride_con(exit_test.mask(), bt), "illegal condition");

We should be explicit here:
Suggestion:

  assert(stride.compute_non_zero_stride_con(exit_test.mask(), bt) != 0, "illegal condition");

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

> 1684:   }
> 1685: 
> 1686:   // Find the trip-counter increment & limit.  Limit must be loop invariant.

Suggestion:

  // Find the trip-counter increment & limit. Limit must be loop invariant.

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

> 1692:   // ---------
> 1693: 
> 1694:   if (!_phase->ctrl_is_member(_loop, _incr)) { // Swapped trip counter and limit?

We can probably also use `is_invariant()` here

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

> 1762:     }
> 1763: 
> 1764:     swap(_xphi, _stride_node);        // 'incr' is commutative, so ok to swap

Comment indentation is off.
Suggestion:

  if (!_stride_node->is_Con()) { // Oops, swap these
    if (!_xphi->is_Con()) {      // Is the other guy a constant?
      return;                    // Nope, unknown stride, bail out
    }
    swap(_xphi, _stride_node); // 'incr' is commutative, so ok to swap

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

> 1830:   while (xphi->Opcode() == Op_Cast(_iv_bt)) {
> 1831:     xphi = xphi->in(1);
> 1832:   }

I'm wondering if this should be part of the `xphi` computation in `LoopIVStride`. Or in other words: Do the other use-sites of `xphi()` do not need this uncast logic? Maybe @rwestrel knows more.

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

> 1841:   Node* sfpt = _back_control->in(0)->in(0);
> 1842:   if (_loop->_child != nullptr && sfpt->Opcode() == Op_SafePoint) {
> 1843:     _safepoint = sfpt->as_SafePoint();

For consistency:
Suggestion:

  Node* safepoint = _back_control->in(0)->in(0);
  if (_loop->_child != nullptr && safepoint->Opcode() == Op_SafePoint) {
    _safepoint = safepoint->as_SafePoint();

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

> 2033: 
> 2034:   // Check trip counter will end up higher than the limit
> 2035:   const TypeInteger* limit_t = igvn->type(_structure.limit())->is_integer(_iv_bt);

Looks like this could now be moved into the only use in `is_infinite_loop()` directly, so you do not need to pass it into as argument. But I see that you reuse it again later in this method. I would have probably still moved it inside `is_infinite_loop()` and re-fetched it further down again. But I leave it up to you to decide :-)

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

> 2250:   //            again and can skip the predicate.
> 2251: 
> 2252:   int sov = check_stride_overflow(_structure.final_limit_correction(), limit_t, _iv_bt);

I suggest to rename it to `stride_overflow_state` or something like that since `sov` is a rather non-intuitive abbreviation. 

The best thing is probably to turn this into a proper enum since the states -1, 0, and 1 are not that easy to comprehend. I leave it up to you if you also want to do this in this PR - minor detail.

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

> 2392:     //    i++;
> 2393:     //    i = i && 0x7fff;
> 2394:     //  }

Somehow indentation seems off:
Suggestion:

    // while (true) {
    //   sum + = array[i];
    //   i++;
    //   i = i && 0x7fff;
    // }

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

> 2546:       mask = BoolTest::gt;
> 2547:     else
> 2548:       ShouldNotReachHere();

We should also add braces here:
Suggestion:

    if (mask == BoolTest::le) {
      mask = BoolTest::lt;
    } else if (mask == BoolTest::ge) {
      mask = BoolTest::gt;
    } else {
      ShouldNotReachHere();
    }

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

> 2570:     nphi = igvn->register_new_node_with_optimizer(nphi);
> 2571:     _phase->set_ctrl(nphi, _phase->get_ctrl(phi));
> 2572:     igvn->replace_node(_structure.phi(), nphi);

You can use `phi` instead of fetching it again with `_structure.phi()`:
Suggestion:

  Node* phi = _structure.phi();
  if (!TypeInteger::bottom(_iv_bt)->higher_equal(phi->bottom_type())) {
    Node* nphi =
        PhiNode::make(phi->in(0), phi->in(LoopNode::EntryControl), TypeInteger::bottom(_iv_bt));
    nphi->set_req(LoopNode::LoopBackControl, phi->in(LoopNode::LoopBackControl));
    nphi = igvn->register_new_node_with_optimizer(nphi);
    _phase->set_ctrl(nphi, _phase->get_ctrl(phi));
    igvn->replace_node(phi, nphi);
    phi = nphi->as_Phi();
  }

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

> 2591: 
> 2592:   // Replace the old IfNode with a new LoopEndNode
> 2593:   Node* lex = igvn->register_new_node_with_optimizer(BaseCountedLoopEndNode::make(iff->in(0),

It's somewhat difficult to follow the logic with the different abbreviations, some referring to the old loop exit and some to the newly created one. Maybe you can improve the naming here by making it more clear what belongs to what. But we could also do that separately at some point since it was like that before and the refactoring has already become quite large :-)

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

> 2637:       _structure.sfpt() != nullptr &&
> 2638:       !_loop->_has_call &&
> 2639:       _phase->is_deleteable_safept(_structure.sfpt());

For me, the old indentation was easier to read.

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

> 2705: #endif
> 2706: 
> 2707:   _phase->C->print_method(PHASE_AFTER_CLOOPS, 3, l);

I was thinking about moving this below setting the phi type on the next lines to have this information already available in the IGV dump. I guess you could squeeze that change in here as well.

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

> 3065: }
> 3066: 
> 3067: //=============================================================================

I guess this can also be removed
Suggestion:

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

> 5472: volatile int PhaseIdealLoop::_long_loop_nests=0; // Number of long loops successfully transformed to a nest
> 5473: volatile int CountedLoopConverter::_long_loop_counted_loops = 0; // Number of long loops successfully transformed to a
> 5474:                                                                  // counted loop

I suggest to move the comment above, then it fits on one line:
Suggestion:

// Number of long loops successfully transformed to a counted loop
volatile int CountedLoopConverter::_long_loop_counted_loops = 0;

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

> 4280:     }
> 4281: 
> 4282:     Node* loop_incr = loop_exit.incr();

Can even be made `const`:

Suggestion:

    const Node* loop_incr = loop_exit.incr();

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

PR Review: https://git.openjdk.org/jdk/pull/24458#pullrequestreview-3505053490
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560032114
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560052236
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2559970421
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560105572
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560128739
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560135552
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560179453
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560227254
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560183704
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560308736
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560323023
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560380302
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560347353
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560387712
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560064816
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560393952
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2560001309

More information about the hotspot-compiler-dev mailing list