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

[Emanuel Peter]

On Thu, 13 Feb 2025 16:57:30 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> 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 two commits:
> 
>  - Merge branch 'master' into JDK-8349139
>  - fix & test

Looks reasonable. I have a few questions / suggestions.
I'm running some testing, please ping me again in a day or two ;)

src/hotspot/share/opto/loopTransform.cpp line 1703:

> 1701:   }
> 1702:   // CastII for the new post loop:
> 1703:   cast_incr_before_loop(zer_opaq->in(1), zer_taken, post_head);

I see it is added for the main and post loop. Why not for the pre loop?

src/hotspot/share/opto/loopnode.cpp line 6091:

> 6089:   if (uncast && init->is_CastII()) {
> 6090:     // skip over the cast added by PhaseIdealLoop::cast_incr_before_loop() when pre/post/main loops are created because
> 6091:     // it can get in the way of type propagation

I think it would be nice if you said more about how it can get in the way of type propagation. Why would we sometimes have `uncast` on and sometimes off? You may even have a quick comment about it at the use-site.

test/hotspot/jtreg/compiler/controldependency/TestDivDependentOnMainLoopGuard.java line 50:

> 48:                 i1 <<= i3;
> 49:             } while (++i2 < 68);
> 50:             for (i23 = 68; i23 > 2; otherPhi=i23-1, i23--) {

This is essencially the test from https://github.com/openjdk/jdk/pull/3190, we only changed the `3` to a `2`. This indicates that we need to probably slightly generalize the test. Can we maybe randomize the constant, just to get a little better coverage?

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

PR Review: https://git.openjdk.org/jdk/pull/23617#pullrequestreview-2639946128
PR Review Comment: https://git.openjdk.org/jdk/pull/23617#discussion_r1969207426
PR Review Comment: https://git.openjdk.org/jdk/pull/23617#discussion_r1969206376
PR Review Comment: https://git.openjdk.org/jdk/pull/23617#discussion_r1969183742