RFR: 8280799: С2: assert(false) failed: cyclic dependency prevents range check elimination [v2]

[Vladimir Kozlov]

On Wed, 9 Feb 2022 12:55:39 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The loop exit condition of the test method:
>> 
>> if (i == stop) {
>>     break;
>> }
>> 
>> requires insertion of a loop limit predicate when the loop is turned
>> into a counted loop. stop is a LoadI. Next:
>> 
>> array[stop - i + j] = 0;
>> 
>> is transformed to:
>> 
>> array[stop - i] = 0;
>> 
>> and the range check for that array access becomes candidate for
>> predication in a subsequent loop opts pass. stop has control just
>> above the loop limit check when that happens (because it is assigned
>> control as late as possible). But the loop predicate for the bound
>> check needs to be above the loop limit check and that causes the
>> assert failure.
>> 
>> There's already logic in PhaseIdealLoop::build_loop_late_post_work()
>> to assign control to nodes above predicates so this sort of issues
>> doesn't occur. But it only applies to node initially on the entry
>> projection right above the loop head. The fix I propose is to remove
>> that restriction.
>> 
>> That logic was added by JDK-8203197 and looking back at this change I
>> noticed I replaced some existing logic with the current logic but,
>> while the 2 overlap, the current logic is not guaranteed to always
>> cover some cases handled by the old logic. So I resurrected that old
>> logic here.
>> 
>> Finally, when running tests, I hit failures because Opaque nodes for
>> skeleton predicates can now end up above a predicate that is split
>> thru phi. That causes the Opaque nodes to be split up and breaks
>> pattern matching. I'm actually not sure this issue is specific to the
>> change here so I think it's best to treat it as a general issue and
>> fix it by cloning the chain of nodes that lead to the Opaque node
>> down.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - fix
>  - Merge branch 'master' into JDK-8280799
>  - fix & test

Scratch my previous comment. `least` value could be updated in new code so we need second check.

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

> 5756:   // Try not to place code on a loop entry projection
> 5757:   // which can inhibit range check elimination.
> 5758:   if (least != early) {

It is the same check as new above - you can combine these scopes.

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

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7319