Skip to content

Commit

Permalink
[GR-59124] Handle multiple ValueProxyNodes in removeIntermediateMater…
Browse files Browse the repository at this point in the history
…ialization

PullRequest: graal/19084
  • Loading branch information
tkrodriguez committed Oct 23, 2024
2 parents 59c335a + c7c9bbe commit feb2135
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
}

});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2256,19 +2256,20 @@ private static void propagateInjectedProfile(BranchProbabilityData profile, EndN
private void connectEnds(List<EndNode> ends, ValuePhiNode phi, EconomicMap<AbstractEndNode, ValueNode> 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<Node> 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);
Expand All @@ -2281,8 +2282,8 @@ private void connectEnds(List<EndNode> ends, ValuePhiNode phi, EconomicMap<Abstr
PhiNode oldPhi = (PhiNode) oldMerge.usages().first();
PhiNode newPhi = graph().addWithoutUnique(new ValuePhiNode(oldPhi.stamp(view), newMerge));

if (proxy != null) {
phi.replaceAtUsages(newPhi, n -> n == proxy);
if (valueProxies != null) {
phi.replaceAtUsages(newPhi, valueProxies::contains);
}

for (EndNode end : ends) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Node> markedNodes, EconomicMap<AbstractMergeNode, List<AbstractEndNode>> unmarkedMerges) {
Expand Down

0 comments on commit feb2135

Please sign in to comment.