[jdk17] RFR: 8269752: C2: assert(false) failed: Bad graph detected in build_loop_late

[Christian Hagedorn]

On Tue, 13 Jul 2021 11:34:24 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> Some nodes are sunk out of the outer loop of vMeth(). A CastII node is
> added on the exit path of the loop to pin the nodes out of the
> loop. The dominating test of that CastII is:
> 
> if (i >= 132) {
> 
> so the CastII is updated with type int [132..]
> 
> The inner loop is eliminated, the outer loop is turned into a counted
> loop, pre/main/post loops are created. The CastII control is now a
> Region that merges paths from the pre/main/post loops. Its data input
> is a Phi that merges the iv Phis of the 3 loops.
> 
> The loop body includes an early exit:
> 
> if (i2 != 0) {
>     vMeth_check_sum += i3;
>     return;
> }
> 
> That test is guaranteed to be taken at the second iteration. As a
> consequence, the backedge of the main loop is removed. What's left of
> the main loop is still guarded by a an entry test that compares the iv
> value out of the pre loop with an Opaque1 node. The CastII is now only
> reachable through that test. The iv Phi of the pre loop constant folds
> to a value that's not >= 132. As a consequence the CastII becomes
> top. top propagates to other data node and as a consequence a Phi is
> replaced by its only non top input. That causes the assert failure.
> 
> The CastII should not be reachable anymore once the iv Phi of the pre
> loop has constant folded and the early exit in the main loop is always
> taken. But because the test that guards the main loop uses an Opaque1,
> it cannot constant fold. As a fix, I propose removing the Opaque1 once
> the backedge of the main loop is removed. This allows the entry test
> to constant fold. The Opaque1 has no purpose anymore given the loop
> doesn't exist at this point.

Nice analysis, that looks reasonable to me.

src/hotspot/share/opto/loopTransform.cpp line 2936:

> 2934: 
> 2935:   // Find RCE'd post loop so that we can stage its guard.
> 2936:   if (legacy_cl->is_canonical_loop_entry() == NULL) return multi_version_succeeded;

Can you also add braces here?

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/247