From b2c7e0670c05a82d3ad29198ab87cf9f5d066df3 Mon Sep 17 00:00:00 2001 From: Tom Rodriguez Date: Thu, 17 Oct 2024 21:36:53 -0700 Subject: [PATCH 1/3] Handle multiple ValueProxyNodes in removeIntermediateMaterialization --- .../src/jdk/graal/compiler/nodes/IfNode.java | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/IfNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/IfNode.java index b1e7a7c9a8b1..5e4130821823 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/IfNode.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/IfNode.java @@ -2256,19 +2256,20 @@ private static void propagateInjectedProfile(BranchProbabilityData profile, EndN private void connectEnds(List ends, ValuePhiNode phi, EconomicMap phiValues, AbstractBeginNode successor, AbstractMergeNode oldMerge, SimplifierTool tool) { if (!ends.isEmpty()) { // If there was a value proxy usage, then the proxy needs a new value. - ValueProxyNode valueProxy = null; + List valueProxies; if (successor instanceof LoopExitNode) { - for (Node usage : phi.usages()) { - if (usage instanceof ValueProxyNode && ((ValueProxyNode) usage).proxyPoint() == successor) { - valueProxy = (ValueProxyNode) usage; - } - } + /* + * In rare cases the ValueProxyNodes might not have GVN'ed so handle as many + * matching ValueProxyNodes as exist. + */ + valueProxies = phi.usages().filter(u -> u instanceof ValueProxyNode && ((ValueProxyNode) u).proxyPoint() == successor).snapshot(); + } else { + valueProxies = null; } - final ValueProxyNode proxy = valueProxy; if (ends.size() == 1) { AbstractEndNode end = ends.get(0); - if (proxy != null) { - phi.replaceAtUsages(phiValues.get(end), n -> n == proxy); + if (valueProxies != null) { + phi.replaceAtUsages(phiValues.get(end), valueProxies::contains); } ((FixedWithNextNode) end.predecessor()).setNext(successor); oldMerge.removeEnd(end); @@ -2281,8 +2282,8 @@ private void connectEnds(List ends, ValuePhiNode phi, EconomicMap n == proxy); + if (valueProxies != null) { + phi.replaceAtUsages(newPhi, valueProxies::contains); } for (EndNode end : ends) { From 1a5c83857f5f9fd163c561e2cb6479c3a8eff105 Mon Sep 17 00:00:00 2001 From: Tom Rodriguez Date: Fri, 18 Oct 2024 12:03:43 -0700 Subject: [PATCH 2/3] Add sanity check that killCFG doesn't produce orphaned control flow --- .../src/jdk/graal/compiler/nodes/util/GraphUtil.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/util/GraphUtil.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/util/GraphUtil.java index 4b2f28463fc9..781bc8b2efdb 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/util/GraphUtil.java +++ b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/util/GraphUtil.java @@ -149,6 +149,11 @@ private static void killCFGInner(FixedNode node) { marked.markDeleted(); } } + for (Node m : markedNodes) { + if (m instanceof FixedWithNextNode fixed) { + GraalError.guarantee(fixed.next() == null || fixed.next().isDeleted(), "dead node %s has live next %s", m, fixed.next()); + } + } } private static void markFixedNodes(FixedNode node, EconomicSet markedNodes, EconomicMap> unmarkedMerges) { From c7c9bbe0d0800a8d8db2514972cb0b6301ff6311 Mon Sep 17 00:00:00 2001 From: Tom Rodriguez Date: Sun, 20 Oct 2024 12:33:45 -0700 Subject: [PATCH 3/3] Fix malformed test graph --- .../test/LoopSafepointStateVerificationTest.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/LoopSafepointStateVerificationTest.java b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/LoopSafepointStateVerificationTest.java index e91f279d74f4..36010e377487 100644 --- a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/LoopSafepointStateVerificationTest.java +++ b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/LoopSafepointStateVerificationTest.java @@ -26,12 +26,12 @@ import java.util.Optional; -import jdk.graal.compiler.debug.DebugOptions; import org.junit.Assert; import org.junit.Test; import jdk.graal.compiler.api.directives.GraalDirectives; import jdk.graal.compiler.core.common.GraalOptions; +import jdk.graal.compiler.debug.DebugOptions; import jdk.graal.compiler.debug.TTY; import jdk.graal.compiler.graph.Graph; import jdk.graal.compiler.nodes.EndNode; @@ -40,6 +40,7 @@ import jdk.graal.compiler.nodes.GraphState; import jdk.graal.compiler.nodes.LoopBeginNode; import jdk.graal.compiler.nodes.LoopEndNode; +import jdk.graal.compiler.nodes.LoopExitNode; import jdk.graal.compiler.nodes.PhiNode; import jdk.graal.compiler.nodes.StructuredGraph; import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions; @@ -78,7 +79,7 @@ public void test01() { test(opt, "snippet01"); Assert.fail("Should have detected that the phase in this class does not retain the mustNotSafepoint flag of a loop begin"); } catch (Throwable t) { - assert t.getMessage().contains("previously the loop had canHaveSafepoints=false but now it has canHaveSafepoints=true"); + assert t.toString().contains("previously the loop had canHaveSafepoints=false but now it has canHaveSafepoints=true") : t; } } @@ -144,6 +145,9 @@ protected void run(StructuredGraph graph, HighTierContext context) { LoopBeginNode oldLoopBegin = lex.loopBegin(); EndNode fwd = oldLoopBegin.forwardEnd(); + for (LoopExitNode exit : oldLoopBegin.loopExits().snapshot()) { + exit.setLoopBegin(lb); + } FixedNode next = oldLoopBegin.next(); oldLoopBegin.setNext(null); @@ -153,10 +157,8 @@ protected void run(StructuredGraph graph, HighTierContext context) { lb.addForwardEnd(fwdEnd); FixedWithNextNode fwn = (FixedWithNextNode) fwd.predecessor(); - fwn.setNext(null); - GraphUtil.killCFG(fwd); fwn.setNext(fwdEnd); - + GraphUtil.killCFG(fwd); } });