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

[Christian Hagedorn]

On Fri, 18 Apr 2025 14:08:11 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 incrementally with one additional commit since the last revision:
> 
>   whitespace

Looks good to me, too.

Nit: You should probably update the title: "divisor not null" -> "divisor not zero".

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

> 6070:     // which is be transformed to (3):
> 6071:     // (AddI pre_loop_iv -1)
> 6072:     // The compiler may be able to constant fold the assert predicate condition for (3) but not (1)

Suggestion:

    // skip over the cast added by PhaseIdealLoop::cast_incr_before_loop() when pre/post/main loops are created because
    // it can get in the way of type propagation. For instance, the index tested by an Assertion Predicate, if the cast is
    // not skipped over, could be (1):
    // (AddI (CastII (AddI pre_loop_iv -2) int) 1)
    // while without the cast, it is (2):
    // (AddI (AddI pre_loop_iv -2) 1)
    // which is be transformed to (3):
    // (AddI pre_loop_iv -1)
    // The compiler may be able to constant fold the Assertion Predicate condition for (3) but not (1)

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

Marked as reviewed by chagedorn (Reviewer).

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