RFR: 8354282: C2: more crashes in compiled code because of dependency on removed range check CastIIs [v3]
Emanuel Peter
epeter at openjdk.org
Wed Nov 12 08:33:29 UTC 2025
On Thu, 2 Oct 2025 09:08:06 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 incrementally with three additional commits since the last revision:
>
> - review
> - infinite loop in gvn fix
> - renaming
@rwestrel Sorry I dropped the review on this one for a long time :/
I left quite a few comments. But on the whole I'm really happy with the direction you are taking. It's getting much clearer. I would still see some more clear explanations/comments. That way, we can make our previously implicit assumptions even more explicit :)
src/hotspot/share/opto/castnode.cpp line 47:
> 45: Node* ConstraintCastNode::Identity(PhaseGVN* phase) {
> 46: if (!_dependency.narrows_type()) {
> 47: return this;
Can you please add a code comment? I don't understand it right away :/
src/hotspot/share/opto/castnode.cpp line 153:
> 151: if (!_dependency.narrows_type()) {
> 152: return nullptr;
> 153: }
Interesting, we already check that at at least some of the use sites. If it turns out we already do it at all use sites, why not just assert? (maybe not possible or desirable, just an idea)
A comment here would also be great.
src/hotspot/share/opto/castnode.cpp line 277:
> 275:
> 276: CastIINode* CastIINode::pin_array_access_node() const {
> 277: assert(depends_only_on_test(), "already pinned");
Would this not be more readable?
Suggestion:
assert(is_dependency_floating(), "already pinned");
src/hotspot/share/opto/castnode.cpp line 588:
> 586:
> 587: // If both inputs are not constant then, with the Cast pushed through the Add/Sub, the cast gets less precised types,
> 588: // and the resulting Add/Sub's type is wider than that of the Cast before pushing.
I find this long sentence a bit complicated to read. Can you reformulate and maybe break it into smaller sentences?
It would also be good to explicitly say why that may require changing the dependency constraint.
src/hotspot/share/opto/castnode.cpp line 615:
> 613: // Widening the type of the Cast (to allow some commoning) causes the Cast to change how it can be optimized (if
> 614: // type of its input is narrower than the Cast's type, we can't remove it to not loose the dependency).
> 615: return make_with(in(1), wide_t, _dependency.widen_type_dependency());
Suggestion:
return make_with(in(1), wide_t, _dependency.with_non_narrowing());
This may be clearer here, since non-narrowing prevents folding the cast away if the input is narrower. I like the code comment you already have though :)
src/hotspot/share/opto/castnode.cpp line 625:
> 623: if (!phase->C->post_loop_opts_phase()) {
> 624: return this_type;
> 625: }
Honestly, I would prefer to see this "delay to post loop opts" to be done outside of `widen_type`. It would just make more sense there. What do you think?
src/hotspot/share/opto/castnode.hpp line 46:
> 44: // 1- and 2- are not always applied depending on what constraint are applied to the Cast: there are cases where 1-
> 45: // and 2- apply, where neither 1- nor 2- apply and where one or the other apply. This class abstract away these
> 46: // details.
Can you spell it out a little more? Right now it feels a little bit like an "exercise for the reader".
For each optimization, what is required of the constraints? I think that would help the reader.
Equally: you could name why those constraints are required in the first place. Or is there some other place we could link to that already has those explanations?
src/hotspot/share/opto/castnode.hpp line 53:
> 51: _narrows_type(narrows_type),
> 52: _desc(desc) {
> 53: }
Could you make the constructor private, and only expose the 4 static fields? That way, nobody comes to the strange idea to construct one of these themselves ;)
src/hotspot/share/opto/castnode.hpp line 62:
> 60: bool narrows_type() const {
> 61: return _narrows_type;
> 62: }
Nits about naming:
I would prefer `is_` for boolean queries. Otherwise, if I look at the names `floating` and `pinned_dependency`, I don't immediately know which one converts to a floating/non-floating, and which one is a boolean query.
Maybe `pinned_dependency` should be renamed to `with_pinned_dependency`.
src/hotspot/share/opto/castnode.hpp line 65:
> 63: void dump_on(outputStream *st) const {
> 64: st->print("%s", _desc);
> 65: }
Suggestion:
bool narrows_type() const {
return _narrows_type;
}
void dump_on(outputStream *st) const {
st->print("%s", _desc);
}
Newline for consistency with surrounding code.
src/hotspot/share/opto/castnode.hpp line 92:
> 90: const bool _floating; // Does this Cast depends on its control input or is it pinned?
> 91: const bool _narrows_type; // Does this Cast narrows the type i.e. if input type is narrower can it be removed?
> 92: const char* _desc;
I thought the hotspot convention was to usually put the fields first, at the top of the class?
src/hotspot/share/opto/castnode.hpp line 104:
> 102: // NonFloatingNarrowingDependency is used when an array access is no longer dependent on a single range check (range
> 103: // check smearing for instance)
> 104: // FloatingNonNarrowingDependency is used after loop opts when Cast nodes' types are widen so Casts that only differ
Suggestion:
// FloatingNonNarrowingDependency is used after loop opts when Cast nodes' types are widened so Casts that only differ
src/hotspot/share/opto/castnode.hpp line 110:
> 108: static const DependencyType FloatingNonNarrowingDependency;
> 109: static const DependencyType NonFloatingNarrowingDependency;
> 110: static const DependencyType NonFloatingNonNarrowingDependency;
Why not put the example at each definition? Would prevent repeating the names :)
It would be good if we could have this section earlier up, so the code comments of the `DependencyType` class and this form a unit. At least link them.
`NonFloatingNonNarrowingDependency` example: can you spell out the why? What could go wrong otherwise? Would the node float back into the loop maybe? What's wrong with that?
`NonFloatingNarrowingDependency` more detail would be helpful. I would like to know why non floating, and why narrowing? Because that's what these examples are for, right?
`FloatingNonNarrowingDependency` ah, maybe that answers one of my questions further up somewhere. If we don't have narrowing, then we should not fold away the cast because of the type, right?
I think if we spell out which optimizations require which constraints, that could help a lot here.
src/hotspot/share/opto/castnode.hpp line 122:
> 120: ShouldNotReachHere();
> 121: return nullptr;
> 122: }
This always smells like a messed up class hierarchy, when I see default methods with "not implemented". But maybe we can't do much better, and I've done similar things recently 🙈 . A short code comment could be helpful though.
Suggestion:
virtual ConstraintCastNode* make_with(Node* parent, const TypeInteger* type, const DependencyType& dependency) const {
ShouldNotReachHere(); // Only implemented for CastII and CastLL
return nullptr;
}
src/hotspot/share/opto/castnode.hpp line 146:
> 144: virtual uint ideal_reg() const = 0;
> 145: bool carry_dependency() const { return !_dependency.cmp(FloatingNarrowingDependency); }
> 146: virtual bool depends_only_on_test() const { return _dependency.floating(); }
Why not rename it to `is_dependency_floating`? That may be more helpful at the use site.
test/hotspot/jtreg/compiler/c2/irTests/TestPushAddThruCast.java line 95:
> 93: j += Objects.checkIndex(i - 1, length);
> 94: return j;
> 95: }
Why not add an additional IR rule that checks that there are more casts before they get commoned? Just for completenes ;)
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24575#pullrequestreview-3451986831
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517197209
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517271796
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517301300
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517315011
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517336133
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517344615
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517236142
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517203781
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517366170
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517205971
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517200829
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517251068
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517260839
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517355725
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517299467
PR Review Comment: https://git.openjdk.org/jdk/pull/24575#discussion_r2517370224
More information about the graal-dev
mailing list