Permit GVN of class initialization checks

Author: tkrodriguez

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassBeingInitializedCheckNode.java

@@ -34,12 +34,13 @@
 import jdk.graal.compiler.nodeinfo.InputType;
 import jdk.graal.compiler.nodeinfo.NodeInfo;
 import jdk.graal.compiler.nodes.DeoptimizingFixedWithNextNode;
+import jdk.graal.compiler.nodes.FixedGlobalValueNumberable;
 import jdk.graal.compiler.nodes.ValueNode;
 import jdk.graal.compiler.nodes.memory.SingleMemoryKill;
 import jdk.graal.compiler.nodes.spi.Lowerable;
 
 @NodeInfo(allowedUsageTypes = {InputType.Memory}, cycles = CYCLES_4, size = SIZE_16)
-public class KlassBeingInitializedCheckNode extends DeoptimizingFixedWithNextNode implements Lowerable, SingleMemoryKill {
+public class KlassBeingInitializedCheckNode extends DeoptimizingFixedWithNextNode implements Lowerable, SingleMemoryKill, FixedGlobalValueNumberable {
     public static final NodeClass<KlassBeingInitializedCheckNode> TYPE = NodeClass.create(KlassBeingInitializedCheckNode.class);
 
     @Input protected ValueNode klass;

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/FixedGlobalValueNumberable.java

@@ -0,0 +1,35 @@
+/*
+ * 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.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * 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.
+ */
+package jdk.graal.compiler.nodes;
+
+/**
+ * Marker for a {@link FixedNode} that can be replaced by another node via
+ * {@link jdk.graal.compiler.phases.common.DominatorBasedGlobalValueNumberingPhase}. This permits
+ * unusual nodes to explicitly opt into processing by dominator GVN. Note that it's the
+ * responsibility of the implementor to ensure the node and its subclasses can be safely value
+ * numbered even in the presence of memory kills.
+ */
+public interface FixedGlobalValueNumberable extends FixedNodeInterface {
+}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/DominatorBasedGlobalValueNumberingPhase.java

@@ -41,6 +41,7 @@
 import jdk.graal.compiler.graph.NodeBitMap;
 import jdk.graal.compiler.graph.spi.NodeWithIdentity;
 import jdk.graal.compiler.nodes.ControlSinkNode;
+import jdk.graal.compiler.nodes.FixedGlobalValueNumberable;
 import jdk.graal.compiler.nodes.FixedGuardNode;
 import jdk.graal.compiler.nodes.FixedNode;
 import jdk.graal.compiler.nodes.FixedWithNextNode;
@@ -313,18 +314,22 @@ private static void processNode(FixedWithNextNode cur, LocationSet thisLoopKille
                 final LoopBeginNode exitedLoop = ((LoopExitNode) cur).loopBegin();
                 killLoopLocations(((HIRLoop) cfg.blockFor(exitedLoop).getLoop()).getKillLocations(), blockMap);
             }
+            boolean mustGVN = (cur instanceof FixedGlobalValueNumberable);
+            if (!mustGVN) {
+                // Handle nodes which don't explicitly opt into DGVN
+                if (MemoryKill.isMemoryKill(cur)) {
+                    blockMap.killValuesByPotentialMemoryKill(cur);
+                    return;
+                }
 
-            if (MemoryKill.isMemoryKill(cur)) {
-                blockMap.killValuesByPotentialMemoryKill(cur);
-                return;
-            }
-
-            if (!canGVN(cur)) {
-                return;
+                if (!canGVN(cur)) {
+                    return;
+                }
             }
 
             boolean canSubsitute = blockMap.hasSubstitute(cur);
-            boolean canLICM = loopCandidate != null;
+            // assume FixedGlobalValueNumberable nodes shouldn't be moved
+            boolean canLICM = loopCandidate != null && !mustGVN;
             if (cur instanceof MemoryAccess) {
                 MemoryAccess access = (MemoryAccess) cur;
                 if (loopKillsLocation(thisLoopKilledLocations, access.getLocationIdentity())) {
@@ -347,15 +352,21 @@ private static void processNode(FixedWithNextNode cur, LocationSet thisLoopKille
              * a limited form of LICM for nodes that are dominated by the loop header and dominate
              * all exits. Such operations that are unconditionally executed in the particular loop.
              */
+            boolean substituted = false;
             if (canSubsitute) {
                 // GVN node
-                blockMap.substitute(cur, cfg, licmNodes, canLICM ? loopCandidate : null);
+                substituted = blockMap.substitute(cur, cfg, licmNodes, canLICM ? loopCandidate : null);
             } else {
                 if (canLICM) {
                     tryPerformLICM(loopCandidate, cur, licmNodes);
                 }
                 blockMap.rememberNodeForGVN(cur);
             }
+            if (mustGVN && !substituted) {
+                if (MemoryKill.isMemoryKill(cur)) {
+                    blockMap.killValuesByPotentialMemoryKill(cur);
+                }
+            }
         }
 
         @Override
@@ -589,47 +600,49 @@ public boolean hasSubstitute(Node n) {
          * Perform actual global value numbering. Replace node {@code n} with an equal node (inputs
          * and data fields) up in the dominance chain.
          */
-        public void substitute(Node n, ControlFlowGraph cfg, NodeBitMap licmNodes, Loop invariantInLoop) {
+        public boolean substitute(Node n, ControlFlowGraph cfg, NodeBitMap licmNodes, Loop invariantInLoop) {
             Node edgeDataEqual = find(n);
-            if (edgeDataEqual != null) {
-                assert edgeDataEqual.graph() != null;
-                assert edgeDataEqual instanceof FixedNode : "Only process fixed nodes";
-                StructuredGraph graph = (StructuredGraph) edgeDataEqual.graph();
-
-                HIRBlock defBlock = cfg.blockFor(edgeDataEqual);
-
-                if (invariantInLoop != null) {
-                    HIRBlock loopDefBlock = cfg.blockFor(invariantInLoop.loopBegin()).getLoop().getHeader().getDominator();
-                    if (loopDefBlock.strictlyDominates(defBlock)) {
-                        /*
-                         * The LICM location strictly dominates the GVN location so it must be the
-                         * final location. Move the GVN node to the LICM location and then perform
-                         * the substitution normally.
-                         */
-                        if (!tryPerformLICM(invariantInLoop, (FixedNode) edgeDataEqual, licmNodes)) {
-                            GraalError.shouldNotReachHere("tryPerformLICM must succeed for " + edgeDataEqual); // ExcludeFromJacocoGeneratedReport
-                        }
-                    } else {
-                        GraalError.guarantee(defBlock.dominates(loopDefBlock), "No dominance relation between GVN and LICM locations: %s and %s", defBlock, loopDefBlock);
+            if (edgeDataEqual == null) {
+                return false;
+            }
+            assert edgeDataEqual.graph() != null;
+            assert edgeDataEqual instanceof FixedNode : "Only process fixed nodes";
+            StructuredGraph graph = (StructuredGraph) edgeDataEqual.graph();
+
+            HIRBlock defBlock = cfg.blockFor(edgeDataEqual);
+
+            if (invariantInLoop != null) {
+                HIRBlock loopDefBlock = cfg.blockFor(invariantInLoop.loopBegin()).getLoop().getHeader().getDominator();
+                if (loopDefBlock.strictlyDominates(defBlock)) {
+                    /*
+                     * The LICM location strictly dominates the GVN location so it must be the final
+                     * location. Move the GVN node to the LICM location and then perform the
+                     * substitution normally.
+                     */
+                    if (!tryPerformLICM(invariantInLoop, (FixedNode) edgeDataEqual, licmNodes)) {
+                        GraalError.shouldNotReachHere("tryPerformLICM must succeed for " + edgeDataEqual); // ExcludeFromJacocoGeneratedReport
                     }
+                } else {
+                    GraalError.guarantee(defBlock.dominates(loopDefBlock), "No dominance relation between GVN and LICM locations: %s and %s", defBlock, loopDefBlock);
                 }
+            }
 
-                if (!LoopUtility.canUseWithoutProxy(cfg, edgeDataEqual, n)) {
-                    earlyGVNAbort.increment(graph.getDebug());
-                    return;
-                }
+            if (!LoopUtility.canUseWithoutProxy(cfg, edgeDataEqual, n)) {
+                earlyGVNAbort.increment(graph.getDebug());
+                return false;
+            }
 
-                graph.getDebug().log(DebugContext.VERY_DETAILED_LEVEL, "Early GVN: replacing %s with %s", n, edgeDataEqual);
-                graph.getDebug().dump(DebugContext.VERY_DETAILED_LEVEL, graph, "Before replacing %s with %s", n, edgeDataEqual);
-                n.replaceAtUsages(edgeDataEqual);
-                GraphUtil.unlinkFixedNode((FixedWithNextNode) n);
-                n.safeDelete();
-                graph.getDebug().dump(DebugContext.VERY_DETAILED_LEVEL, graph, "After replacing %s with %s", n, edgeDataEqual);
-                earlyGVN.increment(graph.getDebug());
-                if (graph.getDebug().isCountEnabled()) {
-                    DebugContext.counter(earlyGVN.getName() + "_" + edgeDataEqual.getClass().getSimpleName()).increment(graph.getDebug());
-                }
+            graph.getDebug().log(DebugContext.VERY_DETAILED_LEVEL, "Early GVN: replacing %s with %s", n, edgeDataEqual);
+            graph.getDebug().dump(DebugContext.VERY_DETAILED_LEVEL, graph, "Before replacing %s with %s", n, edgeDataEqual);
+            n.replaceAtUsages(edgeDataEqual);
+            GraphUtil.unlinkFixedNode((FixedWithNextNode) n);
+            n.safeDelete();
+            graph.getDebug().dump(DebugContext.VERY_DETAILED_LEVEL, graph, "After replacing %s with %s", n, edgeDataEqual);
+            earlyGVN.increment(graph.getDebug());
+            if (graph.getDebug().isCountEnabled()) {
+                DebugContext.counter(earlyGVN.getName() + "_" + edgeDataEqual.getClass().getSimpleName()).increment(graph.getDebug());
             }
+            return true;
         }
 
         /**