RFR: 8353290: C2: Refactor PhaseIdealLoop::is_counted_loop() [v13]
Christian Hagedorn
chagedorn at openjdk.org
Fri Oct 10 10:11:07 UTC 2025
On Wed, 1 Oct 2025 15:23:33 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:
>
> 8354383: C2: enable sinking of Type nodes out of loop
>
> Reviewed-by: chagedorn, thartmann
> (cherry picked from commit a2f99fd88bd03337e1ba73b413ffe4e39f3584cf)
Thanks Kangcheng for coming back with an update and addressing my suggestion! It already looks much better! 😊
I did another, not yet complete, pass and left some more comments. Happy to come back again to this next week but I think I now need a break from reviewing :-)
src/hotspot/share/opto/loopnode.cpp line 380:
> 378:
> 379: void CountedLoopConverter::insert_loop_limit_check_predicate(const ParsePredicateSuccessProj* loop_limit_check_parse_proj,
> 380: Node* cmp_limit, Node* bol) {
Can be made `const` and indentation is off here for the second line.
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?
src/hotspot/share/opto/loopnode.cpp line 1666:
> 1664: #endif
> 1665:
> 1666: //------------------------------Counted Loop Structures-----------------------------
We used to add these headers in the early days (maybe because IDEs were not that powerful?). I think you can remove these nowadays.
Suggestion:
src/hotspot/share/opto/loopnode.cpp line 1667:
> 1665:
> 1666: //------------------------------Counted Loop Structures-----------------------------
> 1667: bool PhaseIdealLoop::LoopExitTest::build() {
You don't seem to use the return value since you only check with `is_valid_with_bt()` afterwards. I suggest to turn this into `void`. Same with some other `build()` returns where we only check `valid()` and don't use the actual return value of `build()`. Maybe double-check them all.
src/hotspot/share/opto/loopnode.cpp line 1693:
> 1691:
> 1692: if (!_phase->is_member(_loop, _phase->get_ctrl(_incr))) { // Swapped trip counter and limit?
> 1693: swap(_incr, _limit); // Then reverse order into the CmpI
Suggestion:
swap(_incr, _limit); // Then reverse order into the CmpI
src/hotspot/share/opto/loopnode.cpp line 1741:
> 1739: _incr = back_control;
> 1740: _phi_incr = phi_incr;
> 1741:
Suggestion:
src/hotspot/share/opto/loopnode.cpp line 1750:
> 1748:
> 1749: _is_valid = true;
> 1750: return true;
In the old code, we returned a `nullptr` from `loop_iv_incr()` and then bailed out in `is_counted_loop()`. But here we seem to set `_incr` regardless and also set `_is_valid` to true. This seems incorrect.
src/hotspot/share/opto/loopnode.cpp line 1801:
> 1799: }
> 1800:
> 1801: _exit_test = PhaseIdealLoop::LoopExitTest(_back_control, _loop, _phase);
It's probably not that bad but here we basically recreate the `LoopExitTest`. Instead of having a default constructor, you could directly initialize `_exit_test` in the constructor of `CountedLoopConverter` by passing only `_loop` and `_phase` and the pass `_back_control` with `_exit_test.build()`. You could do the same for the other classes like `LoopIVIncr`. This could save some reinitializations - it's probably minor but makes `LoopStructure::build()` a little easier to read.
src/hotspot/share/opto/loopnode.cpp line 1814:
> 1812: _iv_incr = PhaseIdealLoop::LoopIVIncr(incr, _head, _loop);
> 1813: _iv_incr.build();
> 1814: if (_iv_incr.incr() == nullptr) {
Why don't you also check with a `is_valid()` method here?
src/hotspot/share/opto/loopnode.cpp line 1843:
> 1841: (_truncated_increment.trunc1() != nullptr && _phi->in(LoopNode::LoopBackControl) != _truncated_increment.trunc1())) {
> 1842: return false;
> 1843: }
Suggestion:
}
src/hotspot/share/opto/loopnode.cpp line 1884:
> 1882: }
> 1883:
> 1884: // Trip-counter increment must be commutative & associative.
This comment did not really make sense. I checked its history and it started to be misplaced here:
https://github.com/openjdk/jdk/commit/baaa8f79ed93d4dc1444fed81599ab0f7c2dd395#diff-dc3fdd0572cfc2cb65bce10f08db4054dbaf1b3b94f8ad7883f6c120b4773cfeR332-R342
I suggest to move the comment again to the right place in your patch in `LoopIVIncr::build()`.
src/hotspot/share/opto/loopnode.cpp line 2028:
> 2026: PhaseIterGVN* igvn = &_phase->igvn();
> 2027:
> 2028: _structure = LoopStructure(_head, _loop, _phase, _iv_bt);
Can you also initialize this directly in the `CountedLoopConverter` constructor?
src/hotspot/share/opto/loopnode.cpp line 2039:
> 2037: // Check trip counter will end up higher than the limit
> 2038: const TypeInteger* limit_t = igvn->type(_structure.exit_test().limit())->is_integer(_iv_bt);
> 2039: if (is_infinite_loop(_structure.truncated_increment().trunc1(), limit_t, _structure.iv_incr().incr())) {
Here and in the following method calls you basically fetch almost all information from `LoopStructure`. Could you also move the methods to `LoopStructure` such that you can directly access the information? You then might not even need to expose the info with accessor methods.
I.e.:
`_structure.is_infinite_loop(...)`
src/hotspot/share/opto/loopnode.cpp line 2098:
> 2096: // iv_post_i < adjusted_limit
> 2097: //
> 2098: // If that is not the case, we need to canonicalize the loop exit check by using different values for adjusted_limit:
I suggest to refer to `final_limit_correction()` comments here. Maybe something like:
If that is not the case, we need to canonicalize the loop exit check by using different values for adjusted_limit (see LoopStructure::final_limit_correction()).
src/hotspot/share/opto/loopnode.cpp line 2116:
> 2114: // Note that:
> 2115: // (AL) limit <= adjusted_limit.
> 2116: //
I think this should be left here because we refer to `(AL)` further down.
Suggestsion:
// Note that after canonicalization:
// (AL) limit <= adjusted_limit.
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();
src/hotspot/share/opto/loopnode.cpp line 3063:
> 3061:
> 3062: //=============================================================================
> 3063: //----------------------match_incr_with_optional_truncation--------------------
Suggestion:
src/hotspot/share/opto/loopnode.cpp line 3067:
> 3065: // CHAR: (i+1)&0x7fff, BYTE: ((i+1)<<8)>>8, or SHORT: ((i+1)<<16)>>16
> 3066: bool CountedLoopNode::TruncatedIncrement::build() {
> 3067: _is_valid = false;
I was about to suggest to remove that since you already initialize it with `false` in the constructor. However, I like the clarity and therefore would suggest to add this as a first line to all the other `build()` methods. Then it's evidently clear that the fall back is always `_is_valid = false` when calling `build()`.
src/hotspot/share/opto/loopnode.cpp line 3070:
> 3068:
> 3069: // Quick cutouts:
> 3070: if (_expr == nullptr || _expr->req() != 3) return false;
I suggest to add braces.
src/hotspot/share/opto/loopnode.cpp line 3073:
> 3071:
> 3072: Node *t1 = nullptr;
> 3073: Node *t2 = nullptr;
Suggestion:
Node* t1 = nullptr;
Node* t2 = nullptr;
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`?
src/hotspot/share/opto/loopnode.hpp line 1347:
> 1345: bool canonicalize_mask(jlong stride_con);
> 1346:
> 1347: bool is_valid_with_bt(BasicType bt) {
Can be made `const`.
src/hotspot/share/opto/loopnode.hpp line 2073:
> 2071:
> 2072: bool _insert_stride_overflow_limit_check = false;
> 2073: bool _insert_init_trip_limit_check = false;
Can you move all fields to the top? This makes it easier to find them.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24458#pullrequestreview-3321712957
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418778220
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418907645
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418774895
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418772979
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418767290
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418766210
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418992345
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418889702
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418981869
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418913201
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418974397
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418754458
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2419178179
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418922405
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418929161
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2419155761
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2419003003
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2419002784
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2419006501
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2419006886
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2419010452
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418783014
PR Review Comment: https://git.openjdk.org/jdk/pull/24458#discussion_r2418751926
More information about the hotspot-compiler-dev
mailing list