6686791: Side effect in NumberFormat tests with -server -Xcomp
Summary: Optimization in CmpPNode::sub() removed the valid compare instruction because of false positive answer from detect_dominating_control().
Reviewed-by: jrose, sgoldman
--- a/hotspot/src/share/vm/opto/memnode.cpp Wed Apr 16 14:55:36 2008 -0700
+++ b/hotspot/src/share/vm/opto/memnode.cpp Wed Apr 16 19:19:48 2008 -0700
@@ -241,36 +241,91 @@
}
// Helper function for proving some simple control dominations.
-// Attempt to prove that control input 'dom' dominates (or equals) 'sub'.
+// Attempt to prove that all control inputs of 'dom' dominate 'sub'.
// Already assumes that 'dom' is available at 'sub', and that 'sub'
// is not a constant (dominated by the method's StartNode).
// Used by MemNode::find_previous_store to prove that the
// control input of a memory operation predates (dominates)
// an allocation it wants to look past.
-bool MemNode::detect_dominating_control(Node* dom, Node* sub) {
- if (dom == NULL) return false;
- if (dom->is_Proj()) dom = dom->in(0);
- if (dom->is_Start()) return true; // anything inside the method
- if (dom->is_Root()) return true; // dom 'controls' a constant
- int cnt = 20; // detect cycle or too much effort
- while (sub != NULL) { // walk 'sub' up the chain to 'dom'
- if (--cnt < 0) return false; // in a cycle or too complex
- if (sub == dom) return true;
- if (sub->is_Start()) return false;
- if (sub->is_Root()) return false;
- Node* up = sub->in(0);
- if (sub == up && sub->is_Region()) {
- for (uint i = 1; i < sub->req(); i++) {
- Node* in = sub->in(i);
- if (in != NULL && !in->is_top() && in != sub) {
- up = in; break; // take any path on the way up to 'dom'
+bool MemNode::all_controls_dominate(Node* dom, Node* sub) {
+ if (dom == NULL || dom->is_top() || sub == NULL || sub->is_top())
+ return false; // Conservative answer for dead code
+
+ // Check 'dom'.
+ dom = dom->find_exact_control(dom);
+ if (dom == NULL || dom->is_top())
+ return false; // Conservative answer for dead code
+
+ if (dom->is_Start() || dom->is_Root() || dom == sub)
+ return true;
+
+ // 'dom' dominates 'sub' if its control edge and control edges
+ // of all its inputs dominate or equal to sub's control edge.
+
+ // Currently 'sub' is either Allocate, Initialize or Start nodes.
+ assert(sub->is_Allocate() || sub->is_Initialize() || sub->is_Start(), "expecting only these nodes");
+
+ // Get control edge of 'sub'.
+ sub = sub->find_exact_control(sub->in(0));
+ if (sub == NULL || sub->is_top())
+ return false; // Conservative answer for dead code
+
+ assert(sub->is_CFG(), "expecting control");
+
+ if (sub == dom)
+ return true;
+
+ if (sub->is_Start() || sub->is_Root())
+ return false;
+
+ {
+ // Check all control edges of 'dom'.
+
+ ResourceMark rm;
+ Arena* arena = Thread::current()->resource_area();
+ Node_List nlist(arena);
+ Unique_Node_List dom_list(arena);
+
+ dom_list.push(dom);
+ bool only_dominating_controls = false;
+
+ for (uint next = 0; next < dom_list.size(); next++) {
+ Node* n = dom_list.at(next);
+ if (!n->is_CFG() && n->pinned()) {
+ // Check only own control edge for pinned non-control nodes.
+ n = n->find_exact_control(n->in(0));
+ if (n == NULL || n->is_top())
+ return false; // Conservative answer for dead code
+ assert(n->is_CFG(), "expecting control");
+ }
+ if (n->is_Start() || n->is_Root()) {
+ only_dominating_controls = true;
+ } else if (n->is_CFG()) {
+ if (n->dominates(sub, nlist))
+ only_dominating_controls = true;
+ else
+ return false;
+ } else {
+ // First, own control edge.
+ Node* m = n->find_exact_control(n->in(0));
+ if (m == NULL)
+ continue;
+ if (m->is_top())
+ return false; // Conservative answer for dead code
+ dom_list.push(m);
+
+ // Now, the rest of edges.
+ uint cnt = n->req();
+ for (uint i = 1; i < cnt; i++) {
+ m = n->find_exact_control(n->in(i));
+ if (m == NULL || m->is_top())
+ continue;
+ dom_list.push(m);
}
}
}
- if (sub == up) return false; // some kind of tight cycle
- sub = up;
+ return only_dominating_controls;
}
- return false;
}
//---------------------detect_ptr_independence---------------------------------
@@ -291,9 +346,9 @@
return (a1 != a2);
} else if (a1 != NULL) { // one allocation a1
// (Note: p2->is_Con implies p2->in(0)->is_Root, which dominates.)
- return detect_dominating_control(p2->in(0), a1->in(0));
+ return all_controls_dominate(p2, a1);
} else { //(a2 != NULL) // one allocation a2
- return detect_dominating_control(p1->in(0), a2->in(0));
+ return all_controls_dominate(p1, a2);
}
return false;
}
@@ -379,8 +434,7 @@
known_identical = true;
else if (alloc != NULL)
known_independent = true;
- else if (ctrl != NULL &&
- detect_dominating_control(ctrl, st_alloc->in(0)))
+ else if (all_controls_dominate(this, st_alloc))
known_independent = true;
if (known_independent) {
@@ -1093,7 +1147,7 @@
Node* base = AddPNode::Ideal_base_and_offset(address, phase, ignore);
if (base != NULL
&& phase->type(base)->higher_equal(TypePtr::NOTNULL)
- && detect_dominating_control(base->in(0), phase->C->start())) {
+ && all_controls_dominate(base, phase->C->start())) {
// A method-invariant, non-null address (constant or 'this' argument).
set_req(MemNode::Control, NULL);
}
@@ -2536,7 +2590,7 @@
// must have preceded the init, or else be equal to the init.
// Even after loop optimizations (which might change control edges)
// a store is never pinned *before* the availability of its inputs.
- if (!MemNode::detect_dominating_control(ctl, this->in(0)))
+ if (!MemNode::all_controls_dominate(n, this))
return false; // failed to prove a good control
}
--- a/hotspot/src/share/vm/opto/memnode.hpp Wed Apr 16 14:55:36 2008 -0700
+++ b/hotspot/src/share/vm/opto/memnode.hpp Wed Apr 16 19:19:48 2008 -0700
@@ -70,7 +70,7 @@
static Node *optimize_simple_memory_chain(Node *mchain, const TypePtr *t_adr, PhaseGVN *phase);
static Node *optimize_memory_chain(Node *mchain, const TypePtr *t_adr, PhaseGVN *phase);
// This one should probably be a phase-specific function:
- static bool detect_dominating_control(Node* dom, Node* sub);
+ static bool all_controls_dominate(Node* dom, Node* sub);
// Is this Node a MemNode or some descendent? Default is YES.
virtual Node *Ideal_DU_postCCP( PhaseCCP *ccp );
--- a/hotspot/src/share/vm/opto/node.cpp Wed Apr 16 14:55:36 2008 -0700
+++ b/hotspot/src/share/vm/opto/node.cpp Wed Apr 16 19:19:48 2008 -0700
@@ -1017,6 +1017,101 @@
return false;
};
+//--------------------------find_exact_control---------------------------------
+// Skip Proj and CatchProj nodes chains. Check for Null and Top.
+Node* Node::find_exact_control(Node* ctrl) {
+ if (ctrl == NULL && this->is_Region())
+ ctrl = this->as_Region()->is_copy();
+
+ if (ctrl != NULL && ctrl->is_CatchProj()) {
+ if (ctrl->as_CatchProj()->_con == CatchProjNode::fall_through_index)
+ ctrl = ctrl->in(0);
+ if (ctrl != NULL && !ctrl->is_top())
+ ctrl = ctrl->in(0);
+ }
+
+ if (ctrl != NULL && ctrl->is_Proj())
+ ctrl = ctrl->in(0);
+
+ return ctrl;
+}
+
+//--------------------------dominates------------------------------------------
+// Helper function for MemNode::all_controls_dominate().
+// Check if 'this' control node dominates or equal to 'sub' control node.
+bool Node::dominates(Node* sub, Node_List &nlist) {
+ assert(this->is_CFG(), "expecting control");
+ assert(sub != NULL && sub->is_CFG(), "expecting control");
+
+ Node* orig_sub = sub;
+ nlist.clear();
+ bool this_dominates = false;
+ uint region_input = 0;
+ while (sub != NULL) { // walk 'sub' up the chain to 'this'
+ if (sub == this) {
+ if (nlist.size() == 0) {
+ // No Region nodes except loops were visited before and the EntryControl
+ // path was taken for loops: it did not walk in a cycle.
+ return true;
+ } else if (!this_dominates) {
+ // Region nodes were visited. Continue walk up to Start or Root
+ // to make sure that it did not walk in a cycle.
+ this_dominates = true; // first time meet
+ } else {
+ return false; // already met before: walk in a cycle
+ }
+ }
+ if (sub->is_Start() || sub->is_Root())
+ return this_dominates;
+
+ Node* up = sub->find_exact_control(sub->in(0));
+ if (up == NULL || up->is_top())
+ return false; // Conservative answer for dead code
+
+ if (sub == up && sub->is_Loop()) {
+ up = sub->in(0); // in(LoopNode::EntryControl);
+ } else if (sub == up && sub->is_Region()) {
+ uint i = 1;
+ if (nlist.size() == 0) {
+ // No Region nodes (except Loops) were visited before.
+ // Take first valid path on the way up to 'this'.
+ } else if (nlist.at(nlist.size() - 1) == sub) {
+ // This Region node was just visited. Take other path.
+ i = region_input + 1;
+ nlist.pop();
+ } else {
+ // Was this Region node visited before?
+ uint size = nlist.size();
+ for (uint j = 0; j < size; j++) {
+ if (nlist.at(j) == sub) {
+ return false; // The Region node was visited before. Give up.
+ }
+ }
+ // The Region node was not visited before.
+ // Take first valid path on the way up to 'this'.
+ }
+ for (; i < sub->req(); i++) {
+ Node* in = sub->in(i);
+ if (in != NULL && !in->is_top() && in != sub) {
+ break;
+ }
+ }
+ if (i < sub->req()) {
+ nlist.push(sub);
+ up = sub->in(i);
+ region_input = i;
+ }
+ }
+ if (sub == up)
+ return false; // some kind of tight cycle
+ if (orig_sub == up)
+ return false; // walk in a cycle
+
+ sub = up;
+ }
+ return false;
+}
+
//------------------------------remove_dead_region-----------------------------
// This control node is dead. Follow the subgraph below it making everything
// using it dead as well. This will happen normally via the usual IterGVN
--- a/hotspot/src/share/vm/opto/node.hpp Wed Apr 16 14:55:36 2008 -0700
+++ b/hotspot/src/share/vm/opto/node.hpp Wed Apr 16 19:19:48 2008 -0700
@@ -817,6 +817,12 @@
// for the transformations to happen.
bool has_special_unique_user() const;
+ // Skip Proj and CatchProj nodes chains. Check for Null and Top.
+ Node* find_exact_control(Node* ctrl);
+
+ // Check if 'this' node dominates or equal to 'sub'.
+ bool dominates(Node* sub, Node_List &nlist);
+
protected:
bool remove_dead_region(PhaseGVN *phase, bool can_reshape);
public: