8007294: ReduceFieldZeroing doesn't check for dependent load and can lead to incorrect execution

InitializeNode::can_capture_store() must check that the captured store doesn't overwrite a memory location that is loaded before the store.

Reviewed-by: kvn

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

@@ -320,6 +320,9 @@ Node *MemNode::Ideal_common(PhaseGVN *phase, bool can_reshape) {
 
   if (mem != old_mem) {
     set_req(MemNode::Memory, mem);
+    if (can_reshape && old_mem->outcnt() == 0) {
+        igvn->_worklist.push(old_mem);
+    }
     if (phase->type( mem ) == Type::TOP) return NodeSentinel;
     return this;
   }
@@ -2319,9 +2322,9 @@ Node *StoreNode::Ideal(PhaseGVN *phase, bool can_reshape) {
   if (ReduceFieldZeroing && /*can_reshape &&*/
       mem->is_Proj() && mem->in(0)->is_Initialize()) {
     InitializeNode* init = mem->in(0)->as_Initialize();
-    intptr_t offset = init->can_capture_store(this, phase);
+    intptr_t offset = init->can_capture_store(this, phase, can_reshape);
     if (offset > 0) {
-      Node* moved = init->capture_store(this, offset, phase);
+      Node* moved = init->capture_store(this, offset, phase, can_reshape);
       // If the InitializeNode captured me, it made a raw copy of me,
       // and I need to disappear.
       if (moved != NULL) {
@@ -3134,7 +3137,7 @@ bool InitializeNode::detect_init_independence(Node* n,
 // an initialization.  Returns zero if a check fails.
 // On success, returns the (constant) offset to which the store applies,
 // within the initialized memory.
-intptr_t InitializeNode::can_capture_store(StoreNode* st, PhaseTransform* phase) {
+intptr_t InitializeNode::can_capture_store(StoreNode* st, PhaseTransform* phase, bool can_reshape) {
   const int FAIL = 0;
   if (st->req() != MemNode::ValueIn + 1)
     return FAIL;                // an inscrutable StoreNode (card mark?)
@@ -3156,6 +3159,91 @@ intptr_t InitializeNode::can_capture_store(StoreNode* st, PhaseTransform* phase)
   if (!detect_init_independence(val, true, complexity_count))
     return FAIL;                // stored value must be 'simple enough'
 
+  // The Store can be captured only if nothing after the allocation
+  // and before the Store is using the memory location that the store
+  // overwrites.
+  bool failed = false;
+  // If is_complete_with_arraycopy() is true the shape of the graph is
+  // well defined and is safe so no need for extra checks.
+  if (!is_complete_with_arraycopy()) {
+    // We are going to look at each use of the memory state following
+    // the allocation to make sure nothing reads the memory that the
+    // Store writes.
+    const TypePtr* t_adr = phase->type(adr)->isa_ptr();
+    int alias_idx = phase->C->get_alias_index(t_adr);
+    ResourceMark rm;
+    Unique_Node_List mems;
+    mems.push(mem);
+    Node* unique_merge = NULL;
+    for (uint next = 0; next < mems.size(); ++next) {
+      Node *m  = mems.at(next);
+      for (DUIterator_Fast jmax, j = m->fast_outs(jmax); j < jmax; j++) {
+        Node *n = m->fast_out(j);
+        if (n->outcnt() == 0) {
+          continue;
+        }
+        if (n == st) {
+          continue;
+        } else if (n->in(0) != NULL && n->in(0) != ctl) {
+          // If the control of this use is different from the control
+          // of the Store which is right after the InitializeNode then
+          // this node cannot be between the InitializeNode and the
+          // Store.
+          continue;
+        } else if (n->is_MergeMem()) {
+          if (n->as_MergeMem()->memory_at(alias_idx) == m) {
+            // We can hit a MergeMemNode (that will likely go away
+            // later) that is a direct use of the memory state
+            // following the InitializeNode on the same slice as the
+            // store node that we'd like to capture. We need to check
+            // the uses of the MergeMemNode.
+            mems.push(n);
+          }
+        } else if (n->is_Mem()) {
+          Node* other_adr = n->in(MemNode::Address);
+          if (other_adr == adr) {
+            failed = true;
+            break;
+          } else {
+            const TypePtr* other_t_adr = phase->type(other_adr)->isa_ptr();
+            if (other_t_adr != NULL) {
+              int other_alias_idx = phase->C->get_alias_index(other_t_adr);
+              if (other_alias_idx == alias_idx) {
+                // A load from the same memory slice as the store right
+                // after the InitializeNode. We check the control of the
+                // object/array that is loaded from. If it's the same as
+                // the store control then we cannot capture the store.
+                assert(!n->is_Store(), "2 stores to same slice on same control?");
+                Node* base = other_adr;
+                assert(base->is_AddP(), err_msg_res("should be addp but is %s", base->Name()));
+                base = base->in(AddPNode::Base);
+                if (base != NULL) {
+                  base = base->uncast();
+                  if (base->is_Proj() && base->in(0) == alloc) {
+                    failed = true;
+                    break;
+                  }
+                }
+              }
+            }
+          }
+        } else {
+          failed = true;
+          break;
+        }
+      }
+    }
+  }
+  if (failed) {
+    if (!can_reshape) {
+      // We decided we couldn't capture the store during parsing. We
+      // should try again during the next IGVN once the graph is
+      // cleaner.
+      phase->C->record_for_igvn(st);
+    }
+    return FAIL;
+  }
+
   return offset;                // success
 }
 
@@ -3266,11 +3354,11 @@ Node* InitializeNode::make_raw_address(intptr_t offset,
 //                      rawstore1 rawstore2)
 //
 Node* InitializeNode::capture_store(StoreNode* st, intptr_t start,
-                                    PhaseTransform* phase) {
+                                    PhaseTransform* phase, bool can_reshape) {
   assert(stores_are_sane(phase), "");
 
   if (start < 0)  return NULL;
-  assert(can_capture_store(st, phase) == start, "sanity");
+  assert(can_capture_store(st, phase, can_reshape) == start, "sanity");
 
   Compile* C = phase->C;
   int size_in_bytes = st->memory_size();

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

@@ -1072,11 +1072,11 @@ public:
 
   // See if this store can be captured; return offset where it initializes.
   // Return 0 if the store cannot be moved (any sort of problem).
-  intptr_t can_capture_store(StoreNode* st, PhaseTransform* phase);
+  intptr_t can_capture_store(StoreNode* st, PhaseTransform* phase, bool can_reshape);
 
   // Capture another store; reformat it to write my internal raw memory.
   // Return the captured copy, else NULL if there is some sort of problem.
-  Node* capture_store(StoreNode* st, intptr_t start, PhaseTransform* phase);
+  Node* capture_store(StoreNode* st, intptr_t start, PhaseTransform* phase, bool can_reshape);
 
   // Find captured store which corresponds to the range [start..start+size).
   // Return my own memory projection (meaning the initial zero bits)

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

@@ -1261,6 +1261,7 @@ static void kill_dead_code( Node *dead, PhaseIterGVN *igvn ) {
       if (dead->is_expensive()) {
         igvn->C->remove_expensive_node(dead);
       }
+      igvn->C->record_dead_node(dead->_idx);
       // Kill all inputs to the dead guy
       for (uint i=0; i < dead->req(); i++) {
         Node *n = dead->in(i);      // Get input to dead guy

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

@@ -1197,6 +1197,18 @@ void PhaseIterGVN::remove_globally_dead_node( Node *dead ) {
                 assert(!(i < imax), "sanity");
               }
             }
+            if (ReduceFieldZeroing && dead->is_Load() && i == MemNode::Memory &&
+                in->is_Proj() && in->in(0) != NULL && in->in(0)->is_Initialize()) {
+              // A Load that directly follows an InitializeNode is
+              // going away. The Stores that follow are candidates
+              // again to be captured by the InitializeNode.
+              for (DUIterator_Fast jmax, j = in->fast_outs(jmax); j < jmax; j++) {
+                Node *n = in->fast_out(j);
+                if (n->is_Store()) {
+                  _worklist.push(n);
+                }
+              }
+            }
           }
         }
         C->record_dead_node(dead->_idx);

hotspot/test/compiler/8007294/Test8007294.java

@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2013, 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 8007294
+ * @summary ReduceFieldZeroing doesn't check for dependent load and can lead to incorrect execution
+ * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+AlwaysIncrementalInline -XX:-UseOnStackReplacement -XX:-BackgroundCompilation Test8007294
+ *
+ */
+
+public class Test8007294 {
+
+    int i1;
+    int i2;
+
+    Test8007294(int i1, int i2) {
+        this.i1 = i1;
+        this.i2 = i2;
+    }
+
+    static int m(int v) {
+        return v;
+    }
+
+    static Test8007294 test1() {
+        Test8007294 obj = new Test8007294(10, 100);
+        int v1 = obj.i1;
+
+        int v3 = m(v1);
+        int v2 = obj.i2;
+        obj.i2 = v3;
+        obj.i1 = v2;
+
+        return obj;
+    }
+
+    static int test2(int i) {
+        int j = 0;
+        if (i > 0) {
+            j = 1;
+        }
+
+        int[] arr = new int[10];
+        arr[0] = 1;
+        arr[1] = 2;
+        int v1 = arr[j];
+        arr[0] = 3;
+        arr[1] = 4;
+
+        return v1;
+    }
+
+    static public void main(String[] args) {
+        boolean failed = false;
+        for (int i = 0; i < 20000; i++) {
+            Test8007294 obj = test1();
+            if (obj.i1 != 100 || obj.i2 != 10) {
+                System.out.println("FAILED test1 obj.i1 = " + obj.i1 +", obj.i2 = " + obj.i2);
+                failed = true;
+                break;
+            }
+        }
+        for (int i = 0; i < 20000; i++) {
+            int res = test2(1);
+            if (res != 2) {
+                System.out.println("FAILED test2 = " + res);
+                failed = true;
+                break;
+            }
+        }
+        if (failed) {
+            System.exit(97);
+        } else {
+            System.out.println("PASSED");
+        }
+    }
+}