RFR: 8354282: C2: more crashes in compiled code because of dependency on removed range check CastIIs [v4]
Christian Hagedorn
chagedorn at openjdk.org
Wed Nov 26 14:31:58 UTC 2025
On Tue, 25 Nov 2025 12:52:35 GMT, Roland Westrelin <roland at openjdk.org> wrote:
>> This is a variant of 8332827. In 8332827, an array access becomes
>> dependent on a range check `CastII` for another array access. When,
>> after loop opts are over, that RC `CastII` was removed, the array
>> access could float and an out of bound access happened. With the fix
>> for 8332827, RC `CastII`s are no longer removed.
>>
>> With this one what happens is that some transformations applied after
>> loop opts are over widen the type of the RC `CastII`. As a result, the
>> type of the RC `CastII` is no longer narrower than that of its input,
>> the `CastII` is removed and the dependency is lost.
>>
>> There are 2 transformations that cause this to happen:
>>
>> - after loop opts are over, the type of the `CastII` nodes are widen
>> so nodes that have the same inputs but a slightly different type can
>> common.
>>
>> - When pushing a `CastII` through an `Add`, if of the type both inputs
>> of the `Add`s are non constant, then we end up widening the type
>> (the resulting `Add` has a type that's wider than that of the
>> initial `CastII`).
>>
>> There are already 3 types of `Cast` nodes depending on the
>> optimizations that are allowed. Either the `Cast` is floating
>> (`depends_only_test()` returns `true`) or pinned. Either the `Cast`
>> can be removed if it no longer narrows the type of its input or
>> not. We already have variants of the `CastII`:
>>
>> - if the Cast can float and be removed when it doesn't narrow the type
>> of its input.
>>
>> - if the Cast is pinned and be removed when it doesn't narrow the type
>> of its input.
>>
>> - if the Cast is pinned and can't be removed when it doesn't narrow
>> the type of its input.
>>
>> What we need here, I think, is the 4th combination:
>>
>> - if the Cast can float and can't be removed when it doesn't narrow
>> the type of its input.
>>
>> Anyway, things are becoming confusing with all these different
>> variants named in ways that don't always help figure out what
>> constraints one of them operate under. So I refactored this and that's
>> the biggest part of this change. The fix consists in marking `Cast`
>> nodes when their type is widen in a way that prevents them from being
>> optimized out.
>>
>> Tobias ran performance testing with a slightly different version of
>> this change and there was no regression.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains nine commits:
>
> - review
> - review
> - Merge branch 'master' into JDK-8354282
> - review
> - infinite loop in gvn fix
> - renaming
> - merge
> - Merge branch 'master' into JDK-8354282
> - fix & test
Introducing a 4th dependency type looks reasonable. It's also nice to see one more refactoring in that area which makes it very expressive now. Thanks for doing that! I left some suggestions to possibly further improve the code.
src/hotspot/share/opto/castnode.cpp line 40:
> 38: const ConstraintCastNode::DependencyType ConstraintCastNode::DependencyType::FloatingNonNarrowing(true, false, "floating non narrowing dependency"); // not pinned, doesn't narrow type
> 39: const ConstraintCastNode::DependencyType ConstraintCastNode::DependencyType::NonFloatingNarrowing(false, true, "now floating narrowing dependency"); // pinned, narrows type
> 40: const ConstraintCastNode::DependencyType ConstraintCastNode::DependencyType::NonFloatingNonNarrowing(false, false, "non floating non narrowing dependency"); // pinned, doesn't narrow type
Adding `-`:
Suggestion:
const ConstraintCastNode::DependencyType ConstraintCastNode::DependencyType::FloatingNonNarrowing(true, false, "floating non-narrowing dependency"); // not pinned, doesn't narrow type
const ConstraintCastNode::DependencyType ConstraintCastNode::DependencyType::NonFloatingNarrowing(false, true, "non-floating narrowing dependency"); // pinned, narrows type
const ConstraintCastNode::DependencyType ConstraintCastNode::DependencyType::NonFloatingNonNarrowing(false, false, "non-floating non-narrowing dependency"); // pinned, doesn't narrow type
src/hotspot/share/opto/castnode.cpp line 50:
> 48: if (!_dependency.narrows_type()) {
> 49: return this;
> 50: }
I suggest to split the comment to make it more clear:
Suggestion:
if (!_dependency.narrows_type()) {
// If this cast doesn't carry a type dependency (i.e. not used for type narrowing), we cannot optimize it.
return this;
}
// This cast node carries a type depedency. We can remove it if:
// - Its input has a narrower type
// - There's a dominating cast with same input but narrower type
src/hotspot/share/opto/castnode.cpp line 634:
> 632: if (wide_t != bottom_t) {
> 633: // Widening the type of the Cast (to allow some commoning) causes the Cast to change how it can be optimized (if
> 634: // type of its input is narrower than the Cast's type, we can't remove it to not loose the dependency).
Suggestion:
// type of its input is narrower than the Cast's type, we can't remove it to not loose the control dependency).
src/hotspot/share/opto/castnode.hpp line 101:
> 99: }
> 100: return NonFloatingNonNarrowing;
> 101: }
Just a side note: We seem to mix the terms "(non-)pinned" with "(non-)floating" freely. Should we stick to just one? But maybe it's justified to use both depending on the situation/code context.
src/hotspot/share/opto/castnode.hpp line 120:
> 118: // be removed in any case otherwise the sunk node floats back into the loop.
> 119: static const DependencyType NonFloatingNonNarrowing;
> 120:
I needed a moment to completely understand all these combinations. I rewrote the definitions in this process a little bit. Feel free to take some of it over:
// All the possible combinations of floating/narrowing with example use cases:
// Use case example: Range Check CastII
// Floating: The Cast is only dependent on the single range check.
// Narrowing: The Cast narrows the type to a positive index. If the input to the Cast is narrower, we can safely
// remove the cast because the array access will be safe.
static const DependencyType FloatingNarrowing;
// Use case example: Widening Cast nodes' types after loop opts: We want to common Casts with slightly different types.
// Floating: These Casts only depend on the single control.
// NonNarrowing: Even when the input type is narrower, we are not removing the Cast. Otherwise, the dependency
// to the single control is lost, and an array access could float above its range check because we
// just removed the dependency to the range check by removing the Cast. This could lead to an
// out-of-bounds access.
static const DependencyType FloatingNonNarrowing;
// Use case example: An array accesses that is no longer dependent on a single range check (e.g. range check smearing).
// NonFloating: The array access must be pinned below all the checks it depends on. If the check it directly depends
// on with a control input is hoisted, we do hoist the Cast as well. If we allowed the Cast to float,
// we risk that the array access ends up above another check it depends on (we cannot model two control
// dependencies for a node in the IR). This could lead to an out-of-bounds access.
// Narrowing: If the Cast does not narrow the input type, then it's safe to remove the cast because the array access
// will be safe.
static const DependencyType NonFloatingNarrowing;
// Use case example: Sinking nodes out of a loop
// Non-Floating & Non-Narrowing: We don't want the Cast that forces the node to be out of loop to be removed in any
// case. Otherwise, the sunk node could float back into the loop, undoing the sinking.
// This Cast is only used for pinning without caring about narrowing types.
static const DependencyType NonFloatingNonNarrowing;
test/hotspot/jtreg/compiler/c2/irTests/TestPushAddThruCast.java line 100:
> 98: @Run(test = "test3")
> 99: public static void test3_runner() {
> 100: i = RANDOM.nextInt(3, length-1);
Suggestion:
i = RANDOM.nextInt(3, length - 1);
-------------
PR Review: https://git.openjdk.org/jdk/pull/24575#pullrequestreview-3510584501
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2565071692
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2565111822
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2565208320
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2565130012
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2565000528
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2565211189
More information about the graal-dev
mailing list