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

[Kangcheng Xu]

On Mon, 27 Oct 2025 12:37:10 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   mark LoopExitTest::is_valid_with_bt() const
>
> src/hotspot/share/opto/loopnode.cpp line 1638:
> 
>> 1636: #ifdef ASSERT
>> 1637: void PhaseIdealLoop::check_counted_loop_shape(IdealLoopTree* loop, Node* x, BasicType bt) {
>> 1638:   Node* back_control = loop_exit_control(x, loop);
> 
> How far away are we from just using `LoopStructure` and then `LoopStructure::is_valid()` instead?

Not much, however, `is_valid()` is checking more conditions that are beyond the scope of a proper counted loop shape. I think it's a better idea to keep these `assert` as-is for easier debugging.

> src/hotspot/share/opto/loopnode.cpp line 1764:
> 
>> 1762:   // Get merge point
>> 1763:   _xphi = incr->in(1);
>> 1764:   _node = incr->in(2);
> 
> Should we name `_node` simply `_stride_con`?

Renamed to `_stride_node` to avoid confusion between `jlong stride_con()`

> src/hotspot/share/opto/loopnode.cpp line 2500:
> 
>> 2498:   PhaseIterGVN* igvn = &_phase->igvn();
>> 2499:   Node* init_control = _head->in(LoopNode::EntryControl);
>> 2500:   const jlong stride_con = _structure.stride().compute_non_zero_stride_con(_structure.exit_test().mask(), _iv_bt);
> 
> I've noticed that you use this pattern a few times. How about having a `LoopStructure::stride_con()` method instead?

added `LoopStructure::stride_con()`

> src/hotspot/share/opto/loopnode.cpp line 2505:
> 
>> 2503:     Node* cmp_limit = CmpNode::make(_structure.exit_test().limit(), igvn->integercon((stride_con > 0
>> 2504:                                                               ? max_signed_integer(_iv_bt)
>> 2505:                                                               : min_signed_integer(_iv_bt))
> 
> It might be easier to read when we extract the `intercon()` call to a separate variable in a line above.

Exracted as following:


jlong adjusted_stride_con = (stride_con > 0
                               ? max_signed_integer(_iv_bt)
                               : min_signed_integer(_iv_bt)) - _structure.final_limit_correction();

Node* cmp_limit = CmpNode::make(_structure.limit(), igvn->integercon(adjusted_stride_con, _iv_bt), _iv_bt);

> src/hotspot/share/opto/loopnode.hpp line 1338:
> 
>> 1336:       _back_control(back_control),
>> 1337:       _loop(loop),
>> 1338:       _phase(phase) {}
> 
> Maybe also add an assert here that `back_control` is non-null.

I disagree: `back_control` is not nessarily non-null always. In fact, `loop_exit_control()` could return null even if `head` and `loop` are non-null. This is also why the original code explicitly checks this as well.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2518474056
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2518468727
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2518481511
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2518485338
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2518462909