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

[Marc Chevalier]

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.

We could make the assert a lot smarter and test for equality of these nodes, up to upcoming simplifications... But that is quite complex for questionable benefit. That is IGVN's job, let's not do it twice! Phi simplification is also not the simplest.

After all, nothing wrong seems to happen: each function seems to do its job right, the graph seems well-structured, with the right semantics. So maybe the assert is just too strong by assuming that the graph would always be cleaned up before reaching it. And then, the best for the moment is simply to weaken the assert: if StressLoopPeeling is on, then the assert might not hold and it's ok. Indeed, by skipping the assert, all end up fine: `do_unroll` replaces the input of the `OpaqueZeroTripGuard` and of the loop halting condition separately:

https://github.com/openjdk/jdk/blob/444007fc234aeff75025831c2d1b5538c87fa8f1/src/hotspot/share/opto/loopTransform.cpp#L2025

https://github.com/openjdk/jdk/blob/444007fc234aeff75025831c2d1b5538c87fa8f1/src/hotspot/share/opto/loopTransform.cpp#L2046

In the future, if we indeed get this situation in the wild, without stress peeling, maybe we should make `do_unroll` more robust, or make it give up if it detects that the graph doesn't fulfill the expected state yet, and give IGVN a chance first.

Thanks,
Marc

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

Commit messages:
 - Weakening the assert

Changes: https://git.openjdk.org/jdk/pull/27586/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=27586&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8361608
  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/27586.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/27586/head:pull/27586

PR: https://git.openjdk.org/jdk/pull/27586