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

[Tobias Hartmann]

On Fri, 12 Jan 2024 10:52:55 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> 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-...

Looks good to me.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17394#pullrequestreview-1821534844