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

[Quan Anh Mai]

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.

My understanding here is that when the backedge is removed, the loop head (which is a merge) is idealized out, and as a result, the `Phi` is idealized to its only live input. I think the idealization of the `Phi` is the issue here, the `Phi` is pinned at the control input it is at, so the result of the idealization cannot float freely. As a result, I propose fixing the idealization of `Phi` to become a `CastNode` that has `UnknownControlDependency` on its input. What do you think?

I think this may be true for other kind of merge points as well. For example, if you perform conditional elimination (the patch you have worked at) on this code:

    if (v > 0) {
        int y;
        if (b) {
            y = v;
        } else {
            y = v + 1;
        }
        x / y;
    }

Then we can be very clever and see that `y` cannot be 0 because it is a `Phi`. However, if we also later see that `b == true`, then the `Phi` is removed and the division `x / v` does not have a control input, which is wrong.

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

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