[jdk20] RFR: 8298176: remove OpaqueZeroTripGuardPostLoop once main-loop disappears

[Christian Hagedorn]

On Tue, 20 Dec 2022 18:47:26 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> As described in https://github.com/openjdk/jdk20/pull/22, the bug is
> caused by the iv phi of a post loop that becomes top but because the
> post loop is guarded by an opaque node, the control flow remains
> alive.
> 
> The fix I propose is based on this comment Vladimir made:
> https://github.com/openjdk/jdk20/pull/22#issuecomment-1349570615
> 
> When CmpINode::Value() encounters a (CmpI (OpaqueZeroTripGuard in1)
> in2), it runs Value() on (CmpI in1 in2) and if it constant folds so
> that the loop is not taken, returns that result. Translating "loop not
> taken" into an actual CmpI type depends on whether the loop goes up or
> down. To make the check above possible, OpaqueZeroTripGuard includes
> the BoolTest::mask that causes the loop to be executed at the zero
> trip guard.
> 
> The new logic in CmpINode::Value() is executed for both the main and
> post loop zero trip guards (while the bug was only seen AFAIK with the
> post loop) because I expect the same bug to exist with the main loop.
> 
> For the main loop, this works because initially the loop should be
> executed and as optimizations proceed and adjust the zero trip guard,
> the range of iterations executed in the loop should narrow (and never
> widen). We may then end up with no iterations executed in the loop. No
> further optimizations would make the main loop executable again. It's
> then fine to fold the zero trip guard as we're done with
> optimizations.
> 
> This works for the post loop because the compiler has no way to tell
> whether it's executed or not as long as there's a main loop: the zero
> trip guard then takes as input a phi that merges the pre and main loop
> ivs. For the case of a loop going up, the zero trip guard should
> initially test whether [init, limit] (the type of phi) is stricly less
> than limit. The compiler can't decide what the result of that test
> is. As optimizations proceed, the [init, limit] range could become
> narrower as I understand and there's no risk for the compiler to
> report the post loop as not taken.
> 
> I still believe it's risky to simply drop the OpaqueZeroTripGuard for
> the post loop even if it can't constant fold at least because we
> wouldn't want the zero trip guard to split thru phi.

That looks reasonable to me.

You should change the bug title as we are now also removing the OpaqueZeroTripGuard node for the main loop and we did not introduce a main and post loop specific opaque node.

> > I still believe it's risky to simply drop the OpaqueZeroTripGuard for
> > the post loop even if it can't constant fold at least because we
> > wouldn't want the zero trip guard to split thru phi.
> 
> Should we investigate this for JDK 21?

I think that would be a good idea.

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk20/pull/65