Integrated: 8349139: C2: Div looses dependency on condition that guarantees divisor not zero in counted loop

[Roland Westrelin]

On Thu, 13 Feb 2025 16:30:20 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.

This pull request has now been integrated.

Changeset: be6e4406
Author:    Roland Westrelin <roland at openjdk.org>
URL:       https://git.openjdk.org/jdk/commit/be6e4406d8c9024bb368ed9dc22d4a6df2a0846a
Stats:     206 lines in 7 files changed: 174 ins; 25 del; 7 mod

8349139: C2: Div looses dependency on condition that guarantees divisor not zero in counted loop

Reviewed-by: chagedorn, epeter, qamai

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

PR: https://git.openjdk.org/jdk/pull/23617