RFR: 8335393: C2: assert(!had_error) failed: bad dominance [v2]

[Christian Hagedorn]

On Wed, 24 Jul 2024 07:45:11 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In `test1/test1Helper`:
>> 
>> 
>> v = array[i - 1];
>> 
>> 
>> is only used out of loop so it's sunk out of loop. Next pre/main/post
>> loops are created and range check elimination takes care of removing
>> the range check from the main loop. The main loop is then empty so
>> it's removed. The array access which is now out of loop becomes:
>> 
>> 
>> v = array[-2]
>> 
>> 
>> -1 is the final iteration of the main loop. -2 is obviously out of
>> bound and causes the range check `CastII` for the array access to
>> become top and the graph to be broken.
>> 
>> The main loop is never entered after range check elimination but the
>> zero trip guard for the main loop doesn't constant fold. That happens
>> because the init value of the loop is unkown which causes the loop
>> `Phi` of the pre loop (and main/post loops as well) to be of type
>> `int`.
>> 
>> For this bug to occur, I think we need an unknown value for the init
>> value of the loop. That way the zero trip guard doesn't constant
>> fold. We also need a known final loop value which happens when the
>> loop limit is known and the final loop only depends on the loop
>> limit. That can only happen when the stride of the loop is 1 (or -1).
>> 
>> If the range check is eliminated by predication, then a predicate for
>> the first and last loop variable values are added. This would catch
>> the case where the array access is out of bound for the last loop
>> variable value. So the bug can only occur with range check
>> elimination.
>> 
>> I see 2 ways to fix this. One is to narrow the type of the loop
>> Phi. After all we known that init < limit so in the test case that
>> start < 0. The other way to fix this is to add an assert predicate for
>> the final value of the loop variable every time an range check is
>> eliminated by range check elimination. That assert predicate only
>> needs to be added by range check elimination when the stride is 1 or
>> -1. It doesn't need to be updated when unrolling happens (because then
>> the stride can't be 1 or -1). That makes for a reasonably simple fix.
>> 
>> The reason I would rather go with the assert predicate fix is that
>> changing type computation of the iv phi node will affect a lot more
>> code and it's hard to be confident that it won't reveal new bugs.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update src/hotspot/share/opto/loopTransform.cpp
>   
>   Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>

Otherwise, looks good to me, too!

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

> 2884:   assert(loop_entry->is_Proj() && loop_entry->in(0)->is_If(), "if projection only");
> 2885: 
> 2886:   // if abs(stride) == 1, an assert predicate for the final iv value is added. We don't know the final iv value until

Suggestion:

  // if abs(stride) == 1, an Assertion Predicate for the final iv value is added. We don't know the final iv value until

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

> 2999:             // If the main loop becomes empty and the array access for this range check is sunk out of the loop, the index
> 3000:             // for the array access will be set to the index value of the final iteration which could be out of loop.
> 3001:             // Add an assert predicate for that corner case. The final iv is computed from LoopLimit which is the

Suggestion:

            // Add an Assertion Predicate for that corner case. The final iv is computed from LoopLimit which is the

test/hotspot/jtreg/compiler/rangechecks/TestEmptyLoopDeadCast.java line 29:

> 27:  * @summary C2: assert(!had_error) failed: bad dominance
> 28:  * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation -XX:-UseLoopPredicate
> 29:  *                   -XX:LoopMaxUnroll=0 TestEmptyLoopDeadCast

You should either add `-XX:+IgnoreUnrecognizedVMOptions` or `@requires vm.compiler2.enabled` since you use C2-specific flags.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20140#pullrequestreview-2196547845
PR Review Comment: https://git.openjdk.org/jdk/pull/20140#discussion_r1689653863
PR Review Comment: https://git.openjdk.org/jdk/pull/20140#discussion_r1689655083
PR Review Comment: https://git.openjdk.org/jdk/pull/20140#discussion_r1689660291