8249607: C2: assert(!had_error) failed: bad dominance

Fix prevented igvn optimization in SplitIf for LoadNodes which resulted in dominanance errors with loop strip mining.

Reviewed-by: roland, kvn

src/hotspot/share/opto/loopnode.cpp

@@ -4674,16 +4674,81 @@ void PhaseIdealLoop::dump_bad_graph(const char* msg, Node* n, Node* early, Node*
     }
   }
   tty->cr();
-  int ct = 0;
-  Node *dbg_legal = LCA;
-  while(!dbg_legal->is_Start() && ct < 100) {
-    tty->print("idom[%d] ",ct); dbg_legal->dump();
-    ct++;
-    dbg_legal = idom(dbg_legal);
-  }
+  tty->print_cr("idoms of early %d:", early->_idx);
+  dump_idom(early);
+  tty->cr();
+  tty->print_cr("idoms of (wrong) LCA %d:", LCA->_idx);
+  dump_idom(LCA);
+  tty->cr();
+  dump_real_LCA(early, LCA);
   tty->cr();
 }
-#endif
+
+// Find the real LCA of early and the wrongly assumed LCA.
+void PhaseIdealLoop::dump_real_LCA(Node* early, Node* wrong_lca) {
+  assert(!is_dominator(early, wrong_lca) && !is_dominator(early, wrong_lca),
+         "sanity check that one node does not dominate the other");
+  assert(!has_ctrl(early) && !has_ctrl(wrong_lca), "sanity check, no data nodes");
+
+  ResourceMark rm;
+  Node_List nodes_seen;
+  Node* real_LCA = NULL;
+  Node* n1 = wrong_lca;
+  Node* n2 = early;
+  uint count_1 = 0;
+  uint count_2 = 0;
+  // Add early and wrong_lca to simplify calculation of idom indices
+  nodes_seen.push(n1);
+  nodes_seen.push(n2);
+
+  // Walk the idom chain up from early and wrong_lca and stop when they intersect.
+  while (!n1->is_Start() && !n2->is_Start()) {
+    n1 = idom(n1);
+    n2 = idom(n2);
+    if (n1 == n2) {
+      // Both idom chains intersect at the same index
+      real_LCA = n1;
+      count_1 = nodes_seen.size() / 2;
+      count_2 = count_1;
+      break;
+    }
+    if (check_idom_chains_intersection(n1, count_1, count_2, &nodes_seen)) {
+      real_LCA = n1;
+      break;
+    }
+    if (check_idom_chains_intersection(n2, count_2, count_1, &nodes_seen)) {
+      real_LCA = n2;
+      break;
+    }
+    nodes_seen.push(n1);
+    nodes_seen.push(n2);
+  }
+
+  assert(real_LCA != NULL, "must always find an LCA");
+  tty->print_cr("Real LCA of early %d (idom[%d]) and (wrong) LCA %d (idom[%d]):", early->_idx, count_2, wrong_lca->_idx, count_1);
+  real_LCA->dump();
+}
+
+// Check if n is already on nodes_seen (i.e. idom chains of early and wrong_lca intersect at n). Determine the idom index of n
+// on both idom chains and return them in idom_idx_new and idom_idx_other, respectively.
+bool PhaseIdealLoop::check_idom_chains_intersection(const Node* n, uint& idom_idx_new, uint& idom_idx_other, const Node_List* nodes_seen) const {
+  if (nodes_seen->contains(n)) {
+    // The idom chain has just discovered n.
+    // Divide by 2 because nodes_seen contains the same amount of nodes from both chains.
+    idom_idx_new = nodes_seen->size() / 2;
+
+    // The other chain already contained n. Search the index.
+    for (uint i = 0; i < nodes_seen->size(); i++) {
+      if (nodes_seen->at(i) == n) {
+        // Divide by 2 because nodes_seen contains the same amount of nodes from both chains.
+        idom_idx_other = i / 2;
+      }
+    }
+    return true;
+  }
+  return false;
+}
+#endif // ASSERT
 
 #ifndef PRODUCT
 //------------------------------dump-------------------------------------------
@@ -4753,7 +4818,19 @@ void PhaseIdealLoop::dump(IdealLoopTree* loop, uint idx, Node_List &rpo_list) co
     }
   }
 }
-#endif
+
+void PhaseIdealLoop::dump_idom(Node* n) const {
+  if (has_ctrl(n)) {
+    tty->print_cr("No idom for data nodes");
+  } else {
+    for (int i = 0; i < 100 && !n->is_Start(); i++) {
+      tty->print("idom[%d] ", i);
+      n->dump();
+      n = idom(n);
+    }
+  }
+}
+#endif // NOT PRODUCT
 
 // Collect a R-P-O for the whole CFG.
 // Result list is in post-order (scan backwards for RPO)

src/hotspot/share/opto/loopnode.hpp

@@ -1440,6 +1440,10 @@ class PhaseIdealLoop : public PhaseTransform {
                                       uint idx_before_clone, Node_List &old_new);
 
   bool _created_loop_node;
+#ifdef ASSERT
+  void dump_real_LCA(Node* early, Node* wrong_lca);
+  bool check_idom_chains_intersection(const Node* n, uint& idom_idx_new, uint& idom_idx_other, const Node_List* nodes_seen) const;
+#endif
 
 public:
   void set_created_loop_node() { _created_loop_node = true; }
@@ -1452,6 +1456,7 @@ class PhaseIdealLoop : public PhaseTransform {
 
 #ifndef PRODUCT
   void dump() const;
+  void dump_idom(Node* n) const;
   void dump(IdealLoopTree* loop, uint rpo_idx, Node_List &rpo_list) const;
   void verify() const;          // Major slow  :-)
   void verify_compare(Node* n, const PhaseIdealLoop* loop_verify, VectorSet &visited) const;

src/hotspot/share/opto/loopopts.cpp

@@ -1495,7 +1495,10 @@ void PhaseIdealLoop::split_if_with_blocks_post(Node *n) {
             // to fold a StoreP and an AddP together (as part of an
             // address expression) and the AddP and StoreP have
             // different controls.
-            if (!x->is_Load() && !x->is_DecodeNarrowPtr()) _igvn._worklist.yank(x);
+            BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
+            if (!x->is_Load() && !x->is_DecodeNarrowPtr() && !x->is_AddP() && !bs->is_gc_barrier_node(x)) {
+              _igvn._worklist.yank(x);
+            }
           }
           _igvn.remove_dead_node(n);
         }

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

@@ -0,0 +1,106 @@
+/*
+ * 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
+ * @bug 8249607
+ * @summary A LoadNode is pinned in split_if_with_blocks_post() on a loop exit node x that is part of a strip mined loop. It has a late control y outside
+            the outer strip mined loop. After pre-main-post, the dominator chain of y does not include x anymore resulting in an assertion failure.
+ * @run main/othervm -Xbatch -XX:CompileCommand=compileonly,compiler.loopopts.TestSplitIfPinnedLoadInStripMinedLoop::*
+ *                   compiler.loopopts.TestSplitIfPinnedLoadInStripMinedLoop
+ */
+package compiler.loopopts;
+
+public class TestSplitIfPinnedLoadInStripMinedLoop {
+
+    public boolean bFld = false;
+    public short sFld = 4;
+    public static int iFld = 5;
+    public static float fFld= 6.0f;
+    public static int iArrFld[] = new int[400];
+
+    public void test() {
+        int x = 7;
+        int y = 8;
+        int a = 9;
+        float f = 10.0f;
+        double d = 11.0f;
+        double dArr[] = new double[400];
+
+        for (int i = 16; i < 350; i++) {
+            for (int j = 1; j < 75; j++) {
+                for (int k = 1; k < 3; k++) {
+                }
+                f = j * 6;
+                y = j;
+                try {
+                    x = (y / 148);
+                } catch (ArithmeticException a_e) {}
+                if (bFld) {
+                    break;
+                }
+                dArr[1] = 4;
+            }
+            for (int k = 75; k > i; k--) {
+                iArrFld[k] = 5;
+            }
+            for (int k = 4; k < 75; k++) {
+                f -= fFld;
+                // The LoadSNode for sFld is cloned in split_if_with_blocks_post() for each use such that they can float out of the loop. All control
+                // inputs of the clone are set to the latest control of the original LoadSNode which in this case is the StoreSNode for iFld that is
+                // aninput to a MergeMemNode which is an input to the SafePointNode in the outer strip mined loop. Both these nodes are not part
+                // of the loop body and thus the StoreNode is also not part of the loop anymore. This means that all the new LoadNode clones get
+                // the loop exit l inside the outer strip mined loop as control input. Some of these clones (**) have a late control outside of
+                // this outer strip mined loop. The dominator chain from the controls nodes of (**) contain l. However, after pre-main-post, we
+                // insert additional Region nodes but do not account for these control inputs of the LoadSNodes. They remain unchanged and still
+                // have l as control input. As a consequence, we do not find l on the dominator chains from the control nodes of (**) anymore
+                // resulting in a dominator assertion failure.
+                iFld = sFld;
+            }
+            switch ((i % 8) + 27) {
+            case 27:
+                if (bFld) {
+                    for (a = 1; a < 75; a++) {
+                        iFld += 6; // (**)
+                    }
+                } else {
+                    d -= x;
+                }
+                break;
+            case 28:
+                iFld = y;
+                // Fall through
+            case 33:
+            case 34:
+                iFld -= (int)d; // (**)
+                break;
+            }
+        }
+    }
+    public static void main(String[] strArr) {
+        TestSplitIfPinnedLoadInStripMinedLoop t = new TestSplitIfPinnedLoadInStripMinedLoop();
+        for (int i = 0; i < 10; i++) {
+            t.test();
+        }
+    }
+}