RFR: 8349139: C2: Div looses dependency on condition that guarantees divisor not null in counted loop [v5]

[Emanuel Peter]

On Mon, 14 Apr 2025 15:37:36 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The test crashes because of a division by zero. The `Div` node for
>> that one is initially part of a counted loop. The control input of the
>> node is cleared because the divisor is non zero. This is because the
>> divisor depends on the loop phi and the type of the loop phi is
>> narrowed down when the counted loop is created. pre/main/post loops
>> are created, unrolling happens, the main loop looses its backedge. The
>> `Div` node can then float above the zero trip guard for the main
>> loop. When the zero trip guard is not taken, there's no guarantee the
>> divisor is non zero so the `Div` node should be pinned below it.
>> 
>> I propose we revert the change I made with 8334724 which removed
>> `PhaseIdealLoop::cast_incr_before_loop()`. The `CastII` that this
>> method inserted was there to handle exactly this problem. It was added
>> initially for a similar issue but with array loads. That problem with
>> loads is handled some other way now and that's why I thought it was
>> safe to proceed with the removal.
>> 
>> The code in this patch is somewhat different from the one we had
>> before for a couple reasons:
>> 
>> 1- assert predicate code evolved and so previous logic can't be
>>   resurrected as it was.
>>   
>> 2- the previous logic has a bug.
>> 
>> Regarding 1-: during pre/main/post loop creation, we used to add the
>> `CastII` and then to add assertion predicates (so assertion predicates
>> depended on the `CastII`). Then when unrolling, when assertion
>> predicates are updated, we would skip over the `CastII`. What I
>> propose here is to add the `CastII` after assertion predicates are
>> added. As a result, they don't depend on the `CastII` and there's no
>> need for any extra logic when unrolling happens. This, however,
>> doesn't work when the assertion predicates are added by RCE. In that
>> case, I had to add logic to skip over the `CastII` (similar to what
>> existed before I removed it).
>> 
>> Regarding 2-: previous implementation for
>> `PhaseIdealLoop::cast_incr_before_loop()` would add the `CastII` at
>> the first loop `Phi` it encounters that's a use of the loop increment:
>> it's usually the iv but not always. I tweaked the test case to show,
>> this bug can actually cause a crash and changed the logic for
>> `PhaseIdealLoop::cast_incr_before_loop()` accordingly.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains nine commits:
> 
>  - review
>  - Merge branch 'master' into JDK-8349139
>  - merge
>  - Merge branch 'master' into JDK-8349139
>  - other test + review comment
>  - Merge branch 'master' into JDK-8349139
>  - Merge branch 'master' into JDK-8349139
>  - Merge branch 'master' into JDK-8349139
>  - fix & test

Looks good, except for the whitespace issue :)

test/hotspot/jtreg/compiler/controldependency/TestDivDependentOnMainLoopGuard.java line 34:

> 32:  *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM TestDivDependentOnMainLoopGuard
> 33:  * @run main/othervm -Xcomp -XX:CompileOnly=TestDivDependentOnMainLoopGuard::* TestDivDependentOnMainLoopGuard
> 34:  *                    

Suggestion:


You got some whitespace issues here, we could just remove the line :)

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23617#pullrequestreview-2775123522
PR Review Comment: https://git.openjdk.org/jdk/pull/23617#discussion_r2048572661