RFR: 8324517: C2: crash in compiled code because of dependency on removed range check CastIIs [v2]

[Roland Westrelin]

> Range check `CastII` nodes are removed once loop opts are over. The
> test case for this change includes 3 cases where elimination of a
> range check `CastII` causes a crash in compiled code because either a
> out of bounds array load or a division by zero happen.
> 
> In `test1`:
> 
> - the range checks for the `array[otherArray.length]` loads constant
>   fold: `otherArray.length` is a `CastII` of i at the `otherArray`
>   allocation. `i` is less than 9. The `CastII` at the allocation
>   narrows the type down further to `[0-9]`.
>   
> - the `array[otherArray.length]` loads are control dependent on the
> unrelated:
> 
> 
> if (flag == 0) {
> 
> 
> test. There's an identical dominating test which replaces that one. As
> a consequence, the `array[otherArray.length]` loads become control
> dependent on the dominating test.
> 
> - The `CastII` nodes at the `otherArray` allocations are replaced by a
>   dominating range check `CastII` nodes for:
>   
> 
> newArray[i] = 42;
> 
> 
> - After loop opts, the range check `CastII` nodes are removed and the
>   2 `array[otherArray.length]` loads common at the first:
>   
> 
> if (flag == 0) {
> 
> 
> test before the:
> 
> 
> float[] otherArray = new float[i];
> 
> 
> and
> 
> 
> newArray[i] = 42;
> 
> 
> that guarantee `i` is positive.
> 
> - `test1` is called with `i = -1`, the array load proceeds with an out
>   of bounds index and the crash occurs.
>   
>   
> `test2` and `test3` are mostly identical except for the check that's
> eliminated (a null divisor check) and the instruction that causes a
> fault (an integer division).
>   
> The fix I propose is to not eliminate range check `CastII` nodes after
> loop opts. When range check`CastII` nodes were introduced, performance
> was observed to regress. Removing them after loop opts was found to
> preserve both correctness and performance. Today, the performance
> regression still exists when `CastII` nodes are left in. So I propose
> we keep them until the end of optimizations (so the 2 array loads
> above don't lose a dependency and wrongly common) but remove them at
> the end of all optimizations.
> 
> In the case of the array loads, they are dependent on a range check
> for another array through a range check `CastII` and we must not lose
> that dependency otherwise the array loads could float above the range
> check at gcm time. I propose we deal with that problem the way it's
> handled for `CastPP` nodes: add the dependency to the load (or
> division)nodes as a precedence edge when the cast is removed.
> 
> @TobiHartmann ran performance testing for that patch (Thanks!) and reported
> no regression.

Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

 - review
 - Merge branch 'master' into JDK-8324517
 - test and fix

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/18377/files
  - new: https://git.openjdk.org/jdk/pull/18377/files/182cd4fc..0de61cbc

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18377&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18377&range=00-01

  Stats: 373272 lines in 3153 files changed: 27823 ins; 20294 del; 325155 mod
  Patch: https://git.openjdk.org/jdk/pull/18377.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18377/head:pull/18377

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