RFR: 8331575: C2: crash when ConvL2I is split thru phi at LongCountedLoop

[Christian Hagedorn]

On Fri, 3 May 2024 12:33:43 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> In the test case:
> 
> 
> long i;
> for (; i > 0; i--) {
>     res += 42 / ((int) i);
> 
> 
> The long counted loop phi has type `[1..100]`. As a consequence, the
> `ConvL2I` also has type `[1..100]`. The `DivI` node that follows can't
> fault: it is not guarded by a zero check and has no control set.
> 
> The `ConvL2I` is split through phi and so is the `DiVI` node:
> `PhaseIdealLoop::cannot_split_division()` returns true because the
> value coming from the backedge into the `DivI` (when it is about to be
> split thru phi) is the result of the `ConvL2I` which has type
> `[1..100`] so is not zero as far as the compiler can tell.
> 
> On the last iteration of the loop, i is 1. Because the DivI was split
> thru Phi, it computes the value for the following iteration, so for i
> = 0. This causes a crash when the compiled code runs.
> 
> The same problem can't happen with an int counted loop because logic
> in `PhaseIdealLoop::split_thru_phi()` prevents a `ConvI2L` from being
> split thru phi. I propose to fix this the same way: in the test case,
> it's not true that once the `ConvL2I` is split thru phi it keeps type
> `[1..100]`. The fix is fairly conservative because it's base on the
> existing logic for `ConvI2L`: we would want to not split a `ConvL2I`
> only a counted loopd but. I suppose the same is true for the `ConvI2L`
> and I thought it would be best to revisit both together.

You could also add the regression tests from the duplicated issue [JDK-8298851](https://bugs.openjdk.org/browse/JDK-8298851).

Marked as reviewed by chagedorn (Reviewer).

src/hotspot/share/opto/loopopts.cpp line 54:

> 52:   if ((n->Opcode() == Op_ConvI2L && n->bottom_type() != TypeLong::LONG) ||
> 53:       (n->Opcode() == Op_ConvL2I && n->bottom_type() != TypeInt::INT)) {
> 54:     // ConvI2L/ConvL2I may have type information on it which is unsafe to push up

The fix looks good and we should probably move forward with that. 

But I'm still wondering though, if these bailouts are really needed in the general case. It seems like this problem is mainly for loop phis. Couldn't we check the types of loop phi inputs and bail out if one includes zero? IIUC, the backedge should be an `AddL` with type `[0..99]`, i.e. post-decremented. So, pushing through seems wrong in this case since the backedge type includes zero. But it could be detected and prevented. However, if the phi has type `[5..100]`, for example, then it should be safe. We probably then just need to update the type of the pushed-through `ConvL2I` to whatever the type of the input is.

This type checking approach could work in the general case. But I'm not sure though, if it's beneficial to split these `Conv` nodes through phis in general. But it seems the bailouts have only been introduced due to correctness bugs and not due to performance reasons. Anyway, this should be investigated separately, including benchmarking.

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

PR Review: https://git.openjdk.org/jdk/pull/19086#pullrequestreview-2040163524
PR Review: https://git.openjdk.org/jdk/pull/19086#pullrequestreview-2040170877
PR Review Comment: https://git.openjdk.org/jdk/pull/19086#discussion_r1590639677