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

[Emanuel Peter]

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.

Looks reasonable.

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

I guess the issue is that ConvL2I and ConvI2L are also type nodes, which can restrict their type, just like CastII nodes. And that restricting of the type is only true under a certain if-branch.

But if the ConvI2L were not a type-node, then it would not restrict type, and you could simply push it through phis. Right?

Why do we have type restriction mixed into ConvI2L? Could that not be separated out into a CastII / CastLL?

Maybe we could generally separate ConvI2L, type restriction, and pinning? CastII also does multiple things, and it has hurt us many times in the past. Would this sort of maximal separation and specialization not be more "see of nodes" style?

Anyway, this would be interesting to look into for a future RFE.

test/hotspot/jtreg/compiler/splitif/TestLongCountedLoopConvL2I.java line 31:

> 29:  *                   -XX:+StressGCM -XX:StressSeed=92643864 TestLongCountedLoopConvL2I
> 30:  * @run main/othervm -XX:-BackgroundCompilation -XX:-TieredCompilation -XX:-UseOnStackReplacement
> 31:  *                   -XX:+StressGCM TestLongCountedLoopConvL2I

Would it make sense to have a run that allows OSR?

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19086#pullrequestreview-2043711442
PR Review Comment: https://git.openjdk.org/jdk/pull/19086#discussion_r1592792340