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

[Kangcheng Xu]

On Fri, 10 Oct 2025 08:26:28 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8354383: C2: enable sinking of Type nodes out of loop
>>   
>>   Reviewed-by: chagedorn, thartmann
>>   (cherry picked from commit a2f99fd88bd03337e1ba73b413ffe4e39f3584cf)
>
> src/hotspot/share/opto/loopnode.cpp line 1659:
> 
>> 1657:   assert(exit_test.mask() != BoolTest::ne, "unexpected condition");
>> 1658:   assert(iv_incr.phi_incr() == nullptr, "bad loop shape");
>> 1659:   assert(exit_test.cmp()->in(1) == iv_incr.incr(), "bad exit test shape");
> 
> About these assertion in this method: Aren't these already implicitly checked with the `is_valid*()` checks further up?

No. This is  method is performing more specific checks that normally don't happen (and therefore in `ASSERT` and `assert()`) that are no included in `.is_valid*()`.

> [...] directly initialize `_exit_test` in the constructor of `CountedLoopConverter` by [...]

Do you mean the constructor of `LoopStructure`?

Also `_back_control` is actually just `_phase->loop_exit_control(_head, _loop)` which are already available in `LoopStructure`. I've made those changes to save reinitializations.

> src/hotspot/share/opto/loopnode.cpp line 2460:
> 
>> 2458:   }
>> 2459: 
>> 2460:   return iff->in(0)->isa_SafePoint();
> 
> `isa_SafePoint()` will also return non-null for subclasses of `SafePoint` like `Call` nodes. But IIUC, we want to have only exact `SafePoint` nodes. Maybe @rwestrel can double-check.
> 
> Same in your patch here:
> 
>   _sfpt = _loop->_child == nullptr
>                           ? _phase->find_safepoint(_back_control, _head, _loop)
>                           : _back_control->in(0)->in(0)->isa_SafePoint();

I believe this is a valid concern, although I'm not sure if this ever happens in practice.

Added `sfpt->Opcode() == Op_SafePoint` condition.

> src/hotspot/share/opto/loopnode.cpp line 3113:
> 
>> 3111:     _incr = n1;
>> 3112:     _trunc1 = t1;
>> 3113:     _trunc2 = t2;
> 
> Can we find some better names instead of `trunc1` and `trunc2`?

Renamed to `_outer_trunc` and `_inner_trunc`.


// Optional truncation for: CHAR: (i+1)&0x7fff, BYTE: ((i+1)<<8)>>8, or SHORT: ((i+1)<<16)>>16
Node* outer_trunc() const { return _outer_trunc; } // the outermost truncating node (either the & or the final >>)
Node* inner_trunc() const { return _inner_trunc; } // the inner truncating node, if applicable (the << in a <</>> pair)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2440858361
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2440858412
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2440857836
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2440858015