RFR: 8361608: C2: assert(opaq->outcnt() == 1 && opaq->in(1) == limit) failed [v5]

[Christian Hagedorn]

On Tue, 21 Oct 2025 07:46:31 GMT, Marc Chevalier <mchevalier at openjdk.org> wrote:

>> Loop peeling works by cloning the loop body, which implies to replace the uses of the data in the loop to be replaced by a phi between the original loop and the clone. This is done by `PhaseIdealLoop::fix_data_uses` and can create a maze of phis. Multiple users of the same original data will get a fresh `PhiNode`, there is no logic trying to reuse them, or simplify. That's IGVN's job.
>> 
>> When we have something like
>> 
>> // any loop
>> while (...) { /* something involving limit */ }
>> // counted loop with zero trip guard
>> if (i < limit) {
>>     for (int i = init; i < limit; i++) { ... }
>> }
>> 
>> and we peel the first loop, the limits in the zero trip guard and in the counted loop condition are not the same node anymore but a fresh `PhiNode`.
>> 
>> But the method `PhaseIdealLoop::do_unroll` has the assert
>> 
>> https://github.com/openjdk/jdk/blob/444007fc234aeff75025831c2d1b5538c87fa8f1/src/hotspot/share/opto/loopTransform.cpp#L1929-L1930
>> 
>> requiring that both `limit` are the same node. But as explained, it might not be the case after peeling the first loop.
>> 
>> This situation doesn't happen if IGVN happens between peeling the first loop and unrolling the second. While there is no formal invariant that this must always be true, I couldn't reproduce the same situation without stress peeling: either peeling happens too early, or not at all, or something else happens so that major progress is set before unrolling, which always saves the day. I've tried to hack on an example to make the peeling decision happen "naturally" (using the normal heuristic), but in the right situation, not too early or too late. At this point it was so hardcoded that it's not significantly different than a run with stress peeling.
>> 
>> But with stress peeling, this situation seems to happen, rarely, but sometimes. What should we do?
>> 
>> By creating many `PhiNode`s `PhaseIdealLoop::fix_data_uses` is doing exactly what we expect. We could make it a lot smarter to try to reuse the `PhiNode`s previously constructed, but that would be hard because the inputs of the fresh phis are recursively adjusted, so we can't share ahead of time when inputs are the same. Duplicating when inputs start to differ would also lead to too many copies since phis look indeed different and some more top down clean up can actually collapse them all.
>> 
>> We could run IGVN to clean up the thing after each peeling: it was deemed not desirable as many things are expected to happen immediately after peeling.
>>...
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Check before

Thanks for the update! Two more suggestions but then I think it looks good from my side.

src/hotspot/share/opto/loopnode.cpp line 4771:

> 4769:         // See PhaseIdealLoop::do_unroll
> 4770:         // This property is desirable, but it maybe not hold after cloning a loop.
> 4771:         // In such a case, we bailout unrolling, and rely on IGVN to cleanup stuff.

Suggestion:

        // In such a case, we bail out from unrolling, and rely on IGVN to cleanup stuff.

src/hotspot/share/opto/loopnode.cpp line 4779:

> 4777:         if (!head->is_pre_loop() && !head->is_post_loop()) {
> 4778:           assert(opaque->outcnt() == 1 && opaque->in(1) == head->limit(), "IGVN should have cleaned that up!");
> 4779:         }

I think we can just check `is_main_loop()` here. `is_canonical_loop_entry()` will bail out if we see anything else than main or post loops:

https://github.com/openjdk/jdk/blob/bdfd5e843a7d3db50edf4375e50449b0ce528f8a/src/hotspot/share/opto/loopnode.cpp#L6330-L6333


Suggestion:

        if (head->is_main_loop()) {
          assert(opaque->outcnt() == 1 && opaque->in(1) == head->limit(), "IGVN should have cleaned that up!");
        }

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

PR Review: https://git.openjdk.org/jdk/pull/27586#pullrequestreview-3364466165
PR Review Comment: https://git.openjdk.org/jdk/pull/27586#discussion_r2451038240
PR Review Comment: https://git.openjdk.org/jdk/pull/27586#discussion_r2451097797