From a3fa18a7a7314b7dcbe1035acdcad84d71dc417f Mon Sep 17 00:00:00 2001 From: Tom Rodriguez Date: Fri, 9 May 2025 11:32:11 -0700 Subject: [PATCH] Permit GVN of class initialization checks --- .../nodes/KlassBeingInitializedCheckNode.java | 3 +- .../nodes/FixedGlobalValueNumberable.java | 35 ++++++ ...minatorBasedGlobalValueNumberingPhase.java | 101 ++++++++++-------- 3 files changed, 94 insertions(+), 45 deletions(-) create mode 100644 compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/FixedGlobalValueNumberable.java diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassBeingInitializedCheckNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassBeingInitializedCheckNode.java index 74ad79ae0ec8..18cbd6e0fe94 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassBeingInitializedCheckNode.java +++ b/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 TYPE = NodeClass.create(KlassBeingInitializedCheckNode.class); @Input protected ValueNode klass; diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/FixedGlobalValueNumberable.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/FixedGlobalValueNumberable.java new file mode 100644 index 000000000000..4682857c52a5 --- /dev/null +++ b/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 { +} diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/DominatorBasedGlobalValueNumberingPhase.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/DominatorBasedGlobalValueNumberingPhase.java index d6f1e4838d53..732e808b2449 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/DominatorBasedGlobalValueNumberingPhase.java +++ b/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; } /**