8351833: Unexpected increase in live nodes when splitting Phis through MergeMems in PhiNode::Ideal

Reviewed-by: chagedorn, rcastanedalo, kvn

src/hotspot/share/opto/cfgnode.cpp

@@ -2530,11 +2530,9 @@ Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
         //     MergeMem(Phi(...m0...), Phi:AT1(...m1...), Phi:AT2(...m2...))
         PhaseIterGVN* igvn = phase->is_IterGVN();
         assert(igvn != nullptr, "sanity check");
-        Node* hook = new Node(1);
         PhiNode* new_base = (PhiNode*) clone();
         // Must eagerly register phis, since they participate in loops.
         igvn->register_new_node_with_optimizer(new_base);
-        hook->add_req(new_base);
 
         MergeMemNode* result = MergeMemNode::make(new_base);
         for (uint i = 1; i < req(); ++i) {
@@ -2548,7 +2546,6 @@ Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
                 Node* new_phi = new_base->slice_memory(mms.adr_type(phase->C));
                 made_new_phi = true;
                 igvn->register_new_node_with_optimizer(new_phi);
-                hook->add_req(new_phi);
                 mms.set_memory(new_phi);
               }
               Node* phi = mms.memory();
@@ -2566,19 +2563,12 @@ Node *PhiNode::Ideal(PhaseGVN *phase, bool can_reshape) {
             }
           }
         }
-        // Already replace this phi node to cut it off from the graph to not interfere in dead loop checks during the
-        // transformations of the new phi nodes below. Otherwise, we could wrongly conclude that there is no dead loop
-        // because we are finding this phi node again. Also set the type of the new MergeMem node in case we are also
-        // visiting it in the transformations below.
-        igvn->replace_node(this, result);
-        igvn->set_type(result, result->bottom_type());
 
-        // now transform the new nodes, and return the mergemem
-        for (MergeMemStream mms(result); mms.next_non_empty(); ) {
-          Node* phi = mms.memory();
-          mms.set_memory(phase->transform(phi));
-        }
-        hook->destruct(igvn);
+        // We could immediately transform the new Phi nodes here, but that can
+        // result in creating an excessive number of new nodes within a single
+        // IGVN iteration. We have put the Phi nodes on the IGVN worklist, so
+        // they are transformed later on in any case.
+
         // Replace self with the result.
         return result;
       }

src/hotspot/share/opto/phaseX.cpp

@@ -1024,11 +1024,18 @@ void PhaseIterGVN::optimize() {
     shuffle_worklist();
   }
 
+  // The node count check in the loop below (check_node_count) assumes that we
+  // increase the live node count with at most
+  // max_live_nodes_increase_per_iteration in between checks. If this
+  // assumption does not hold, there is a risk that we exceed the max node
+  // limit in between checks and trigger an assert during node creation.
+  const int max_live_nodes_increase_per_iteration = NodeLimitFudgeFactor * 2;
+
   uint loop_count = 0;
   // Pull from worklist and transform the node. If the node has changed,
   // update edge info and put uses on worklist.
-  while(_worklist.size()) {
-    if (C->check_node_count(NodeLimitFudgeFactor * 2, "Out of nodes")) {
+  while (_worklist.size() > 0) {
+    if (C->check_node_count(max_live_nodes_increase_per_iteration, "Out of nodes")) {
       C->print_method(PHASE_AFTER_ITER_GVN, 3);
       return;
     }
@@ -1043,7 +1050,16 @@ void PhaseIterGVN::optimize() {
     if (n->outcnt() != 0) {
       NOT_PRODUCT(const Type* oldtype = type_or_null(n));
       // Do the transformation
+      DEBUG_ONLY(int live_nodes_before = C->live_nodes();)
       Node* nn = transform_old(n);
+      DEBUG_ONLY(int live_nodes_after = C->live_nodes();)
+      // Ensure we did not increase the live node count with more than
+      // max_live_nodes_increase_per_iteration during the call to transform_old
+      DEBUG_ONLY(int increase = live_nodes_after - live_nodes_before;)
+      assert(increase < max_live_nodes_increase_per_iteration,
+             "excessive live node increase in single iteration of IGVN: %d "
+             "(should be at most %d)",
+             increase, max_live_nodes_increase_per_iteration);
       NOT_PRODUCT(trace_PhaseIterGVN(n, nn, oldtype);)
     } else if (!n->is_top()) {
       remove_dead_node(n);

test/hotspot/jtreg/TEST.groups

@@ -185,6 +185,7 @@ tier1_compiler_2 = \
   compiler/integerArithmetic/ \
   compiler/interpreter/ \
   compiler/jvmci/ \
+  compiler/igvn/ \
   -compiler/classUnloading/methodUnloading/TestOverloadCompileQueues.java \
   -compiler/codecache/stress \
   -compiler/codegen/aes \

test/hotspot/jtreg/compiler/igvn/TestSplitPhiThroughMergeMem.java

@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2025, 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 8351833
+ * @summary Splitting Phi memory nodes through MergeMem nodes in PhiNode::Ideal
+ *          could sometimes result in a too large number of added nodes within
+ *          a single iteration of the main loop in PhaseIterGVN::optimize. This
+ *          test, reduced from TestScalarReplacementMaxLiveNodes, triggers an
+ *          assert added as part of the fix for the linked bug/issue (the
+ *          assert naturally triggers only before the fix). The test's ability
+ *          to trigger the issue is quite sensitive to the specific String
+ *          constants used. The current set of chosen String constants happened
+ *          to work particularly well.
+ * @run main/othervm -Xbatch
+ *                   -XX:CompileCommand=CompileOnly,compiler.igvn.TestSplitPhiThroughMergeMem::test
+ *                   compiler.igvn.TestSplitPhiThroughMergeMem
+ * @run main compiler.igvn.TestSplitPhiThroughMergeMem
+ */
+
+package compiler.igvn;
+
+public class TestSplitPhiThroughMergeMem {
+
+    public static void main(String[] args) {
+        for (int i = 0; i < 10_000; i++) {
+            int val = i % 50;
+            test(val == 0, val % 10, val % 20);
+        }
+    }
+
+    static void test(boolean flag, int param1, int param2) {
+        if (flag) {
+            new String("tenth" + param1);
+            new String("eleventh" + param2);
+            new String("fifteenth" + param2);
+            new String("sixteenth" + param1);
+            new String("seventeenth" + param1);
+            new String("nineteenth" + param2);
+            new String("tweenth" + param1);
+            new String("nineth" + param1);
+            new String("nineth" + param1);
+            new String("eighteenth" + param1);
+            new String("abcdef" + param2);
+            new String("ghijklmn" + param1);
+            new String("ghijklmn" + param1);
+        }
+    }
+}