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

[Quan Anh Mai]

On Thu, 13 Feb 2025 16:57:30 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 two commits:
> 
>  - Merge branch 'master' into JDK-8349139
>  - fix & test

Hmmm, may be you are right. I think adding a comment at `PhiNode` saying that people must not rely on it being pinned at the `Region` for dependencies would be a wise move, I can't think of any reason for that besides value narrowing right now but being pinned is a property of `Phi` regardless and we should tell people not to rely on this behaviour.

For this bug, I think a more general fix is to try to compare the type of the `Phi` with that of the input it is going to be replaced with. If the former is not wider than the latter then we add a `CastNode`, since the cast is only about value range, not strict dependency, we can use `CarryDependency` instead of `UnconditionalDependency`. Am I right?

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

PR Comment: https://git.openjdk.org/jdk/pull/23617#issuecomment-2659999245