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

[Emanuel Peter]

On Tue, 19 Mar 2024 13:21:49 GMT, Roland Westrelin <roland at 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 as a precedence edge when the cast is removed.
> 
> @TobiHartmann ran performance testing for that patch (Thanks!) and reported
> no regression.

Thanks @rwestrel !
Generally makes sense, I have a few suggestions and questions.

src/hotspot/share/opto/castnode.cpp line 222:

> 220:   if (!_range_check_dependency) {
> 221:     res = widen_type(phase, res, T_INT);
> 222:   }

Can you explain why you changed this, and why it is ok?

src/hotspot/share/opto/castnode.cpp line 240:

> 238:     phase->C->record_for_post_loop_opts_igvn(this);
> 239:   }
> 240:   if (!_type->is_int()->empty()) {

Can you also explain this change, please?

src/hotspot/share/opto/compile.cpp line 3898:

> 3896:     // as a precedence edge, so they can't float above the cast in case that cast's narrowed type helped eliminated a
> 3897:     // range check or a null divisor check.
> 3898:     assert(cast->in(0) != nullptr, "");

Suggestion:

    assert(cast->in(0) != nullptr, "All RangeCheck CastII must have a control dependency");

test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterRCCastIIEliminated.java line 39:

> 37:  *                   -XX:+StressIGVN -XX:StressSeed=94546681 TestArrayAccessAboveRCAfterRCCastIIEliminated
> 38:  *
> 39:  */

Can you please add a "vanilla" run like this:
`@run main TestArrayAccessAboveRCAfterRCCastIIEliminated`
That would allow us to run the test with any flag combination from the outside.

test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterRCCastIIEliminated.java line 41:

> 39:  */
> 40: 
> 41: public class TestArrayAccessAboveRCAfterRCCastIIEliminated {

It seems this test is not in a package. Is this on purpose?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18377#pullrequestreview-1965886193
PR Review Comment: https://git.openjdk.org/jdk/pull/18377#discussion_r1542784125
PR Review Comment: https://git.openjdk.org/jdk/pull/18377#discussion_r1542784375
PR Review Comment: https://git.openjdk.org/jdk/pull/18377#discussion_r1542774195
PR Review Comment: https://git.openjdk.org/jdk/pull/18377#discussion_r1542786077
PR Review Comment: https://git.openjdk.org/jdk/pull/18377#discussion_r1542786478