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

Roland Westrelin roland at openjdk.org
Tue Mar 19 13:27:31 UTC 2024


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.

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

Commit messages:
 - test and fix

Changes: https://git.openjdk.org/jdk/pull/18377/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18377&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8324517
  Stats: 212 lines in 4 files changed: 186 ins; 23 del; 3 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


More information about the hotspot-compiler-dev mailing list