RFR: 8332827: [REDO] C2: crash in compiled code because of dependency on removed range check CastIIs [v3]

[Christian Hagedorn]

On Thu, 12 Dec 2024 08:57:07 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The failures that caused the backout were due to a bug in:
>> 
>> `find_or_make_integer_cast()`
>> 
>> which caused the `_range_check_dependency` field's value of the
>> existing cast node to not be set in the new cast node. I re-ran some
>> testing with this fixed and current jdk repo and found that a few
>> vectorization tests fail now because the patch pushes range check
>> `CastII` nodes through `AddI`/`SubI`. To fix this, I delayed that
>> transformation to after loop opts.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review

Looks good to me, too. Friendly ping to @eme64 about the performance testing results.

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

> 3145:     DivModNode* divmod = DivModNode::make(n, bt, is_unsigned);
> 3146:     // If the divisor input for a Div (or Mod etc.) is not null, then the control input of the Div is set to null.
> 3147:     // It could be that the divisor input is found not null because its type is narrowed down by a CastII in the

You mean not zero?
Suggestion:

    // If the divisor input for a Div (or Mod etc.) is not zero, then the control input of the Div is set to null.
    // It could be that the divisor input is found not zero because its type is narrowed down by a CastII in the

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

> 3439:     n->as_CastII()->remove_range_check_cast(this);
> 3440:   }
> 3441:   break;

I guess it still works but you should move the `break` inside the braces:
Suggestion:

    n->as_CastII()->remove_range_check_cast(this);
    break;
  }

src/hotspot/share/opto/compile.hpp line 56:

> 54: class CallGenerator;
> 55: class CallStaticJavaNode;
> 56: class CastIINode;

Is this required? There is no additional change/use of `CastIINode` in this file.

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

> 33:  *                   -XX:CompileCommand=dontinline,TestArrayAccessAboveRCAfterRCCastIIEliminated::notInlined
> 34:  *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM TestArrayAccessAboveRCAfterRCCastIIEliminated
> 35:  * @run main/othervm TestArrayAccessAboveRCAfterRCCastIIEliminated

Would `main` be sufficient if you run without flags?
Suggestion:

 * @run main TestArrayAccessAboveRCAfterRCCastIIEliminated

test/hotspot/jtreg/compiler/rangechecks/TestRangeCheckCastIISplitThruPhi.java line 30:

> 28:  *
> 29:  * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation TestRangeCheckCastIISplitThruPhi
> 30:  * @run main/othervm TestRangeCheckCastIISplitThruPhi

Same here:
Suggestion:

 * @run main TestRangeCheckCastIISplitThruPhi

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22568#pullrequestreview-2505483146
PR Review Comment: https://git.openjdk.org/jdk/pull/22568#discussion_r1886383838
PR Review Comment: https://git.openjdk.org/jdk/pull/22568#discussion_r1886397864
PR Review Comment: https://git.openjdk.org/jdk/pull/22568#discussion_r1886446579
PR Review Comment: https://git.openjdk.org/jdk/pull/22568#discussion_r1886456422
PR Review Comment: https://git.openjdk.org/jdk/pull/22568#discussion_r1886456893