-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor #24013
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
Conversation
|
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
|
@chhagedorn This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 106 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@chhagedorn The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
src/hotspot/share/opto/cfgnode.hpp
Outdated
| bool is_useless() const; | ||
| void mark_useless(PhaseIterGVN& igvn); | ||
| void mark_maybe_useful(); | ||
| bool is_useful() const; | ||
| void mark_useful(); |
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.
Needed to move these definitions to the source file because I cannot include predicates.hpp here due to circular dependencies. I solved this by forward declaring PredicateState and moving the definitions to the source file.
Same for these methods for OpaqueTemplateAssertionPredicate further down.
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 add predicates.inline.hpp for 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.
Do you mean the enum definition for PredicateState? That could work, so I can just include that header when I need to use the enum. Should I then rather name the hpp file something like predicates_enums.hpp? IIUC, xyz.inline.hpp should be used primarily for inline methods.
Having the separate enum header also allows us to put this one there which makes things cleaner:
jdk/src/hotspot/share/opto/predicates.hpp
Lines 203 to 211 in e3c29c9
| // Assertion Predicates are either emitted to check the initial value of a range check in the first iteration or the last | |
| // value of a range check in the last iteration of a loop. | |
| enum class AssertionPredicateType { | |
| None, // Not an Assertion Predicate | |
| InitValue, | |
| LastValue, | |
| // Used for the Initialized Assertion Predicate emitted during Range Check Elimination for the final IV value. | |
| FinalIv | |
| }; |
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've pushed an update introducing predicates_enums.hpp and also moving some typedefs for predicates in there. Let me know if that's what you had in mind. I could now move almost all definitions back to the header file except for mark_useless() which uses PhaseIterGVN which is not a complete type in the header file - I guess that's okay to just move this one to the source file.
| assert(_predicate_state != PredicateState::MaybeUseful, "should only be MaybeUseful when eliminating useless " | ||
| "predicates during loop opts"); |
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.
Best effort assert to ensure we are not seeing MaybeUseful anywhere else except during Predicate elimination. Same for OpaqueTemplateAssertionPredicate.
| fatal("unknown kind"); | ||
| } | ||
| if (_useless) { | ||
| if (_predicate_state == PredicateState::Useless) { |
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 only print useless since MaybeUseful is only set for a very brief moment and should normally not be visible when dumping/in IGV dumps.
| if (n->is_OpaqueTemplateAssertionPredicate()) { | ||
| C->add_template_assertion_predicate_opaque(n->as_OpaqueTemplateAssertionPredicate()); | ||
| } |
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.
See PR description "Other Included Changes".
| assert(!is_template_assertion_predicate || AssertionPredicate::has_halt(maybe_success_proj->as_IfTrue()), | ||
| "Template Assertion Predicate must have a Halt Node on the failing path"); |
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.
New verification - see "Other Included Changes" in PR description.
| // Or we have a Region that merges serval paths to a single Halt node. Even though OpaqueInitializedAssertionPredicate | ||
| // nodes do not common up (i.e. NO_HASH), we could have Initialized Assertion Predicates from already folded loops | ||
| // being now part of the innermost loop. When then further splitting this loop, we could be cloning the If node | ||
| // of the Initialized Assertion Predicate (part of the loop body) while the OpaqueInitializedAssertionPredicate is not | ||
| // cloned because it's outside the loop body. We end up sharing the OpaqueInitializedAssertionPredicate between the | ||
| // original and the cloned If. This should be fine. |
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.
Reason for not reusing AssertionPredicate::has_halt(). I will revisit the AssertionPredicate class in a future PR, so I have not tried to unify these methods in a way.
| return phase->type(in(1)); | ||
| } | ||
|
|
||
| OpaqueTemplateAssertionPredicateNode::OpaqueTemplateAssertionPredicateNode(BoolNode* bol): Node(nullptr, bol), |
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.
Moved some methods to source file - see comment above for ParsePredicateNode.
| #include "opto/subnode.hpp" | ||
|
|
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.
Noticed that opcodes.hpp was unused and subnode.hpp missed the opto prefix. Fixed here as well.
| // the ParsePredicateNode is not marked useless. | ||
| bool is_valid() const { | ||
| return _parse_predicate_node != nullptr; | ||
| return _parse_predicate_node != nullptr && !_parse_predicate_node->is_useless(); |
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.
Avoids visiting useless Parse Predicates during Predicate iteration.
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.
When can _parse_predicate_node be null?
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.
When iterating through predicates, we use a PredicateBlockIterator for each Predicate Block, which consists of an optional Parse Predicate and a number of Regular Predicates (Runtime and Assertion Predicates). We could have either already removed the Parse Predicate before here:
jdk/src/hotspot/share/opto/loopnode.cpp
Lines 5055 to 5066 in e3c29c9
| // Keep loop predicates and perform optimizations with them | |
| // until no more loop optimizations could be done. | |
| // After that switch predicates off and do more loop optimizations. | |
| if (!C->major_progress() && (C->parse_predicate_count() > 0)) { | |
| C->mark_parse_predicate_nodes_useless(_igvn); | |
| assert(C->parse_predicate_count() == 0, "should be zero now"); | |
| if (TraceLoopOpts) { | |
| tty->print_cr("PredicatesOff"); | |
| } | |
| C->set_major_progress(); | |
| } | |
| } |
or there is just no Parse Predicate for this loop. So, when initializing the PredicateBlockIterator, we could pass here a non-Parse-Predicate projection to the constructor of ParsePredicate:
jdk/src/hotspot/share/opto/predicates.hpp
Line 747 in e3c29c9
| _parse_predicate(start_node, deopt_reason), |
We then set _parse_predicate_node to null here due to this mismatch:
jdk/src/hotspot/share/opto/predicates.hpp
Lines 293 to 296 in e3c29c9
| static IfTrueNode* init_success_proj(const Node* parse_predicate_proj) { | |
| assert(parse_predicate_proj != nullptr, "must not be null"); | |
| return parse_predicate_proj->isa_IfTrue(); | |
| } |
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 the last code snippet the relevant one?
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.
Sorry, I copy-pasted the wrong snippet. The null is coming from here:
jdk/src/hotspot/share/opto/predicates.cpp
Lines 71 to 82 in e3c29c9
| // Returns the Parse Predicate node if the provided node is a Parse Predicate success proj. Otherwise, return null. | |
| ParsePredicateNode* ParsePredicate::init_parse_predicate(const Node* parse_predicate_proj, | |
| Deoptimization::DeoptReason deopt_reason) { | |
| assert(parse_predicate_proj != nullptr, "must not be null"); | |
| if (parse_predicate_proj->is_IfTrue() && parse_predicate_proj->in(0)->is_ParsePredicate()) { | |
| ParsePredicateNode* parse_predicate_node = parse_predicate_proj->in(0)->as_ParsePredicate(); | |
| if (parse_predicate_node->deopt_reason() == deopt_reason) { | |
| return parse_predicate_node; | |
| } | |
| } | |
| return nullptr; | |
| } |
Webrevs
|
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 looks good to me.
|
Thanks Vladimir for your review! |
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.
Looks good 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.
Generally looks good, I only gave it a quick scan. I'm wondering if there is a naming inconsistency, see below ;)
| void EliminateUselessPredicates::mark_all_predicates_non_useful() const { | ||
| mark_predicates_on_list_maybe_useful(_parse_predicates); | ||
| mark_predicates_on_list_maybe_useful(_template_assertion_predicate_opaques); | ||
| } |
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.
| void EliminateUselessPredicates::mark_all_predicates_non_useful() const { | |
| mark_predicates_on_list_maybe_useful(_parse_predicates); | |
| mark_predicates_on_list_maybe_useful(_template_assertion_predicate_opaques); | |
| } | |
| void EliminateUselessPredicates::mark_all_predicates_maybe_useful() const { | |
| mark_predicates_on_list_maybe_useful(_parse_predicates); | |
| mark_predicates_on_list_maybe_useful(_template_assertion_predicate_opaques); | |
| } |
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.
For consistency?
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.
Definitely an inconsistency, good catch! Will update this and the other occurrences.
| void EliminateUselessPredicates::eliminate() const { | ||
| mark_all_predicates_non_useful(); | ||
| if (C->has_loops()) { | ||
| mark_loop_associated_predicates_useful(); | ||
| } | ||
| mark_non_useful_predicates_useless(); | ||
| } |
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 would replace non_useful to maybe_useful to keep it consistent with the comments above.
|
Thanks Roland and Emanuel for your reviews! I've pushed an updated with your suggestions Emanuel. |
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 updates. I'm rubber stamping this, only did a quick scan ;)
|
Thanks Emanuel! I will run some testing again with the recent updates before integration. |
|
/integrate |
|
Going to push as commit e57b272.
Your commit was automatically rebased without conflicts. |
|
@chhagedorn Pushed as commit e57b272. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This patch cleans the Parse and Template Assertion Predicate elimination code up. We now use a single
PredicateVisitorand share code in a newEliminateUselessPredicatesclass which contains the code previously found inPhaseIdealLoop::eliminate_useless_predicates().Unified Logic to Clean Up Parse and Template Assertion Predicates
We now use the following algorithm:
jdk/src/hotspot/share/opto/predicates.cpp
Lines 1174 to 1179 in 5e4b6ca
This is different from the old algorithm where we used a single boolean state
_useless. But that does no longer work because when we first mark Template Assertion Predicates useless, we are no longer visiting them when iterating through predicates:jdk/src/hotspot/share/opto/predicates.hpp
Lines 704 to 708 in a21fa46
We therefore require a third state. Thus, I introduced a new tri-state
PredicateStatethat provides a specialMaybeUsefulvalue which we can set each Predicate to.Ignoring Useless Parse Predicates
While working on this patch, I've noticed that we are always visiting Parse Predicates - even when they useless. We should change that to align it with what we have for the other Predicates (changed in JDK-8351280). To make this work, we also replace the
_uselessstate inParsePredicateNodewith a newPredicateState.Sharing Code for Parse and Template Assertion Predicates
With all the mentioned changes in place, I could nicely share code for the elimination of Parse and Template Assertion Predicates in
EliminateUselessPredicatesby using templates. The following additional changes were required:_template_assertion_predicate_opaquesto the more specificOpaqueTemplateAssertionPredicateNodetype.Compile.ParsePredicate::mark_useless()to pass inPhaseIterGVN, as done for Assertion PredicatesNote that we still do not directly replace the useless Predicates but rather mark them useless as initiated by JDK-8351280.
Other Included Changes
_template_assertion_predicate_opaqueslist. It was done directly in the old cloning methods. This is not relevant for correctness but could hinder some optimizations. I've added the code now inNode::clone()to make sure we do not miss any Template Assertion Predicates (similar to what we do for Parse Predicates already):jdk/src/hotspot/share/opto/node.cpp
Lines 514 to 516 in 5e4b6ca
TemplateAssertionPredicate::is_predicate()andInitializedAssertionPredicate::is_predicate()to verify that when we find anOpaque*AssertionPredicatenode that we also find the associatedHaltnode.Thanks,
Christian
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24013/head:pull/24013$ git checkout pull/24013Update a local copy of the PR:
$ git checkout pull/24013$ git pull https://git.openjdk.org/jdk.git pull/24013/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24013View PR using the GUI difftool:
$ git pr show -t 24013Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24013.diff
Using Webrev
Link to Webrev Comment