8238438: SuperWord::co_locate_pack picks memory state of first instead of last load

Summary: Fix selection of first and last memory state in SuperWord::co_locate_pack
Reviewed-by: thartmann, kvn

src/hotspot/share/opto/superword.cpp

@@ -2141,55 +2141,88 @@ void SuperWord::co_locate_pack(Node_List* pk) {
         _igvn.replace_input_of(ld, MemNode::Memory, upper_insert_pt);
       }
     }
-  } else if (pk->at(0)->is_Load()) { //load
-    // all loads in the pack should have the same memory state. By default,
+  } else if (pk->at(0)->is_Load()) { // Load pack
+    // All loads in the pack should have the same memory state. By default,
     // we use the memory state of the last load. However, if any load could
     // not be moved down due to the dependence constraint, we use the memory
     // state of the first load.
-    Node* last_mem  = pk->at(0)->in(MemNode::Memory);
-    Node* first_mem = last_mem;
-    // Walk the memory graph from the current first load until the
-    // start of the loop and check if nodes on the way are memory
-    // edges of loads in the pack. The last one we encounter is the
-    // first load.
-    for (Node* current = first_mem; in_bb(current); current = current->is_Phi() ? current->in(LoopNode::EntryControl) : current->in(MemNode::Memory)) {
-     assert(current->is_Mem() || (current->is_Phi() && current->in(0) == bb()), "unexpected memory");
-     for (uint i = 1; i < pk->size(); i++) {
-        Node* ld = pk->at(i);
-        if (ld->in(MemNode::Memory) == current) {
-          first_mem = current;
-          break;
-        }
+    Node* mem_input = pick_mem_state(pk);
+    _igvn.hash_delete(mem_input);
+    // Give each load the same memory state
+    for (uint i = 0; i < pk->size(); i++) {
+      LoadNode* ld = pk->at(i)->as_Load();
+      _igvn.replace_input_of(ld, MemNode::Memory, mem_input);
+    }
+  }
+}
+
+// Finds the first and last memory state and then picks either of them by checking dependence constraints.
+// If a store is dependent on an earlier load then we need to pick the memory state of the first load and cannot
+// pick the memory state of the last load.
+Node* SuperWord::pick_mem_state(Node_List* pk) {
+  Node* first_mem = find_first_mem_state(pk);
+  Node* last_mem  = find_last_mem_state(pk, first_mem);
+
+  for (uint i = 0; i < pk->size(); i++) {
+    Node* ld = pk->at(i);
+    for (Node* current = last_mem; current != ld->in(MemNode::Memory); current = current->in(MemNode::Memory)) {
+      assert(current->is_Mem() && in_bb(current), "unexpected memory");
+      assert(current != first_mem, "corrupted memory graph");
+      if (!independent(current, ld)) {
+#ifdef ASSERT
+        // Added assertion code since no case has been observed that should pick the first memory state.
+        // Remove the assertion code whenever we find a (valid) case that really needs the first memory state.
+        pk->dump();
+        first_mem->dump();
+        last_mem->dump();
+        current->dump();
+        ld->dump();
+        ld->in(MemNode::Memory)->dump();
+        assert(false, "never observed that first memory should be picked");
+#endif
+        return first_mem; // A later store depends on this load, pick memory state of first load
       }
     }
-    // Find the last load by going over the pack again and walking
-    // the memory graph from the loads of the pack to the memory of
-    // the first load. If we encounter the memory of the current last
-    // load, then we started from further down in the memory graph and
-    // the load we started from is the last load. Check for dependence
-    // constraints in that loop as well.
-    bool schedule_last = true;
-    for (uint i = 0; i < pk->size(); i++) {
+  }
+  return last_mem;
+}
+
+// Walk the memory graph from the current first load until the
+// start of the loop and check if nodes on the way are memory
+// edges of loads in the pack. The last one we encounter is the
+// first load.
+Node* SuperWord::find_first_mem_state(Node_List* pk) {
+  Node* first_mem = pk->at(0)->in(MemNode::Memory);
+  for (Node* current = first_mem; in_bb(current); current = current->is_Phi() ? current->in(LoopNode::EntryControl) : current->in(MemNode::Memory)) {
+    assert(current->is_Mem() || (current->is_Phi() && current->in(0) == bb()), "unexpected memory");
+    for (uint i = 1; i < pk->size(); i++) {
       Node* ld = pk->at(i);
-      for (Node* current = ld->in(MemNode::Memory); current != first_mem; current = current->in(MemNode::Memory)) {
-        assert(current->is_Mem() && in_bb(current), "unexpected memory");
-        if (current->in(MemNode::Memory) == last_mem) {
-          last_mem = ld->in(MemNode::Memory);
-        }
-        if (!independent(current, ld)) {
-          schedule_last = false; // a later store depends on this load
-        }
+      if (ld->in(MemNode::Memory) == current) {
+        first_mem = current;
+        break;
       }
     }
+  }
+  return first_mem;
+}
 
-    Node* mem_input = schedule_last ? last_mem : first_mem;
-    _igvn.hash_delete(mem_input);
-    // Give each load the same memory state
-    for (uint i = 0; i < pk->size(); i++) {
-      LoadNode* ld = pk->at(i)->as_Load();
-      _igvn.replace_input_of(ld, MemNode::Memory, mem_input);
+// Find the last load by going over the pack again and walking
+// the memory graph from the loads of the pack to the memory of
+// the first load. If we encounter the memory of the current last
+// load, then we started from further down in the memory graph and
+// the load we started from is the last load.
+Node* SuperWord::find_last_mem_state(Node_List* pk, Node* first_mem) {
+  Node* last_mem = pk->at(0)->in(MemNode::Memory);
+  for (uint i = 0; i < pk->size(); i++) {
+    Node* ld = pk->at(i);
+    for (Node* current = ld->in(MemNode::Memory); current != first_mem; current = current->in(MemNode::Memory)) {
+      assert(current->is_Mem() && in_bb(current), "unexpected memory");
+      if (current->in(MemNode::Memory) == last_mem) {
+        last_mem = ld->in(MemNode::Memory);
+      }
     }
   }
+  return last_mem;
 }
 
 #ifndef PRODUCT

src/hotspot/share/opto/superword.hpp

@@ -480,6 +480,10 @@ class SuperWord : public ResourceObj {
   // Within a store pack, schedule stores together by moving out the sandwiched memory ops according
   // to dependence info; and within a load pack, move loads down to the last executed load.
   void co_locate_pack(Node_List* p);
+  Node* pick_mem_state(Node_List* pk);
+  Node* find_first_mem_state(Node_List* pk);
+  Node* find_last_mem_state(Node_List* pk, Node* first_mem);
+
   // Convert packs into vector node operations
   void output();
   // Create a vector operand for the nodes in pack p for operand: in(opd_idx)

test/hotspot/jtreg/compiler/loopopts/superword/CoLocatePackMemoryState.java

+/*
+ * Copyright (c) 2020, 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
+ * @requires vm.compiler2.enabled
+ * @bug 8238438
+ * @summary Tests to select the memory state of the last load in a load pack in SuperWord::co_locate_pack.
+ *
+ * @run main/othervm -Xbatch -XX:CompileCommand=compileonly,compiler.loopopts.superword.CoLocatePackMemoryState::test
+ *      -XX:LoopMaxUnroll=16 compiler.loopopts.superword.CoLocatePackMemoryState
+ */
+
+package compiler.loopopts.superword;
+
+public class CoLocatePackMemoryState {
+
+    public static final int N = 64;
+    public static byte byFld;
+    public static int iArr[] = new int[N+1];
+
+    public static void test() {
+        // Needs to pick the memory state of the last load for the iArr[i] load pack in SuperWord::co_locate_pack
+        // since it is dependent on the iArr[i+1] stores.
+        for (int i = 0; i < N; ++i) {
+            iArr[i+1] = i;
+            iArr[i] -= 15;
+            byFld += 35;
+        }
+    }
+
+    public static void main(String[] strArr) {
+        for (int i = 0; i < 2000; i++) {
+            for (int j = 0; j < N; j++) {
+                iArr[j] = 0;
+            }
+            test();
+
+            if (iArr[0] != -15) {
+                throw new RuntimeException("iArr[0] must be -15 but was " + iArr[0]);
+            }
+            for (int j = 1; j < N; j++) {
+                if (iArr[j] != (j-16)) {
+                    throw new RuntimeException("iArr[" + j + "] must be " + (j-16) + " but was " + iArr[j]);
+                }
+            }
+        }
+    }
+}