-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8354282: C2: more crashes in compiled code because of dependency on removed range check CastIIs #24575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back roland! A progress list of the required criteria for merging this PR into |
|
@rwestrel This change is no longer ready for integration - check the PR body for details. |
Webrevs
|
|
If a |
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwestrel thanks for looking into this one!
I have not yet deeply studied the PR, but am feeling some confusion about the naming.
I think the DependencyType is really a good step into the right direction, it helps clean things up.
I'm wondering if we should pick either depends_only_on_test or pinned, and use it everywhere consistently. Having both around as near synonymes (antonymes?) is a bit confusing for me.
I'll look into the code more later.
src/hotspot/share/opto/castnode.cpp
Outdated
| const ConstraintCastNode::DependencyType ConstraintCastNode::RegularDependency(true, true, "regular dependency"); // not pinned, narrows type | ||
| const ConstraintCastNode::DependencyType ConstraintCastNode::WidenTypeDependency(true, false, "widen type dependency"); // not pinned, doesn't narrow type | ||
| const ConstraintCastNode::DependencyType ConstraintCastNode::StrongDependency(false, true, "strong dependency"); // pinned, narrows type | ||
| const ConstraintCastNode::DependencyType ConstraintCastNode::UnconditionalDependency(false, false, "unconditional dependency"); // pinned, doesn't narrow type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there really a good reason to have the names Regular, WidenType, Strong and Unconditional? Did we just get used to these names over time, or do they really have a good reason for existance. They just don't really mean that much to me. Calling them (non)pinned and (non)narrowing would make more sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So NonPinnedNarrowingDependency, NonPinnedNonNarrowingDependeny, PinnedNarrowingDependency and NonPinnedNonNarrowingDependency?
Or to avoid using a negation for the one that's the weakest dependency:
FloatingNarrowingDependency, FloatingNonNarrowingDependency, NonFloatingNarrowingDependency and NonFloatingNonNarrowingDependency ?
What do you think @eme64 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either of these sound great :)
src/hotspot/share/opto/castnode.hpp
Outdated
| bool depends_only_on_test() const { | ||
| return _depends_only_on_test; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this synonimous to non_pinning? Might that be more descriptive?
src/hotspot/share/opto/castnode.hpp
Outdated
| const bool _depends_only_on_test; // Does this Cast depends on its control input or is it pinned? | ||
| const bool _narrows_type; // Does this Cast narrows the type i.e. if input type is narrower can it be removed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to have a really strong definition of these two, because everything else depends on it.
I would recommend to either use depends_only_on_test as the "primary" word here, or else pinned. But then try to consistently use the chosen one everywhere. Just to avoid confusion with these near synonymes.
It may also be helpful to have an example for each of the 4 combinations, just as an illustration of your definitions.
The current patch constant folds the |
|
@emea thanks for the comments. As mentioned in another comment, I'm in the process of reworking the patch.
|
|
Just wondering, since we are getting closer to RDP 1 for JDK 25 (June 05, 2025), should we defer this to JDK 26? |
Deferring makes sense. This is a corner case anyway. I've been reworking the patch and it's getting more complicated so it will likely need more time for reviews. |
|
Sounds good, I'll defer it to JDK 26 then. Thanks for the quick reply! |
|
@rwestrel this pull request can not be integrated into git checkout JDK-8354282
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
@rwestrel This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@rwestrel This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
/keepalive |
|
@rwestrel This command can only be used in open pull requests. |
|
/open |
|
@rwestrel This pull request is now open |
|
@rwestrel This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@rwestrel This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
@rwestrel This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 :)
| if (!_dependency.narrows_type()) { | ||
| return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a code comment? I don't understand it right away :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm slowly starting to understand... but a code comment would still help a lot here.
We are trying to find a dominating cast that has the same or narrower type, and replace with that one.
We are only allowed to do that if we have a narrowing cast, because ...
src/hotspot/share/opto/castnode.hpp
Outdated
| private: | ||
| const bool _floating; // Does this Cast depends on its control input or is it pinned? | ||
| const bool _narrows_type; // Does this Cast narrows the type i.e. if input type is narrower can it be removed? | ||
| const char* _desc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the hotspot convention was to usually put the fields first, at the top of the class?
| DependencyType(bool depends_on_test, bool narrows_type, const char* desc) | ||
| : _floating(depends_on_test), | ||
| _narrows_type(narrows_type), | ||
| _desc(desc) { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would probably require moving the 4 static fields into this class here.
Example:
ConstraintCastNode::DependencyType::FloatingNarrowing
Just an idea. Maybe you have a different solution. But a private constructor would be great for sure.
| bool narrows_type() const { | ||
| return _narrows_type; | ||
| } | ||
| void dump_on(outputStream *st) const { | ||
| st->print("%s", _desc); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bool narrows_type() const { | |
| return _narrows_type; | |
| } | |
| void dump_on(outputStream *st) const { | |
| st->print("%s", _desc); | |
| } | |
| bool narrows_type() const { | |
| return _narrows_type; | |
| } | |
| void dump_on(outputStream *st) const { | |
| st->print("%s", _desc); | |
| } |
Newline for consistency with surrounding code.
| // 1- and 2- are not always applied depending on what constraint are applied to the Cast: there are cases where 1- | ||
| // and 2- apply, where neither 1- nor 2- apply and where one or the other apply. This class abstract away these | ||
| // details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.cpp
Outdated
| if (wide_t != bottom_t) { | ||
| // Widening the type of the Cast (to allow some commoning) causes the Cast to change how it can be optimized (if | ||
| // type of its input is narrower than the Cast's type, we can't remove it to not loose the dependency). | ||
| return make_with(in(1), wide_t, _dependency.widen_type_dependency()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return make_with(in(1), wide_t, _dependency.widen_type_dependency()); | |
| 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
Outdated
| const TypeInteger* this_type = res->is_integer(bt); | ||
| if (!phase->C->post_loop_opts_phase()) { | ||
| return this_type; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe that is a refactoring for a separate RFE, and then not really worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But conceptually, we want to say: if we are in post loop opts, then widen the types.
Now it looks like we want to widen always ... but then we check for post loop opts inside the method and bail out anyway. Not very transparent. Another idea: rename the method to widen_type_in_post_loop_opts.
Totally up to you though.
| virtual ConstraintCastNode* make_with(Node* parent, const TypeInteger* type, const DependencyType& dependency) const { | ||
| ShouldNotReachHere(); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| virtual ConstraintCastNode* make_with(Node* parent, const TypeInteger* type, const DependencyType& dependency) const { | |
| ShouldNotReachHere(); | |
| return nullptr; | |
| } | |
| 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
Outdated
| bool floating() const { | ||
| return _floating; | ||
| } | ||
|
|
||
| bool narrows_type() const { | ||
| return _narrows_type; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| // Test commoning of Casts after loop opts when they are at the same control | ||
| @Test | ||
| @IR(counts = { IRNode.CAST_II, "2" }) | ||
| public static int test3() { | ||
| int j = Objects.checkIndex(i - 3, length); | ||
| j += Objects.checkIndex(i, length); | ||
| j += Objects.checkIndex(i - 2, length); | ||
| j += Objects.checkIndex(i - 1, length); | ||
| return j; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add an additional IR rule that checks that there are more casts before they get commoned? Just for completenes ;)
|
@eme64 updated change should address your comments |
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| // used when a floating node is sunk out of loop: 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 floats back into the loop. | ||
| static const DependencyType NonFloatingNonNarrowing; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking it over :-)
| const DependencyType& with_pinned_dependency() const { | ||
| if (_narrows_type) { | ||
| return NonFloatingNarrowing; | ||
| } | ||
| return NonFloatingNonNarrowing; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch as it is now adds some extra uses of "pinned" and "floating". What could make sense, I suppose, would be to try to use "floating"/"non floating" instead but there are so many uses of "pinned" in the code base already, and I don't see us getting rid of them, that I wonder if it would make a difference. So, I'm not too sure what to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's true. I was also unsure about whether we should stick with one or just allow both interchangeably. I guess since there are so many uses, we can just move forward with what you have now and still come back to clean it up if necessary - we can always do that.
test/hotspot/jtreg/compiler/c2/irTests/TestPushAddThruCast.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>
Thanks for the comments/suggestions. Updated change should take care of all of them. |
chhagedorn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, it looks good to me! If @eme64 also agrees with the latest patch, we can submit some testing and then hopefully get it in right before the fork.
| const DependencyType& with_pinned_dependency() const { | ||
| if (_narrows_type) { | ||
| return NonFloatingNarrowing; | ||
| } | ||
| return NonFloatingNonNarrowing; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's true. I was also unsure about whether we should stick with one or just allow both interchangeably. I guess since there are so many uses, we can just move forward with what you have now and still come back to clean it up if necessary - we can always do that.
| // used when a floating node is sunk out of loop: 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 floats back into the loop. | ||
| static const DependencyType NonFloatingNonNarrowing; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking it over :-)
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwestrel Nice work! We not just only fixed the bug but made the concepts much clearer. This makes me very happy 😊
|
|
||
| // All the possible combinations of floating/narrowing with example use cases: | ||
|
|
||
| // Use case example: Range Check CastII |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is incorrect, a range check should be floating non-narrowing. It is only narrowing if the length of the array is a constant. It is because this cast encodes the dependency on the condition index u< length. This condition cannot be expressed in terms of Type unless length is a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Range check CastII were added to protect the ConvI2L in the address expression on 64 bits. The problem there was, in some cases, that the ConvI2L would float above the range check (because ConvI2L has no control input) and could end up with an out of range input (which in turn would cause the ConvI2L to become top in places where it wasn't expected).
So CastII doesn't carry the control dependency of an array access on its range check. That dependency is carried by the MemNode which has its control input set to the range check.
What you're saying, if I understand it correctly, would be true if the CastII was required to prevent an array Load from floating. But that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, sorry I misunderstood!
Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>
|
@merykitty @eme64 @chhagedorn thanks for the reviews |
|
@rwestrel I'll run some testing now ... |
This is a variant of 8332827. In 8332827, an array access becomes
dependent on a range check
CastIIfor another array access. When,after loop opts are over, that RC
CastIIwas removed, the arrayaccess could float and an out of bound access happened. With the fix
for 8332827, RC
CastIIs 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, thetype of the RC
CastIIis no longer narrower than that of its input,the
CastIIis removed and the dependency is lost.There are 2 transformations that cause this to happen:
after loop opts are over, the type of the
CastIInodes are widenso nodes that have the same inputs but a slightly different type can
common.
When pushing a
CastIIthrough anAdd, if of the type both inputsof the
Adds are non constant, then we end up widening the type(the resulting
Addhas a type that's wider than that of theinitial
CastII).There are already 3 types of
Castnodes depending on theoptimizations that are allowed. Either the
Castis floating(
depends_only_test()returnstrue) or pinned. Either theCastcan 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:
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
Castnodes 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.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24575/head:pull/24575$ git checkout pull/24575Update a local copy of the PR:
$ git checkout pull/24575$ git pull https://git.openjdk.org/jdk.git pull/24575/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24575View PR using the GUI difftool:
$ git pr show -t 24575Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24575.diff
Using Webrev
Link to Webrev Comment