RFR: 8323101: C2: assert(n->in(0) == nullptr) failed: divisions with zero check should already have bailed out earlier in split-if

Christian Hagedorn chagedorn at openjdk.org
Fri Jan 12 11:02:38 UTC 2024


The assertion added by [JDK-8299259](https://bugs.openjdk.org/browse/JDK-8299259) is wrong. I've originally assumed that at this point, there are no pinned `Div/Mod` nodes anymore that we possibly want to split through a phi due to bailing out earlier here:
https://github.com/openjdk/jdk/blob/3e19bf88d5b51fe10c183f930b99bce961a368c1/src/hotspot/share/opto/loopopts.cpp#L1137-L1140

The assumption was that a `Div/Mod` node with a control input to the zero-check `IfProj` could only have a `Phi` input with a `Region` that is further up in the graph. This is normally true. However, in `testIntDiv()`, we split an `If` with `do_split_if()` and need to empty the basic block. We split the store `iFld = sub` up. This includes the `StoreI` as well as the `AddI` which also has the `Region` as current `ctrl` (i.e. returned with `get_ctrl()`).

The `AddI` also has the `DivI` as output which, however, is not pushed up since it's not part of the same basic block. We end up with the following graph after the completion of `do_split_if()`:

![image](https://github.com/openjdk/jdk/assets/17833009/b76293e1-a593-4319-b026-254be3c098fc)

The `DivI` now has `252 Phi` as input which merges the split `AddI` nodes. `246 Region` of the `252 Phi` is further down than the control input `83 IfTrue` of the `DivI`.This is rather unusual and thus was missed when the assert was added in JDK-8299259. When finally processing the `DivI` node in the DFS walk of Split-If, we fail with the assertion.

The fix is straight forward to turn this assert into a simple bailout: We should not split a `Div/Mod` node that is pinned (i.e. has a zero check).

While working on this bug, I've also tried to trigger the assert with `DivL/ModL` nodes. However, this did not work because `split_up()` does not split the `Add` node up. The reason is that we set late ctrl to early ctrl for the `DivL/ModL` node (and thus also set the same late ctrl for the `Add` node) while we do not do that for `DivI/ModI` nodes. It seems that we miss to treat `DivL/ModL` nodes as unpinned here which would allow us to set a later ctrl:
https://github.com/openjdk/jdk/blob/82a63a03c0155288e8e43b9f766c8be70be50b6a/src/hotspot/share/opto/loopnode.cpp#L6091-L6101

I've done some digging and found that `DivL/ModL` nodes were added after this switch statment. So, I assume we simply forgot to also treat them as unpinned here. It's not wrong but I think just an unnecessary limitation. I filed [JDK-8323652](https://bugs.openjdk.org/browse/JDK-8323652) to fix this. 

Either way, I left the code for the long cases in even though they do not trigger. They should once JDK-8323652 is fixed.

Thanks,
Christian

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

Commit messages:
 - 8323101: C2: assert(n->in(0) == nullptr) failed: divisions with zero check should already have bailed out earlier in split-if

Changes: https://git.openjdk.org/jdk/pull/17394/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17394&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8323101
  Stats: 215 lines in 2 files changed: 214 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17394.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17394/head:pull/17394

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


More information about the hotspot-compiler-dev mailing list