Skip to content
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

8257498: Remove useless skeleton predicates #2075

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/hotspot/share/opto/compile.cpp
Expand Up @@ -390,6 +390,9 @@ void Compile::remove_useless_node(Node* dead) {
if (dead->is_expensive()) {
remove_expensive_node(dead);
}
if (dead->Opcode() == Op_Opaque4) {
remove_skeleton_predicate_opaq(dead);
}
if (dead->for_post_loop_opts_igvn()) {
remove_from_post_loop_opts_igvn(dead);
}
Expand Down Expand Up @@ -565,6 +568,7 @@ Compile::Compile( ciEnv* ci_env, ciMethod* target, int osr_bci,
_intrinsics (comp_arena(), 0, 0, NULL),
_macro_nodes (comp_arena(), 8, 0, NULL),
_predicate_opaqs (comp_arena(), 8, 0, NULL),
_skeleton_predicate_opaqs (comp_arena(), 8, 0, NULL),
_expensive_nodes (comp_arena(), 8, 0, NULL),
_for_post_loop_igvn(comp_arena(), 8, 0, NULL),
_congraph(NULL),
Expand Down
17 changes: 14 additions & 3 deletions src/hotspot/share/opto/compile.hpp
Expand Up @@ -315,6 +315,7 @@ class Compile : public Phase {
GrowableArray<CallGenerator*> _intrinsics; // List of intrinsics.
GrowableArray<Node*> _macro_nodes; // List of nodes which need to be expanded before matching.
GrowableArray<Node*> _predicate_opaqs; // List of Opaque1 nodes for the loop predicates.
GrowableArray<Node*> _skeleton_predicate_opaqs; // List of Opaque4 nodes for the loop skeleton predicates.
GrowableArray<Node*> _expensive_nodes; // List of nodes that are expensive to compute and that we'd better not let the GVN freely common
GrowableArray<Node*> _for_post_loop_igvn; // List of nodes for IGVN after loop opts are over
ConnectionGraph* _congraph;
Expand Down Expand Up @@ -657,11 +658,13 @@ class Compile : public Phase {
void end_method(int level = 1);

int macro_count() const { return _macro_nodes.length(); }
int predicate_count() const { return _predicate_opaqs.length();}
int predicate_count() const { return _predicate_opaqs.length(); }
int skeleton_predicate_count() const { return _skeleton_predicate_opaqs.length(); }
int expensive_count() const { return _expensive_nodes.length(); }

Node* macro_node(int idx) const { return _macro_nodes.at(idx); }
Node* predicate_opaque1_node(int idx) const { return _predicate_opaqs.at(idx);}
Node* predicate_opaque1_node(int idx) const { return _predicate_opaqs.at(idx); }
Node* skeleton_predicate_opaque4_node(int idx) const { return _skeleton_predicate_opaqs.at(idx); }
Node* expensive_node(int idx) const { return _expensive_nodes.at(idx); }

ConnectionGraph* congraph() { return _congraph;}
Expand Down Expand Up @@ -689,7 +692,15 @@ class Compile : public Phase {
assert(_macro_nodes.contains(n), "should have already been in macro list");
_predicate_opaqs.append(n);
}

void add_skeleton_predicate_opaq(Node* n) {
assert(!_skeleton_predicate_opaqs.contains(n), "duplicate entry in skeleton predicate opaque4 list");
_skeleton_predicate_opaqs.append(n);
}
void remove_skeleton_predicate_opaq(Node* n) {
if (skeleton_predicate_count() > 0) {
_skeleton_predicate_opaqs.remove_if_existing(n);
}
}
bool post_loop_opts_phase() { return _post_loop_opts_phase; }
void set_post_loop_opts_phase() { _post_loop_opts_phase = true; }
void reset_post_loop_opts_phase() { _post_loop_opts_phase = false; }
Expand Down
57 changes: 37 additions & 20 deletions src/hotspot/share/opto/loopPredicate.cpp
Expand Up @@ -231,33 +231,20 @@ ProjNode* PhaseIdealLoop::clone_predicate_to_unswitched_loop(ProjNode* predicate
// the old predicates to the new cloned predicates.
void PhaseIdealLoop::clone_skeleton_predicates_to_unswitched_loop(IdealLoopTree* loop, const Node_List& old_new, Deoptimization::DeoptReason reason,
ProjNode* old_predicate_proj, ProjNode* iffast_pred, ProjNode* ifslow_pred) {
IfNode* iff = old_predicate_proj->in(0)->as_If();
assert(iffast_pred->in(0)->is_If() && ifslow_pred->in(0)->is_If(), "sanity check");
ProjNode* uncommon_proj = iff->proj_out(1 - old_predicate_proj->as_Proj()->_con);
Node* rgn = uncommon_proj->unique_ctrl_out();
assert(rgn->is_Region() || rgn->is_Call(), "must be a region or call uct");
assert(iff->in(1)->in(1)->Opcode() == Op_Opaque1, "unexpected predicate shape");
Node* predicate = iff->in(0);
// Only need to clone range check predicates as those can be changed and duplicated by inserting pre/main/post loops
// and doing loop unrolling. Push the original predicates on a list to later process them in reverse order to keep the
// original predicate order.
Unique_Node_List list;
while (predicate != NULL && predicate->is_Proj() && predicate->in(0)->is_If()) {
iff = predicate->in(0)->as_If();
uncommon_proj = iff->proj_out(1 - predicate->as_Proj()->_con);
if (uncommon_proj->unique_ctrl_out() != rgn)
break;
if (iff->in(1)->Opcode() == Op_Opaque4 && skeleton_predicate_has_opaque(iff)) {
// Only need to clone range check predicates as those can be changed and duplicated by inserting pre/main/post loops
// and doing loop unrolling. Push the original predicates on a list to later process them in reverse order to keep the
// original predicate order.
list.push(predicate);
}
predicate = predicate->in(0)->in(0);
}
get_skeleton_predicates(old_predicate_proj, list);

Node_List to_process;
IfNode* iff = old_predicate_proj->in(0)->as_If();
ProjNode* uncommon_proj = iff->proj_out(1 - old_predicate_proj->as_Proj()->_con);
// Process in reverse order such that 'create_new_if_for_predicate' can be used in 'clone_skeleton_predicate_for_unswitched_loops'
// and the original order is maintained.
for (int i = list.size() - 1; i >= 0; i--) {
predicate = list.at(i);
Node* predicate = list.at(i);
assert(predicate->in(0)->is_If(), "must be If node");
iff = predicate->in(0)->as_If();
assert(predicate->is_Proj() && predicate->as_Proj()->is_IfProj(), "predicate must be a projection of an if node");
Expand Down Expand Up @@ -288,6 +275,34 @@ void PhaseIdealLoop::clone_skeleton_predicates_to_unswitched_loop(IdealLoopTree*
}
}

// Put all skeleton predicate projections on a list, starting at 'predicate' and going up in the tree. If 'get_opaque'
// is set, then the Opaque4 nodes of the skeleton predicates are put on the list instead of the projections.
void PhaseIdealLoop::get_skeleton_predicates(Node* predicate, Unique_Node_List& list, bool get_opaque) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the get_opaque parameter, populate the list with projections (the get_opaque false case) and have the caller retrieve the opaque node (predicate->in(0)->in(1)) if that's what it needs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I updated it and removed the get_opaque flag.

IfNode* iff = predicate->in(0)->as_If();
ProjNode* uncommon_proj = iff->proj_out(1 - predicate->as_Proj()->_con);
Node* rgn = uncommon_proj->unique_ctrl_out();
assert(rgn->is_Region() || rgn->is_Call(), "must be a region or call uct");
assert(iff->in(1)->in(1)->Opcode() == Op_Opaque1, "unexpected predicate shape");
predicate = iff->in(0);
while (predicate != NULL && predicate->is_Proj() && predicate->in(0)->is_If()) {
iff = predicate->in(0)->as_If();
uncommon_proj = iff->proj_out(1 - predicate->as_Proj()->_con);
if (uncommon_proj->unique_ctrl_out() != rgn) {
break;
}
if (iff->in(1)->Opcode() == Op_Opaque4 && skeleton_predicate_has_opaque(iff)) {
if (get_opaque) {
// Collect the predicate Opaque4 node.
list.push(iff->in(1));
} else {
// Collect the predicate projection.
list.push(predicate);
}
}
predicate = predicate->in(0)->in(0);
}
}

// Clone a skeleton predicate for an unswitched loop. OpaqueLoopInit and OpaqueLoopStride nodes are cloned and uncommon
// traps are kept for the predicate (a Halt node is used later when creating pre/main/post loops and copying this cloned
// predicate again).
Expand Down Expand Up @@ -1241,6 +1256,7 @@ ProjNode* PhaseIdealLoop::insert_initial_skeleton_predicate(IfNode* iff, IdealLo
register_new_node(opaque_init, upper_bound_proj);
BoolNode* bol = rc_predicate(loop, upper_bound_proj, scale, offset, opaque_init, limit, stride, rng, (stride > 0) != (scale > 0), overflow);
Node* opaque_bol = new Opaque4Node(C, bol, _igvn.intcon(1)); // This will go away once loop opts are over
C->add_skeleton_predicate_opaq(opaque_bol);
register_new_node(opaque_bol, upper_bound_proj);
ProjNode* new_proj = create_new_if_for_predicate(predicate_proj, NULL, reason, overflow ? Op_If : iff->Opcode());
_igvn.replace_input_of(new_proj->in(0), 1, opaque_bol);
Expand All @@ -1258,6 +1274,7 @@ ProjNode* PhaseIdealLoop::insert_initial_skeleton_predicate(IfNode* iff, IdealLo
register_new_node(max_value, new_proj);
bol = rc_predicate(loop, new_proj, scale, offset, max_value, limit, stride, rng, (stride > 0) != (scale > 0), overflow);
opaque_bol = new Opaque4Node(C, bol, _igvn.intcon(1));
C->add_skeleton_predicate_opaq(opaque_bol);
register_new_node(opaque_bol, new_proj);
new_proj = create_new_if_for_predicate(predicate_proj, NULL, reason, overflow ? Op_If : iff->Opcode());
_igvn.replace_input_of(new_proj->in(0), 1, opaque_bol);
Expand Down
37 changes: 26 additions & 11 deletions src/hotspot/share/opto/loopnode.cpp
Expand Up @@ -3530,8 +3530,7 @@ void PhaseIdealLoop::log_loop_tree() {
//---------------------collect_potentially_useful_predicates-----------------------
// Helper function to collect potentially useful predicates to prevent them from
// being eliminated by PhaseIdealLoop::eliminate_useless_predicates
void PhaseIdealLoop::collect_potentially_useful_predicates(
IdealLoopTree * loop, Unique_Node_List &useful_predicates) {
void PhaseIdealLoop::collect_potentially_useful_predicates(IdealLoopTree* loop, Unique_Node_List &useful_predicates) {
if (loop->_child) { // child
collect_potentially_useful_predicates(loop->_child, useful_predicates);
}
Expand All @@ -3542,22 +3541,28 @@ void PhaseIdealLoop::collect_potentially_useful_predicates(
!loop->tail()->is_top()) {
LoopNode* lpn = loop->_head->as_Loop();
Node* entry = lpn->in(LoopNode::EntryControl);
Node* predicate_proj = find_predicate(entry); // loop_limit_check first
if (predicate_proj != NULL) { // right pattern that can be used by loop predication

Node* predicate = find_predicate_insertion_point(entry, Deoptimization::Reason_loop_limit_check);
if (predicate != NULL) { // right pattern that can be used by loop predication
assert(entry->in(0)->in(1)->in(1)->Opcode() == Op_Opaque1, "must be");
useful_predicates.push(entry->in(0)->in(1)->in(1)); // good one
entry = skip_loop_predicates(entry);
}
if (UseProfiledLoopPredicate) {
predicate_proj = find_predicate(entry); // Predicate
if (predicate_proj != NULL) {
predicate = find_predicate_insertion_point(entry, Deoptimization::Reason_profile_predicate);
if (predicate != NULL) { // right pattern that can be used by loop predication
useful_predicates.push(entry->in(0)->in(1)->in(1)); // good one
get_skeleton_predicates(entry, useful_predicates, true);
entry = skip_loop_predicates(entry);
}
}
predicate_proj = find_predicate(entry); // Predicate
if (predicate_proj != NULL) {
useful_predicates.push(entry->in(0)->in(1)->in(1)); // good one

if (UseLoopPredicate) {
predicate = find_predicate_insertion_point(entry, Deoptimization::Reason_predicate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to do this for Deoptimization::Reason_profile_predicate as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we need that as well. I updated it.

if (predicate != NULL) { // right pattern that can be used by loop predication
useful_predicates.push(entry->in(0)->in(1)->in(1)); // good one
get_skeleton_predicates(entry, useful_predicates, true);
}
}
}

Expand All @@ -3571,21 +3576,31 @@ void PhaseIdealLoop::collect_potentially_useful_predicates(
// Note: it will also eliminates loop limits check predicate since it also uses
// Opaque1 node (see Parse::add_predicate()).
void PhaseIdealLoop::eliminate_useless_predicates() {
if (C->predicate_count() == 0)
if (C->predicate_count() == 0 && C->skeleton_predicate_count() == 0) {
return; // no predicate left
}

Unique_Node_List useful_predicates; // to store useful predicates
if (C->has_loops()) {
collect_potentially_useful_predicates(_ltree_root->_child, useful_predicates);
}

for (int i = C->predicate_count(); i > 0; i--) {
Node * n = C->predicate_opaque1_node(i-1);
Node* n = C->predicate_opaque1_node(i - 1);
assert(n->Opcode() == Op_Opaque1, "must be");
if (!useful_predicates.member(n)) { // not in the useful list
_igvn.replace_node(n, n->in(1));
}
}

for (int i = C->skeleton_predicate_count(); i > 0; i--) {
const int idx = i - 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be removed for consistency with above code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was a left over from a previous commit where we needed idx twice. I update that.

Node* n = C->skeleton_predicate_opaque4_node(idx);
assert(n->Opcode() == Op_Opaque4, "must be");
if (!useful_predicates.member(n)) { // not in the useful list
_igvn.replace_node(n, n->in(2));
}
}
}

//------------------------process_expensive_nodes-----------------------------
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/opto/loopnode.hpp
Expand Up @@ -914,7 +914,8 @@ class PhaseIdealLoop : public PhaseTransform {
IdealLoopTree* outer_loop, Node* input_proj);
Node* clone_skeleton_predicate_bool(Node* iff, Node* new_init, Node* new_stride, Node* predicate, Node* uncommon_proj, Node* control,
IdealLoopTree* outer_loop);
bool skeleton_predicate_has_opaque(IfNode* iff);
static bool skeleton_predicate_has_opaque(IfNode* iff);
static void get_skeleton_predicates(Node* predicate, Unique_Node_List& list, bool get_opaque = false);
void update_main_loop_skeleton_predicates(Node* ctrl, CountedLoopNode* loop_head, Node* init, int stride_con);
void insert_loop_limit_check(ProjNode* limit_check_proj, Node* cmp_limit, Node* bol);
#ifdef ASSERT
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/opto/node.cpp
Expand Up @@ -644,6 +644,9 @@ void Node::destruct(PhaseValues* phase) {
if (is_expensive()) {
compile->remove_expensive_node(this);
}
if (Opcode() == Op_Opaque4) {
compile->remove_skeleton_predicate_opaq(this);
}
if (for_post_loop_opts_igvn()) {
compile->remove_from_post_loop_opts_igvn(this);
}
Expand Down