8269820: C2 PhaseIdealLoop::do_unroll get wrong opaque node

Reviewed-by: kvn, thartmann, chagedorn

src/hotspot/share/opto/cfgnode.cpp

@@ -619,7 +619,7 @@ Node *RegionNode::Ideal(PhaseGVN *phase, bool can_reshape) {
         if (opaq != NULL) {
           // This is not a loop anymore. No need to keep the Opaque1 node on the test that guards the loop as it won't be
           // subject to further loop opts.
-          assert(opaq->Opcode() == Op_Opaque1, "");
+          assert(opaq->Opcode() == Op_OpaqueZeroTripGuard, "");
           igvn->replace_node(opaq, opaq->in(1));
         }
       }

src/hotspot/share/opto/classes.hpp

@@ -268,6 +268,7 @@ macro(OnSpinWait)
 macro(Opaque1)
 macro(OpaqueLoopInit)
 macro(OpaqueLoopStride)
+macro(OpaqueZeroTripGuard)
 macro(Opaque3)
 macro(Opaque4)
 macro(ProfileBoolean)

src/hotspot/share/opto/loopTransform.cpp

@@ -1300,7 +1300,7 @@ void PhaseIdealLoop::ensure_zero_trip_guard_proj(Node* node, bool is_main_loop)
   assert(zer_cmp != NULL && zer_cmp->Opcode() == Op_CmpI, "must be CmpI");
   // For the main loop, the opaque node is the second input to zer_cmp, for the post loop it's the first input node
   Node* zer_opaq = zer_cmp->in(is_main_loop ? 2 : 1);
-  assert(zer_opaq != NULL && zer_opaq->Opcode() == Op_Opaque1, "must be Opaque1");
+  assert(zer_opaq != NULL && zer_opaq->Opcode() == Op_OpaqueZeroTripGuard, "must be OpaqueZeroTripGuard");
 }
 #endif
 
@@ -1705,7 +1705,7 @@ void PhaseIdealLoop::insert_pre_post_loops(IdealLoopTree *loop, Node_List &old_n
   // pre-loop, the main-loop may not execute at all.  Later in life this
   // zero-trip guard will become the minimum-trip guard when we unroll
   // the main-loop.
-  Node *min_opaq = new Opaque1Node(C, limit);
+  Node *min_opaq = new OpaqueZeroTripGuardNode(C, limit);
   Node *min_cmp  = new CmpINode(pre_incr, min_opaq);
   Node *min_bol  = new BoolNode(min_cmp, b_test);
   register_new_node(min_opaq, new_pre_exit);
@@ -1994,7 +1994,7 @@ Node *PhaseIdealLoop::insert_post_loop(IdealLoopTree* loop, Node_List& old_new,
   // (the previous loop trip-counter exit value) because we will be changing
   // the exit value (via additional unrolling) so we cannot constant-fold away the zero
   // trip guard until all unrolling is done.
-  Node *zer_opaq = new Opaque1Node(C, incr);
+  Node *zer_opaq = new OpaqueZeroTripGuardNode(C, incr);
   Node *zer_cmp = new CmpINode(zer_opaq, limit);
   Node *zer_bol = new BoolNode(zer_cmp, main_end->test_trip());
   register_new_node(zer_opaq, new_main_exit);

src/hotspot/share/opto/loopnode.cpp

@@ -5480,12 +5480,12 @@ Node* CountedLoopNode::is_canonical_loop_entry() {
   if (input >= cmpzm->req() || cmpzm->in(input) == NULL) {
     return NULL;
   }
-  bool res = cmpzm->in(input)->Opcode() == Op_Opaque1;
+  bool res = cmpzm->in(input)->Opcode() == Op_OpaqueZeroTripGuard;
 #ifdef ASSERT
   bool found_opaque = false;
   for (uint i = 1; i < cmpzm->req(); i++) {
     Node* opnd = cmpzm->in(i);
-    if (opnd && opnd->Opcode() == Op_Opaque1) {
+    if (opnd && opnd->is_Opaque1()) {
       found_opaque = true;
       break;
     }

src/hotspot/share/opto/opaquenode.hpp

@@ -70,6 +70,13 @@ class OpaqueLoopStrideNode : public Opaque1Node {
   virtual int Opcode() const;
 };
 
+class OpaqueZeroTripGuardNode : public Opaque1Node {
+public:
+  OpaqueZeroTripGuardNode(Compile* C, Node *n) : Opaque1Node(C, n) {
+  }
+  virtual int Opcode() const;
+};
+
 //------------------------------Opaque3Node------------------------------------
 // A node to prevent unwanted optimizations. Will be optimized only during
 // macro nodes expansion.

src/hotspot/share/opto/split_if.cpp

@@ -27,6 +27,7 @@
 #include "opto/callnode.hpp"
 #include "opto/loopnode.hpp"
 #include "opto/movenode.hpp"
+#include "opto/opaquenode.hpp"
 
 
 //------------------------------split_thru_region------------------------------
@@ -220,6 +221,24 @@ bool PhaseIdealLoop::split_up( Node *n, Node *blk1, Node *blk2 ) {
     }
   }
 
+  if (n->Opcode() == Op_OpaqueZeroTripGuard) {
+    // If this Opaque1 is part of the zero trip guard for a loop:
+    // 1- it can't be shared
+    // 2- the zero trip guard can't be the if that's being split
+    // As a consequence, this node could be assigned control anywhere between its current control and the zero trip guard.
+    // Move it down to get it out of the way of split if and avoid breaking the zero trip guard shape.
+    Node* cmp = n->unique_out();
+    assert(cmp->Opcode() == Op_CmpI, "bad zero trip guard shape");
+    Node* bol = cmp->unique_out();
+    assert(bol->Opcode() == Op_Bool, "bad zero trip guard shape");
+    Node* iff = bol->unique_out();
+    assert(iff->Opcode() == Op_If, "bad zero trip guard shape");
+    set_ctrl(n, iff->in(0));
+    set_ctrl(cmp, iff->in(0));
+    set_ctrl(bol, iff->in(0));
+    return true;
+  }
+
   // See if splitting-up a Store.  Any anti-dep loads must go up as
   // well.  An anti-dep load might be in the wrong block, because in
   // this particular layout/schedule we ignored anti-deps and allow

src/hotspot/share/opto/subnode.cpp

@@ -1444,7 +1444,7 @@ Node *BoolNode::Ideal(PhaseGVN *phase, bool can_reshape) {
   // Move constants to the right of compare's to canonicalize.
   // Do not muck with Opaque1 nodes, as this indicates a loop
   // guard that cannot change shape.
-  if( con->is_Con() && !cmp2->is_Con() && cmp2_op != Op_Opaque1 &&
+  if (con->is_Con() && !cmp2->is_Con() && cmp2_op != Op_OpaqueZeroTripGuard &&
       // Because of NaN's, CmpD and CmpF are not commutative
       cop != Op_CmpD && cop != Op_CmpF &&
       // Protect against swapping inputs to a compare when it is used by a

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

@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2022, Red Hat, Inc. 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 8269820
+ * @summary C2 PhaseIdealLoop::do_unroll get wrong opaque node
+ *
+ * @run main/othervm -Xbatch -XX:-TieredCompilation TestCanonicalLoopEntryOpaqueOrder
+ *
+ */
+
+public class TestCanonicalLoopEntryOpaqueOrder {
+    static void test() {
+        int ina8[] = new int[1478];
+        int in2 = 136;
+        long lo3 = 0L;
+        try {
+            for (int i = 0; i < 34; i++) {
+                Math.log1p(1);
+            }
+        } catch (Exception e) {
+            in2 = 1;
+        }
+
+        for (int i = 0; i < in2; i++) {
+            if (in2 > 10) {  // split if and create wrong opaque order
+                for (int j = 0; j < in2; j++) {
+                    lo3 -= 1L;
+                }
+            }
+        }
+    }
+    public static void main(String[] args) {
+        for (int i = 0; i < 10000; i++) {
+            test();
+        }
+    }
+}