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

[Roland Westrelin]

> 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 test/hotspot/jtreg/compiler/rangechecks/TestEmptyLoopDeadCast.java
  
  Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/20140/files
  - new: https://git.openjdk.org/jdk/pull/20140/files/da8d6b13..c4dc7bb6

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=20140&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20140&range=03-04

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/20140.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20140/head:pull/20140

PR: https://git.openjdk.org/jdk/pull/20140