8086046: escape analysis generates incorrect code as of B67

Load bypasses arraycopy that sets the value after the ArrayCopyNode is expanded

Reviewed-by: kvn

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

@@ -724,6 +724,26 @@ uint CallNode::match_edge(uint idx) const {
 //
 bool CallNode::may_modify(const TypeOopPtr *t_oop, PhaseTransform *phase) {
   assert((t_oop != NULL), "sanity");
+  if (is_call_to_arraycopystub()) {
+    const TypeTuple* args = _tf->domain();
+    Node* dest = NULL;
+    // Stubs that can be called once an ArrayCopyNode is expanded have
+    // different signatures. Look for the second pointer argument,
+    // that is the destination of the copy.
+    for (uint i = TypeFunc::Parms, j = 0; i < args->cnt(); i++) {
+      if (args->field_at(i)->isa_ptr()) {
+        j++;
+        if (j == 2) {
+          dest = in(i);
+          break;
+        }
+      }
+    }
+    if (!dest->is_top() && may_modify_arraycopy_helper(phase->type(dest)->is_oopptr(), t_oop, phase)) {
+      return true;
+    }
+    return false;
+  }
   if (t_oop->is_known_instance()) {
     // The instance_id is set only for scalar-replaceable allocations which
     // are not passed as arguments according to Escape Analysis.
@@ -909,6 +929,12 @@ Node *CallNode::Ideal(PhaseGVN *phase, bool can_reshape) {
   return SafePointNode::Ideal(phase, can_reshape);
 }
 
+bool CallNode::is_call_to_arraycopystub() const {
+  if (_name != NULL && strstr(_name, "arraycopy") != 0) {
+    return true;
+  }
+  return false;
+}
 
 //=============================================================================
 uint CallJavaNode::size_of() const { return sizeof(*this); }
@@ -1007,14 +1033,6 @@ void CallRuntimeNode::calling_convention( BasicType* sig_bt, VMRegPair *parm_reg
 
 
 //=============================================================================
-bool CallLeafNode::is_call_to_arraycopystub() const {
-  if (_name != NULL && strstr(_name, "arraycopy") != 0) {
-    return true;
-  }
-  return false;
-}
-
-
 #ifndef PRODUCT
 void CallLeafNode::dump_spec(outputStream *st) const {
   st->print("# ");
@@ -1930,26 +1948,3 @@ bool CallNode::may_modify_arraycopy_helper(const TypeOopPtr* dest_t, const TypeO
   return true;
 }
 
-bool CallLeafNode::may_modify(const TypeOopPtr *t_oop, PhaseTransform *phase) {
-  if (is_call_to_arraycopystub()) {
-    const TypeTuple* args = _tf->domain();
-    Node* dest = NULL;
-    // Stubs that can be called once an ArrayCopyNode is expanded have
-    // different signatures. Look for the second pointer argument,
-    // that is the destination of the copy.
-    for (uint i = TypeFunc::Parms, j = 0; i < args->cnt(); i++) {
-      if (args->field_at(i)->isa_ptr()) {
-        j++;
-        if (j == 2) {
-          dest = in(i);
-          break;
-        }
-      }
-    }
-    if (!dest->is_top() && may_modify_arraycopy_helper(phase->type(dest)->is_oopptr(), t_oop, phase)) {
-      return true;
-    }
-    return false;
-  }
-  return CallNode::may_modify(t_oop, phase);
-}

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

@@ -565,13 +565,15 @@ public:
   address      _entry_point;  // Address of method being called
   float        _cnt;          // Estimate of number of times called
   CallGenerator* _generator;  // corresponding CallGenerator for some late inline calls
+  const char *_name;           // Printable name, if _method is NULL
 
   CallNode(const TypeFunc* tf, address addr, const TypePtr* adr_type)
     : SafePointNode(tf->domain()->cnt(), NULL, adr_type),
       _tf(tf),
       _entry_point(addr),
       _cnt(COUNT_UNKNOWN),
-      _generator(NULL)
+      _generator(NULL),
+      _name(NULL)
   {
     init_class_id(Class_Call);
   }
@@ -630,6 +632,8 @@ public:
 
   virtual uint match_edge(uint idx) const;
 
+  bool is_call_to_arraycopystub() const;
+
 #ifndef PRODUCT
   virtual void        dump_req(outputStream *st = tty) const;
   virtual void        dump_spec(outputStream *st) const;
@@ -683,7 +687,7 @@ class CallStaticJavaNode : public CallJavaNode {
   virtual uint size_of() const; // Size is bigger
 public:
   CallStaticJavaNode(Compile* C, const TypeFunc* tf, address addr, ciMethod* method, int bci)
-    : CallJavaNode(tf, addr, method, bci), _name(NULL) {
+    : CallJavaNode(tf, addr, method, bci) {
     init_class_id(Class_CallStaticJava);
     if (C->eliminate_boxing() && (method != NULL) && method->is_boxing_method()) {
       init_flags(Flag_is_macro);
@@ -694,14 +698,14 @@ public:
   }
   CallStaticJavaNode(const TypeFunc* tf, address addr, const char* name, int bci,
                      const TypePtr* adr_type)
-    : CallJavaNode(tf, addr, NULL, bci), _name(name) {
+    : CallJavaNode(tf, addr, NULL, bci) {
     init_class_id(Class_CallStaticJava);
     // This node calls a runtime stub, which often has narrow memory effects.
     _adr_type = adr_type;
     _is_scalar_replaceable = false;
     _is_non_escaping = false;
+    _name = name;
   }
-  const char *_name;      // Runtime wrapper name
 
   // Result of Escape Analysis
   bool _is_scalar_replaceable;
@@ -754,13 +758,12 @@ class CallRuntimeNode : public CallNode {
 public:
   CallRuntimeNode(const TypeFunc* tf, address addr, const char* name,
                   const TypePtr* adr_type)
-    : CallNode(tf, addr, adr_type),
+    : CallNode(tf, addr, adr_type)
-      _name(name)
   {
     init_class_id(Class_CallRuntime);
+    _name = name;
   }
 
-  const char *_name;            // Printable name, if _method is NULL
   virtual int   Opcode() const;
   virtual void  calling_convention( BasicType* sig_bt, VMRegPair *parm_regs, uint argcnt ) const;
 
@@ -785,8 +788,6 @@ public:
 #ifndef PRODUCT
   virtual void  dump_spec(outputStream *st) const;
 #endif
-  bool is_call_to_arraycopystub() const;
-  virtual bool may_modify(const TypeOopPtr *t_oop, PhaseTransform *phase);
 };
 
 //------------------------------CallLeafNoFPNode-------------------------------

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

@@ -108,11 +108,10 @@ extern void print_alias_types();
 
 #endif
 
-static bool membar_for_arraycopy_helper(const TypeOopPtr *t_oop, MergeMemNode* mm, PhaseTransform *phase) {
+static bool membar_for_arraycopy_helper(const TypeOopPtr *t_oop, Node* n, PhaseTransform *phase) {
-  if (mm->memory_at(Compile::AliasIdxRaw)->is_Proj()) {
+  if (n->is_Proj()) {
-    Node* n = mm->memory_at(Compile::AliasIdxRaw)->in(0);
+    n = n->in(0);
-    if ((n->is_ArrayCopy() && n->as_ArrayCopy()->may_modify(t_oop, phase)) ||
+    if (n->is_Call() && n->as_Call()->may_modify(t_oop, phase)) {
-        (n->is_CallLeaf() && n->as_CallLeaf()->may_modify(t_oop, phase))) {
       return true;
     }
   }
@@ -121,16 +120,22 @@ static bool membar_for_arraycopy_helper(const TypeOopPtr *t_oop, MergeMemNode* m
 
 static bool membar_for_arraycopy(const TypeOopPtr *t_oop, MemBarNode* mb, PhaseTransform *phase) {
   Node* mem = mb->in(TypeFunc::Memory);
+
   if (mem->is_MergeMem()) {
-    return membar_for_arraycopy_helper(t_oop, mem->as_MergeMem(), phase);
+    Node* n = mem->as_MergeMem()->memory_at(Compile::AliasIdxRaw);
-  } else if (mem->is_Phi()) {
+    if (membar_for_arraycopy_helper(t_oop, n, phase)) {
-    // after macro expansion of an ArrayCopyNode we may have a Phi
+      return true;
-    for (uint i = 1; i < mem->req(); i++) {
+    } else if (n->is_Phi()) {
-      if (mem->in(i) != NULL && mem->in(i)->is_MergeMem() && membar_for_arraycopy_helper(t_oop, mem->in(i)->as_MergeMem(), phase)) {
+      for (uint i = 1; i < n->req(); i++) {
-        return true;
+        if (n->in(i) != NULL) {
+          if (membar_for_arraycopy_helper(t_oop, n->in(i), phase)) {
+            return true;
+          }
+        }
       }
     }
   }
+
   return false;
 }
 

hotspot/test/compiler/arraycopy/TestLoadBypassArrayCopy.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2015, 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 8086046
+ * @summary load bypasses arraycopy that sets the value after the ArrayCopyNode is expanded
+ * @run main/othervm -XX:-BackgroundCompilation  -XX:-UseOnStackReplacement -XX:CompileCommand=dontinline,TestLoadBypassArrayCopy::test_helper -XX:-TieredCompilation TestLoadBypassArrayCopy
+ *
+ */
+
+public class TestLoadBypassArrayCopy {
+
+    static long i;
+    static boolean test_helper() {
+        i++;
+        if ((i%10) == 0) {
+            return false;
+        }
+        return true;
+    }
+
+    static int test(int[] src, int len, boolean flag) {
+        int[] dest = new int[10];
+        int res = 0;
+        while (test_helper()) {
+            System.arraycopy(src, 0, dest, 0, len);
+            // predicate moved out of loop so control of following
+            // load is not the ArrayCopyNode. Otherwise, if the memory
+            // of the load is changed and the control is set to the
+            // ArrayCopyNode the graph is unschedulable and the test
+            // doesn't fail.
+            if (flag) {
+            }
+            // The memory of this load shouldn't bypass the arraycopy
+            res = dest[0];
+        }
+        return res;
+    }
+
+    static public void main(String[] args) {
+        int[] src = new int[10];
+        src[0] = 0x42;
+        for (int i = 0; i < 20000; i++) {
+            int res = test(src, 10, false);
+            if (res != src[0]) {
+                throw new RuntimeException("test failed");
+            }
+        }
+    }
+}