8277529: SIGSEGV in C2 CompilerThread Node::rematerialize() compiling Packet::readUnsignedTrint

Reviewed-by: thartmann, roland, kvn

src/hotspot/share/opto/loopPredicate.cpp

@@ -555,6 +555,7 @@ class Invariance : public StackObj {
   Node_List _old_new; // map of old to new (clone)
   IdealLoopTree* _lpt;
   PhaseIdealLoop* _phase;
+  Node* _data_dependency_on; // The projection into the loop on which data nodes are dependent or NULL otherwise
 
   // Helper function to set up the invariance for invariance computation
   // If n is a known invariant, set up directly. Otherwise, look up the
@@ -656,7 +657,8 @@ class Invariance : public StackObj {
     _visited(area), _invariant(area),
     _stack(area, 10 /* guess */),
     _clone_visited(area), _old_new(area),
-    _lpt(lpt), _phase(lpt->_phase)
+    _lpt(lpt), _phase(lpt->_phase),
+    _data_dependency_on(NULL)
   {
     LoopNode* head = _lpt->_head->as_Loop();
     Node* entry = head->skip_strip_mined()->in(LoopNode::EntryControl);
@@ -664,7 +666,12 @@ class Invariance : public StackObj {
       // If a node is pinned between the predicates and the loop
       // entry, we won't be able to move any node in the loop that
       // depends on it above it in a predicate. Mark all those nodes
-      // as non loop invariatnt.
+      // as non-loop-invariant.
+      // Loop predication could create new nodes for which the below
+      // invariant information is missing. Mark the 'entry' node to
+      // later check again if a node needs to be treated as non-loop-
+      // invariant as well.
+      _data_dependency_on = entry;
       Unique_Node_List wq;
       wq.push(entry);
       for (uint next = 0; next < wq.size(); ++next) {
@@ -683,6 +690,12 @@ class Invariance : public StackObj {
     }
   }
 
+  // Did we explicitly mark some nodes non-loop-invariant? If so, return the entry node on which some data nodes
+  // are dependent that prevent loop predication. Otherwise, return NULL.
+  Node* data_dependency_on() {
+    return _data_dependency_on;
+  }
+
   // Map old to n for invariance computation and clone
   void map_ctrl(Node* old, Node* n) {
     assert(old->is_CFG() && n->is_CFG(), "must be");
@@ -757,31 +770,44 @@ bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invari
   Node* offset = NULL;
   jlong scale = 0;
   Node* iv = _head->as_BaseCountedLoop()->phi();
-  if (is_range_check_if(iff, phase, T_INT, iv, range, offset, scale)) {
+  Compile* C = Compile::current();
-    if (!invar.is_invariant(range)) {
+  const uint old_unique_idx = C->unique();
+  if (!is_range_check_if(iff, phase, T_INT, iv, range, offset, scale)) {
+    return false;
+  }
+  if (!invar.is_invariant(range)) {
+    return false;
+  }
+  if (offset != NULL) {
+    if (!invar.is_invariant(offset)) { // offset must be invariant
       return false;
     }
-    if (offset && !invar.is_invariant(offset)) { // offset must be invariant
+    Node* data_dependency_on = invar.data_dependency_on();
-      return false;
+    if (data_dependency_on != NULL && old_unique_idx < C->unique()) {
-    }
+      // 'offset' node was newly created in is_range_check_if(). Check that it does not depend on the entry projection
-#ifdef ASSERT
+      // into the loop. If it does, we cannot perform loop predication (see Invariant::Invariant()).
-    if (offset && phase->has_ctrl(offset)) {
+      assert(!offset->is_CFG(), "offset must be a data node");
-      Node* offset_ctrl = phase->get_ctrl(offset);
+      if (_phase->get_ctrl(offset) == data_dependency_on) {
-      if (phase->get_loop(predicate_proj) == phase->get_loop(offset_ctrl) &&
+        return false;
-          phase->is_dominator(predicate_proj, offset_ctrl)) {
-        // If the control of offset is loop predication promoted by previous pass,
-        // then it will lead to cyclic dependency.
-        // Previously promoted loop predication is in the same loop of predication
-        // point.
-        // This situation can occur when pinning nodes too conservatively - can we do better?
-        assert(false, "cyclic dependency prevents range check elimination, idx: offset %d, offset_ctrl %d, predicate_proj %d",
-               offset->_idx, offset_ctrl->_idx, predicate_proj->_idx);
       }
     }
-#endif
-    return true;
   }
-  return false;
+#ifdef ASSERT
+  if (offset && phase->has_ctrl(offset)) {
+    Node* offset_ctrl = phase->get_ctrl(offset);
+    if (phase->get_loop(predicate_proj) == phase->get_loop(offset_ctrl) &&
+        phase->is_dominator(predicate_proj, offset_ctrl)) {
+      // If the control of offset is loop predication promoted by previous pass,
+      // then it will lead to cyclic dependency.
+      // Previously promoted loop predication is in the same loop of predication
+      // point.
+      // This situation can occur when pinning nodes too conservatively - can we do better?
+      assert(false, "cyclic dependency prevents range check elimination, idx: offset %d, offset_ctrl %d, predicate_proj %d",
+             offset->_idx, offset_ctrl->_idx, predicate_proj->_idx);
+    }
+  }
+#endif
+  return true;
 }
 
 //------------------------------rc_predicate-----------------------------------

src/hotspot/share/opto/memnode.cpp

@@ -1627,14 +1627,7 @@ Node *LoadNode::split_through_phi(PhaseGVN *phase) {
       the_clone = x;            // Remember for possible deletion.
       // Alter data node to use pre-phi inputs
       if (this->in(0) == region) {
-        if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL &&
+        x->set_req(0, in);
-            MemNode::all_controls_dominate(address, region)) {
-          // Enable other optimizations such as loop predication which does not work
-          // if we directly pin the node to node `in`
-          x->set_req(0, mem->in(i)->in(0)); // Use same control as memory
-        } else {
-          x->set_req(0, in);
-        }
       } else {
         x->set_req(0, NULL);
       }

test/hotspot/jtreg/compiler/loopopts/TestDepBetweenLoopAndPredicate.java

@@ -0,0 +1,134 @@
+/*
+ * Copyright (c) 2021, 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 8277529
+ * @summary RangeCheck should not be moved out of a loop if a node on the data input chain for the bool is dependent
+ *          on the projection into the loop (after the predicates).
+ * @run main/othervm -Xbatch -XX:CompileCommand=compileonly,compiler.loopopts.TestDepBetweenLoopAndPredicate::test*
+ *                   compiler.loopopts.TestDepBetweenLoopAndPredicate
+ */
+
+package compiler.loopopts;
+
+public class TestDepBetweenLoopAndPredicate {
+    static int x, y, z;
+    static boolean flag;
+    static int[] iArrFld = new int[25];
+    static int[] iArrFld2 = new int[5];
+    static int limit = 5;
+
+    public static void main(String[] args) {
+        for (int i = 0; i < 10000; i++) {
+            flag = !flag;
+            test();
+        }
+
+        for (int i = 0; i < 5000; i++) {
+            flag = !flag;
+            test2();
+            test3();
+            test4();
+            test5();
+        }
+    }
+
+    public static void test()  {
+        int[] iArr = new int[20];
+        System.arraycopy(iArrFld, x, iArr, y, 18);
+
+        if (flag) {
+            return;
+        }
+
+        for (int i = 0; i < limit; i++) {
+            iArr[19]++;
+        }
+    }
+
+    public static void test2() {
+        for (int i = 0; i < limit; i++) {
+            int[] iArr = new int[20];
+            System.arraycopy(iArrFld, x, iArr, y, 18);
+
+            if (flag) {
+                return;
+            }
+
+            for (int j = i; j < limit; j++) {
+                x = iArrFld[iArr[19]]; // No new offset node created
+                iArr[19]++;
+            }
+        }
+    }
+
+    public static void test3() {
+        for (int i = 0; i < limit; i++) {
+            int[] iArr = new int[20];
+            System.arraycopy(iArrFld, x, iArr, y, 18);
+
+            if (flag) {
+                return;
+            }
+
+            for (int j = i + 1; j < limit; j++) {
+                x = iArrFld[iArr[19]]; // New offset node created
+                iArr[19]++;
+            }
+        }
+    }
+
+    public static void test4() {
+        for (int i = 0; i < limit; i++) {
+            int[] iArr = new int[20];
+            System.arraycopy(iArrFld, x, iArr, y, 18);
+
+            if (flag) {
+                return;
+            }
+
+            for (int j = i + 1 + z; j < limit; j++) {
+                x = iArrFld[iArr[19]]; // New offset node created
+                iArr[19]++;
+            }
+        }
+    }
+
+    public static void test5() {
+        for (int i = 0; i < limit; i++) {
+            int[] iArr = new int[20];
+            System.arraycopy(iArrFld, x, iArr, y, 18);
+
+            if (flag) {
+                return;
+            }
+
+            for (int j = i + 1 + z; j < limit; j++) {
+                x = iArrFld[iArr[19]]; // New offset node created
+                iArr[19]++;
+                y += iArrFld2[3]; // Range check removed because not dependent on projection into the loop
+            }
+        }
+    }
+}