6686791: Side effect in NumberFormat tests with -server -Xcomp

Optimization in CmpPNode::sub() removed the valid compare instruction because of false positive answer from detect_dominating_control().

Reviewed-by: jrose, sgoldman

hotspot/src/share/vm/opto/memnode.cpp

@@ -241,36 +241,91 @@ Node *MemNode::Ideal_common(PhaseGVN *phase, bool can_reshape) {
 }
 
 // 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) {
+bool MemNode::all_controls_dominate(Node* dom, Node* sub) {
-  if (dom == NULL)      return false;
+  if (dom == NULL || dom->is_top() || sub == NULL || sub->is_top())
-  if (dom->is_Proj())   dom = dom->in(0);
+    return false; // Conservative answer for dead code
-  if (dom->is_Start())  return true; // anything inside the method
+
-  if (dom->is_Root())   return true; // dom 'controls' a constant
+  // Check 'dom'.
-  int cnt = 20;                      // detect cycle or too much effort
+  dom = dom->find_exact_control(dom);
-  while (sub != NULL) {              // walk 'sub' up the chain to 'dom'
+  if (dom == NULL || dom->is_top())
-    if (--cnt < 0)   return false;   // in a cycle or too complex
+    return false; // Conservative answer for dead code
-    if (sub == dom)  return true;
+
-    if (sub->is_Start())  return false;
+  if (dom->is_Start() || dom->is_Root() || dom == sub)
-    if (sub->is_Root())   return false;
+    return true;
-    Node* up = sub->in(0);
+
-    if (sub == up && sub->is_Region()) {
+  // 'dom' dominates 'sub' if its control edge and control edges
-      for (uint i = 1; i < sub->req(); i++) {
+  // of all its inputs dominate or equal to sub's control edge.
-        Node* in = sub->in(i);
+
-        if (in != NULL && !in->is_top() && in != sub) {
+  // Currently 'sub' is either Allocate, Initialize or Start nodes.
-          up = in; break;            // take any path on the way up to 'dom'
+  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
+    return only_dominating_controls;
-    sub = up;
   }
-  return false;
 }
 
 //---------------------detect_ptr_independence---------------------------------
@@ -291,9 +346,9 @@ bool MemNode::detect_ptr_independence(Node* p1, AllocateNode* a1,
     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 @@ Node* MemNode::find_previous_store(PhaseTransform* phase) {
         known_identical = true;
       else if (alloc != NULL)
         known_independent = true;
-      else if (ctrl != NULL &&
+      else if (all_controls_dominate(this, st_alloc))
-               detect_dominating_control(ctrl, st_alloc->in(0)))
         known_independent = true;
 
       if (known_independent) {
@@ -1093,7 +1147,7 @@ Node *LoadNode::Ideal(PhaseGVN *phase, bool can_reshape) {
     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 @@ bool InitializeNode::detect_init_independence(Node* n,
     // 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
 
   }

hotspot/src/share/vm/opto/memnode.hpp

@@ -70,7 +70,7 @@ public:
   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 );

hotspot/src/share/vm/opto/node.cpp

@@ -1017,6 +1017,101 @@ bool Node::has_special_unique_user() const {
   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

hotspot/src/share/vm/opto/node.hpp

@@ -817,6 +817,12 @@ public:
   // 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: