8280123: C2: Infinite loop in CMoveINode::Ideal during IGVN

Reviewed-by: kvn, thartmann, chagedorn

src/hotspot/share/opto/movenode.cpp

@@ -75,21 +75,25 @@
 // Return a node which is more "ideal" than the current node.
 // Move constants to the right.
 Node *CMoveNode::Ideal(PhaseGVN *phase, bool can_reshape) {
-  if( in(0) && remove_dead_region(phase, can_reshape) ) return this;
+  if (in(0) != NULL && remove_dead_region(phase, can_reshape)) {
+    return this;
+  }
   // Don't bother trying to transform a dead node
-  if( in(0) && in(0)->is_top() )  return NULL;
+  if (in(0) != NULL && in(0)->is_top()) {
+    return NULL;
+  }
   assert(in(Condition) != this &&
-         in(IfFalse) != this &&
-         in(IfTrue) != this, "dead loop in CMoveNode::Ideal" );
-  if( phase->type(in(Condition)) == Type::TOP )
-  return NULL; // return NULL when Condition is dead
-
-  if( in(IfFalse)->is_Con() && !in(IfTrue)->is_Con() ) {
-    if( in(Condition)->is_Bool() ) {
-      BoolNode* b  = in(Condition)->as_Bool();
-      BoolNode* b2 = b->negate(phase);
-      return make(in(Control), phase->transform(b2), in(IfTrue), in(IfFalse), _type);
-    }
+         in(IfFalse)   != this &&
+         in(IfTrue)    != this, "dead loop in CMoveNode::Ideal");
+  if (phase->type(in(Condition)) == Type::TOP ||
+      phase->type(in(IfFalse))   == Type::TOP ||
+      phase->type(in(IfTrue))    == Type::TOP) {
+    return NULL;
+  }
+  // Canonicalize the node by moving constants to the right input.
+  if (in(Condition)->is_Bool() && phase->type(in(IfFalse))->singleton() && !phase->type(in(IfTrue))->singleton()) {
+    BoolNode* b = in(Condition)->as_Bool()->negate(phase);
+    return make(in(Control), phase->transform(b), in(IfTrue), in(IfFalse), _type);
   }
   return NULL;
 }
@@ -191,14 +195,10 @@ Node *CMoveINode::Ideal(PhaseGVN *phase, bool can_reshape) {
 
   // If zero is on the left (false-case, no-move-case) it must mean another
   // constant is on the right (otherwise the shared CMove::Ideal code would
-  // have moved the constant to the right).  This situation is bad for Intel
-  // and a don't-care for Sparc.  It's bad for Intel because the zero has to
-  // be manifested in a register with a XOR which kills flags, which are live
-  // on input to the CMoveI, leading to a situation which causes excessive
-  // spilling on Intel.  For Sparc, if the zero in on the left the Sparc will
-  // zero a register via G0 and conditionally-move the other constant.  If the
-  // zero is on the right, the Sparc will load the first constant with a
-  // 13-bit set-lo and conditionally move G0.  See bug 4677505.
+  // have moved the constant to the right). This situation is bad for x86 because
+  // the zero has to be manifested in a register with a XOR which kills flags,
+  // which are live on input to the CMoveI, leading to a situation which causes
+  // excessive spilling. See bug 4677505.
   if( phase->type(in(IfFalse)) == TypeInt::ZERO && !(phase->type(in(IfTrue)) == TypeInt::ZERO) ) {
     if( in(Condition)->is_Bool() ) {
       BoolNode* b  = in(Condition)->as_Bool();

test/hotspot/jtreg/compiler/c2/TestCMoveInfiniteGVN.java

@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2022, 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
+ * @key stress randomness
+ * @bug 8280123
+ * @run main/othervm -Xcomp -XX:-TieredCompilation
+ *                   -XX:CompileCommand=compileonly,compiler.c2.TestCMoveInfiniteGVN::test
+ *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressIGVN -XX:StressSeed=43739875
+ *                   compiler.c2.TestCMoveInfiniteGVN
+ * @run main/othervm -Xcomp -XX:-TieredCompilation
+ *                   -XX:CompileCommand=compileonly,compiler.c2.TestCMoveInfiniteGVN::test
+ *                   -XX:+UnlockDiagnosticVMOptions -XX:+StressIGVN
+ *                   compiler.c2.TestCMoveInfiniteGVN
+ */
+
+package compiler.c2;
+
+public class TestCMoveInfiniteGVN {
+
+    static int test(boolean b, int i) {
+        int iArr[] = new int[2];
+
+        double d = Math.max(i, i);
+        for (int i1 = 1; i1 < 2; i1++) {
+            if (i1 != 0) {
+                return (b ? 1 : 0); // CMoveI
+            }
+            for (int i2 = 1; i2 < 2; i2++) {
+                switch (i2) {
+                    case 1: d -= Math.max(i1, i2); break;
+                }
+                d -= iArr[i1 - 1];
+            }
+        }
+        return 0;
+    }
+
+    static void test() {
+        test(true, 234);
+    }
+
+    public static void main(String[] strArr) {
+        test(); // compilation, then nmethod invalidation during execution
+        test(); // trigger crashing recompilation
+    }
+}