7044738: Loop unroll optimization causes incorrect result

Take into account memory dependencies when clonning nodes in clone_up_backedge_goo().

Reviewed-by: never

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

@@ -824,13 +824,23 @@ bool IdealLoopTree::policy_peel_only( PhaseIdealLoop *phase ) const {
 //------------------------------clone_up_backedge_goo--------------------------
 // If Node n lives in the back_ctrl block and cannot float, we clone a private
 // version of n in preheader_ctrl block and return that, otherwise return n.
-Node *PhaseIdealLoop::clone_up_backedge_goo( Node *back_ctrl, Node *preheader_ctrl, Node *n ) {
+Node *PhaseIdealLoop::clone_up_backedge_goo( Node *back_ctrl, Node *preheader_ctrl, Node *n, VectorSet &visited, Node_Stack &clones ) {
   if( get_ctrl(n) != back_ctrl ) return n;
 
+  // Only visit once
+  if (visited.test_set(n->_idx)) {
+    Node *x = clones.find(n->_idx);
+    if (x != NULL)
+      return x;
+    return n;
+  }
+
   Node *x = NULL;               // If required, a clone of 'n'
   // Check for 'n' being pinned in the backedge.
   if( n->in(0) && n->in(0) == back_ctrl ) {
+    assert(clones.find(n->_idx) == NULL, "dead loop");
     x = n->clone();             // Clone a copy of 'n' to preheader
+    clones.push(x, n->_idx);
     x->set_req( 0, preheader_ctrl ); // Fix x's control input to preheader
   }
 
@@ -838,10 +848,13 @@ Node *PhaseIdealLoop::clone_up_backedge_goo( Node *back_ctrl, Node *preheader_ct
   // If there are no changes we can just return 'n', otherwise
   // we need to clone a private copy and change it.
   for( uint i = 1; i < n->req(); i++ ) {
-    Node *g = clone_up_backedge_goo( back_ctrl, preheader_ctrl, n->in(i) );
+    Node *g = clone_up_backedge_goo( back_ctrl, preheader_ctrl, n->in(i), visited, clones );
     if( g != n->in(i) ) {
-      if( !x )
+      if( !x ) {
+        assert(clones.find(n->_idx) == NULL, "dead loop");
         x = n->clone();
+        clones.push(x, n->_idx);
+      }
       x->set_req(i, g);
     }
   }
@@ -960,6 +973,9 @@ void PhaseIdealLoop::insert_pre_post_loops( IdealLoopTree *loop, Node_List &old_
   post_head->set_req(LoopNode::EntryControl, zer_taken);
   set_idom(post_head, zer_taken, dd_main_exit);
 
+  Arena *a = Thread::current()->resource_area();
+  VectorSet visited(a);
+  Node_Stack clones(a, main_head->back_control()->outcnt());
   // Step A3: Make the fall-in values to the post-loop come from the
   // fall-out values of the main-loop.
   for (DUIterator_Fast imax, i = main_head->fast_outs(imax); i < imax; i++) {
@@ -968,7 +984,8 @@ void PhaseIdealLoop::insert_pre_post_loops( IdealLoopTree *loop, Node_List &old_
       Node *post_phi = old_new[main_phi->_idx];
       Node *fallmain  = clone_up_backedge_goo(main_head->back_control(),
                                               post_head->init_control(),
-                                              main_phi->in(LoopNode::LoopBackControl));
+                                              main_phi->in(LoopNode::LoopBackControl),
+                                              visited, clones);
       _igvn.hash_delete(post_phi);
       post_phi->set_req( LoopNode::EntryControl, fallmain );
     }
@@ -1032,6 +1049,8 @@ void PhaseIdealLoop::insert_pre_post_loops( IdealLoopTree *loop, Node_List &old_
   main_head->set_req(LoopNode::EntryControl, min_taken);
   set_idom(main_head, min_taken, dd_main_head);
 
+  visited.Clear();
+  clones.clear();
   // Step B3: Make the fall-in values to the main-loop come from the
   // fall-out values of the pre-loop.
   for (DUIterator_Fast i2max, i2 = main_head->fast_outs(i2max); i2 < i2max; i2++) {
@@ -1040,7 +1059,8 @@ void PhaseIdealLoop::insert_pre_post_loops( IdealLoopTree *loop, Node_List &old_
       Node *pre_phi = old_new[main_phi->_idx];
       Node *fallpre  = clone_up_backedge_goo(pre_head->back_control(),
                                              main_head->init_control(),
-                                             pre_phi->in(LoopNode::LoopBackControl));
+                                             pre_phi->in(LoopNode::LoopBackControl),
+                                             visited, clones);
       _igvn.hash_delete(main_phi);
       main_phi->set_req( LoopNode::EntryControl, fallpre );
     }

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

@@ -843,7 +843,7 @@ public:
   void insert_pre_post_loops( IdealLoopTree *loop, Node_List &old_new, bool peel_only );
   // If Node n lives in the back_ctrl block, we clone a private version of n
   // in preheader_ctrl block and return that, otherwise return n.
-  Node *clone_up_backedge_goo( Node *back_ctrl, Node *preheader_ctrl, Node *n );
+  Node *clone_up_backedge_goo( Node *back_ctrl, Node *preheader_ctrl, Node *n, VectorSet &visited, Node_Stack &clones );
 
   // Take steps to maximally unroll the loop.  Peel any odd iterations, then
   // unroll to do double iterations.  The next round of major loop transforms

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

@@ -391,13 +391,9 @@ Node *PhaseMacroExpand::value_from_mem_phi(Node *mem, BasicType ft, const Type *
     }
   }
   // Check if an appropriate new value phi already exists.
-  Node* new_phi = NULL;
+  Node* new_phi = value_phis->find(mem->_idx);
-  uint size = value_phis->size();
+  if (new_phi != NULL)
-  for (uint i=0; i < size; i++) {
+    return new_phi;
-    if ( mem->_idx == value_phis->index_at(i) ) {
-      return value_phis->node_at(i);
-    }
-  }
 
   if (level <= 0) {
     return NULL; // Give up: phi tree too deep

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

@@ -2012,6 +2012,16 @@ void Node_Stack::grow() {
   _inode_top = _inodes + old_top;        // restore _top
 }
 
+// Node_Stack is used to map nodes.
+Node* Node_Stack::find(uint idx) const {
+  uint sz = size();
+  for (uint i=0; i < sz; i++) {
+    if (idx == index_at(i) )
+      return node_at(i);
+  }
+  return NULL;
+}
+
 //=============================================================================
 uint TypeNode::size_of() const { return sizeof(*this); }
 #ifndef PRODUCT

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

@@ -1463,6 +1463,9 @@ public:
   bool is_nonempty() const { return (_inode_top >= _inodes); }
   bool is_empty() const { return (_inode_top < _inodes); }
   void clear() { _inode_top = _inodes - 1; } // retain storage
+
+  // Node_Stack is used to map nodes.
+  Node* find(uint idx) const;
 };
 
 

hotspot/test/compiler/7044738/Test7044738.java

@@ -0,0 +1,104 @@
+/*
+ * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+/**
+ * @test
+ * @bug 7044738
+ * @summary Loop unroll optimization causes incorrect result
+ *
+ * @run main/othervm -Xbatch Test7044738
+ */
+
+public class Test7044738 {
+
+  private static final int INITSIZE = 10000;
+  public int d[] = { 1, 2, 3, 4 };
+  public int i, size;
+
+  private static int iter = 5;
+
+  boolean done() { return (--iter > 0); }
+
+  public static void main(String args[]) {
+    Test7044738 t = new Test7044738();
+    t.test();
+  }
+
+  int test() {
+
+    while (done()) {
+      size = INITSIZE;
+
+      for (i = 0; i < size; i++) {
+        d[0] = d[1]; // 2
+        d[1] = d[2]; // 3
+        d[2] = d[3]; // 4
+        d[3] = d[0]; // 2
+
+        d[0] = d[1]; // 3
+        d[1] = d[2]; // 4
+        d[2] = d[3]; // 2
+        d[3] = d[0]; // 3
+
+        d[0] = d[1]; // 4
+        d[1] = d[2]; // 2
+        d[2] = d[3]; // 3
+        d[3] = d[0]; // 4
+
+        d[0] = d[1]; // 2
+        d[1] = d[2]; // 3
+        d[2] = d[3]; // 4
+        d[3] = d[0]; // 2
+
+        d[0] = d[1]; // 3
+        d[1] = d[2]; // 4
+        d[2] = d[3]; // 2
+        d[3] = d[0]; // 3
+
+        d[0] = d[1]; // 4
+        d[1] = d[2]; // 2
+        d[2] = d[3]; // 3
+        d[3] = d[0]; // 4
+
+        d[0] = d[1]; // 2
+        d[1] = d[2]; // 3
+        d[2] = d[3]; // 4
+        d[3] = d[0]; // 2
+
+        d[0] = d[1]; // 3
+        d[1] = d[2]; // 4
+        d[2] = d[3]; // 2
+        d[3] = d[0]; // 3
+      }
+
+      // try to defeat dead code elimination
+      if (d[0] == d[1]) {
+        System.out.println("test failed: iter=" + iter + "  i=" + i + " d[] = { " + d[0] + ", " + d[1] + ", " + d[2] + ", " + d[3] + " } ");
+        System.exit(97);
+      }
+    }
+    return d[3];
+  }
+
+}

hotspot/test/compiler/7046096/Test7046096.java

@@ -0,0 +1,65 @@
+/*
+ * Copyright (c) 2011, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+/**
+ * @test
+ * @bug 7046096
+ * @summary SEGV IN C2 WITH 6U25
+ *
+ * @run main/othervm -Xbatch -XX:+IgnoreUnrecognizedVMOptions -XX:+OptimizeStringConcat Test7046096
+ */
+
+
+public class Test7046096 {
+
+  static int first = 1;
+
+  String add(String str) {
+    if (first != 0) {
+      return str + "789";
+    } else {
+      return "null";
+    }
+  }
+
+  String test(String str) {
+    for (int i=0; i < first; i++) {
+      if (i > 1)
+        return "bad";
+    }
+    return add(str+"456");
+  }
+
+  public static void main(String [] args) {
+    Test7046096 t = new Test7046096();
+    for (int i = 0; i < 11000; i++) {
+      String str = t.test("123");
+      if (!str.equals("123456789")) {
+        System.out.println("FAILED: " + str + " != \"123456789\"");
+        System.exit(97);
+      }
+    }
+  }
+}
+