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

Roland Westrelin roland at openjdk.org
Fri Mar 28 09:34:51 UTC 2025


> 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 five commits:

 - other test + review comment
 - Merge branch 'master' into JDK-8349139
 - Merge branch 'master' into JDK-8349139
 - Merge branch 'master' into JDK-8349139
 - fix & test

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

Changes: https://git.openjdk.org/jdk/pull/23617/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23617&range=02
  Stats: 199 lines in 7 files changed: 168 ins; 25 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/23617.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/23617/head:pull/23617

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


More information about the hotspot-compiler-dev mailing list