[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR: 8324517: C2: crash in compiled code because of dependency on removed range check CastIIs [v
From:       Tobias Hartmann <thartmann () openjdk ! org>
Date:       2024-05-16 14:47:12
Message-ID: YKksR9IW2JBElQhZ3VJLAM2MsV2fP2rWfAD4OsuaxK8=.cc34f5e0-3041-4461-a3c0-871a25513e85 () github ! com
[Download RAW message or body]

On Wed, 15 May 2024 12:07:22 GMT, Roland Westrelin <roland@openjdk.org> wrote:

> > 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 ...
> 
> 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 ten additional commits since the last \
> revision: 
> - Node::is_div_or_mod()
> - Merge branch 'master' into JDK-8324517
> - test fix
> - review
> - Merge branch 'master' into JDK-8324517
> - Merge branch 'master' into JDK-8324517
> - review
> - Merge branch 'master' into JDK-8324517
> - test and fix

Right, I did run testing on an early draft and v01 and only saw the `Error: VM option \
'StressIGVN' is diagnostic and must be enabled via -XX:+UnlockDiagnosticVMOptions` \
issue I reported above. We missed re-running testing of later versions.

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

PR Comment: https://git.openjdk.org/jdk/pull/18377#issuecomment-2115447227


[prev in list] [next in list] [prev in thread] [next in thread]