RFR: 8353290: C2: Refactor PhaseIdealLoop::is_counted_loop() [v17]
Christian Hagedorn
chagedorn at openjdk.org
Mon Oct 27 14:01:03 UTC 2025
On Wed, 22 Oct 2025 19:07:32 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:
>
> mark LoopExitTest::is_valid_with_bt() const
Nice update, it looks very good already! I did a complete pass and left some more comments.
I'm a bit worried about breaking something which might not be noticed because we just don't create a counted loop anymore. Have you thought about some ways to test this? One idea could be to do some runs with some custom logging in place when a counted loop was successfully created and then compare the output to a baseline without your patch and the same logging in place. We should just have some confidence that we do not introduce (performance) regressions.
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?
src/hotspot/share/opto/loopnode.cpp line 1698:
> 1696: _mask = BoolTest(_mask).commute(); // And commute the exit test
> 1697: }
> 1698: if (_phase->is_member(_loop, _phase->get_ctrl(_limit))) { // Limit must be loop-invariant
Here and related loop-(in)variant checks: Could we use `IdealLoopTree::is_invariant()`?
src/hotspot/share/opto/loopnode.cpp line 1709:
> 1707:
> 1708: // Canonicalize the loop condition if it is 'ne'.
> 1709: bool PhaseIdealLoop::LoopExitTest::canonicalize_mask(jlong stride_con) {
You do not seem to use the return value and thus it can be removed.
src/hotspot/share/opto/loopnode.cpp line 1731:
> 1729:
> 1730: // Should not reach
> 1731: return false;
With the assert above for `stride_con` being 1 or -1, do we need this `if` here? Or are you concerned about when this does not hold in product? But since we are not using the return value anyways, I don't think it makes a difference and you could just drop the `if`.
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`?
src/hotspot/share/opto/loopnode.cpp line 1803:
> 1801: _is_valid = false;
> 1802:
> 1803: _back_control = _phase->loop_exit_control(_head, _loop);
You already call `loop_exit_control()` in the constructor. How about doing the following in the constructor instead?
_back_control(_phase->loop_exit_control(_head, _loop))
_exit_test(_back_control, _loop, _phase)
src/hotspot/share/opto/loopnode.cpp line 1821:
> 1819: // if (!_iv_incr.is_valid_with_bt(_iv_bt)) {
> 1820: // return;
> 1821: // }
leftover?
src/hotspot/share/opto/loopnode.cpp line 2141:
> 2139: // If that is not the case, we need to canonicalize the loop exit check by using different values for adjusted_limit
> 2140: // (see LoopStructure::final_limit_correction()).
> 2141: // Note that after canonicalization:
Add some space:
Suggestion:
//
// Note that after canonicalization:
src/hotspot/share/opto/loopnode.cpp line 2405:
> 2403: // }
> 2404: //
> 2405: // If the array is shorter than 0x8000 this exits through a AIOOB
Suggestion:
// If the array is shorter than 0x8000 this exits through an AIOOB
src/hotspot/share/opto/loopnode.cpp line 2412:
> 2410: const TypeInteger* incr_t = igvn.type(_iv_incr.incr())->is_integer(_iv_bt);
> 2411: if (limit_t->hi_as_long() > incr_t->hi_as_long()) {
> 2412: // if the limit can have a higher value than the increment (before the0 phi)
Suggestion:
// if the limit can have a higher value than the increment (before the phi)
src/hotspot/share/opto/loopnode.cpp line 2477:
> 2475: }
> 2476:
> 2477: bool CountedLoopConverter::is_safepoint_invalid(SafePointNode* sfpt) {
Can be made `const`.
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?
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.
src/hotspot/share/opto/loopnode.cpp line 2513:
> 2511: Node* init_trip = _structure.phi()->in(LoopNode::EntryControl);
> 2512: if (_insert_init_trip_limit_check) {
> 2513: Node* cmp_limit = CmpNode::make(init_trip, _structure.exit_test().limit(), _iv_bt);
You also seem to be using `_structure.exit_test().limit()` a lot. We could also provide a `structure.limit()` method instead.
src/hotspot/share/opto/loopnode.cpp line 2559:
> 2557: }
> 2558: _phase->set_subtree_ctrl(adjusted_limit, false);
> 2559: _phase->set_subtree_ctrl(adjusted_limit, false);
Duplicated
Suggestion:
src/hotspot/share/opto/loopnode.cpp line 2615:
> 2613: Node* iffalse = iff->as_If()->proj_out(!(iftrue_op == Op_IfTrue));
> 2614:
> 2615: // Need to swap loop-exit and loop-back control?
Suggestion:
// Need to swap loop-exit and loop-back control?
src/hotspot/share/opto/loopnode.cpp line 2747:
> 2745: // If there is one, then we do not need to create an additional Loop Limit Check Predicate.
> 2746: bool CountedLoopConverter::has_dominating_loop_limit_check(Node* init_trip, Node* limit, const jlong stride_con,
> 2747: const BasicType iv_bt, Node* loop_entry) {
Can be made `const` and indentation is off.
src/hotspot/share/opto/loopnode.hpp line 277:
> 275:
> 276: // Match increment with optional truncation
> 277: class TruncatedIncrement {
You could move this code down to the other loop structure classes.
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.
src/hotspot/share/opto/loopnode.hpp line 2021:
> 2019: PhaseIdealLoop::LoopIVStride _stride;
> 2020: PhiNode* _phi = nullptr;
> 2021: SafePointNode* _sfpt = nullptr;
Can we name it `_safepoint`?
src/hotspot/share/opto/loopnode.hpp line 2062:
> 2060: #ifdef ASSERT
> 2061: bool _checked_for_counted_loop = false;
> 2062: #endif
For single lines, you can use `DEBUG_ONLY()`, same at other places as well in the patch.
Suggestion:
DEBUG_ONLY(bool _checked_for_counted_loop = false;)
src/hotspot/share/opto/loopnode.hpp line 2093:
> 2091: assert(head != nullptr, "");
> 2092: assert(loop != nullptr, "");
> 2093: assert(iv_bt == T_INT || iv_bt == T_LONG, ""); // Loops can be either int or long.
Maybe add an assertion message:
Suggestion:
assert(phase != nullptr, "must be"); // Fail early if mandatory parameters are null.
assert(head != nullptr, "must be");
assert(loop != nullptr, "must be");
assert(iv_bt == T_INT || iv_bt == T_LONG, "either int or long loops");
src/hotspot/share/opto/loopopts.cpp line 4273:
> 4271: // With an extra phi for the candidate iv?
> 4272: // Or the region node is the loop head
> 4273: if (!loop_exit.incr()->is_Phi() || loop_exit.incr()->in(0) == head) {
You seem to query `incr()` many times - might be worth to extract to a separate `loop_incr` variable and reuse it.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24458#pullrequestreview-3383037106
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465494653
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465359232
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465561539
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465569195
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465429129
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465334860
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465349597
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465517176
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465402120
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465401282
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465572745
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465627831
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465648729
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465653480
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465676111
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465706245
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465716571
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465736725
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465343886
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465575237
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465582423
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465327913
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2465756399
More information about the hotspot-compiler-dev
mailing list