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

[Kangcheng Xu]

On Tue, 25 Nov 2025 14:22:59 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix trip counter loop-variant detection
>
> 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 :-)

moved to `is_infinite_loop()`

> 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.

Create enum `StrideOverflowState.Overflow`, `.NoOverflow`, and `.RequireLimitCheck`

> 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 :-)

Renamed to `new_cmp`, `new_test`, `loop_end` and `loop_end_exit`. Added coresponding comments.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2569804636
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2569806048
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2569807660