Skip to content

Commit

Permalink
[GR-43827] Add limited dominator-based GVN to the fix reads phase.
Browse files Browse the repository at this point in the history 
PullRequest: graal/13618
  • Loading branch information
@ntemmar
ntemmar committed Feb 2, 2023
2 parents 6d51c4a + 4838f5b commit c051ca9
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 34 deletions.
60 changes: 39 additions & 21 deletions compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Graph.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.graalvm.compiler.graph.Node.NodeInsertionStackTrace;
import org.graalvm.compiler.graph.Node.ValueNumberable;
import org.graalvm.compiler.graph.iterators.NodeIterable;
import org.graalvm.compiler.graph.iterators.NodePredicate;
import org.graalvm.compiler.options.Option;
import org.graalvm.compiler.options.OptionKey;
import org.graalvm.compiler.options.OptionType;
Expand Down Expand Up @@ -464,37 +465,51 @@ public <T extends Node> T addOrUnique(T node) {
return node;
}
if (node.getNodeClass().valueNumberable()) {
return uniqueHelper(node);
return uniqueHelper(node, null);
}
return add(node);
}

public <T extends Node> T addOrUniqueWithInputs(T node) {
return addOrUniqueWithInputs(node, null);
}

public <T extends Node> T addOrUniqueWithInputs(T node, NodePredicate predicate) {
if (node.isAlive()) {
assert node.graph() == this;
return node;
} else {
assert node.isUnregistered();
addInputs(node);
addInputs(node, predicate);
if (node.getNodeClass().valueNumberable()) {
return uniqueHelper(node);
return uniqueHelper(node, predicate);
}
return add(node);
}
}

public <T extends Node> T addWithoutUniqueWithInputs(T node) {
addInputs(node);
addInputs(node, null);
return addHelper(node);
}

private final class AddInputsFilter extends Node.EdgeVisitor {

private final NodePredicate predicate;

AddInputsFilter(NodePredicate predicate) {
this.predicate = predicate;
}

AddInputsFilter() {
this(null);
}

@Override
public Node apply(Node self, Node input) {
if (!input.isAlive()) {
assert !input.isDeleted();
return addOrUniqueWithInputs(input);
return addOrUniqueWithInputs(input, predicate);
} else {
return input;
}
Expand All @@ -504,8 +519,12 @@ public Node apply(Node self, Node input) {

private AddInputsFilter addInputsFilter = new AddInputsFilter();

private <T extends Node> void addInputs(T node) {
node.applyInputs(addInputsFilter);
private <T extends Node> void addInputs(T node, NodePredicate predicate) {
if (predicate == null) {
node.applyInputs(addInputsFilter);
} else {
node.applyInputs(new AddInputsFilter(predicate));
}
}

private <T extends Node> T addHelper(T node) {
Expand Down Expand Up @@ -736,12 +755,12 @@ public NodeEventScope trackNodeEvents(NodeEventListener listener) {
* @return a node similar to {@code node} if one exists, otherwise {@code node}
*/
public <T extends Node & ValueNumberable> T unique(T node) {
return uniqueHelper(node);
return uniqueHelper(node, null);
}

<T extends Node> T uniqueHelper(T node) {
<T extends Node> T uniqueHelper(T node, NodePredicate predicate) {
assert node.getNodeClass().valueNumberable();
T other = this.findDuplicate(node);
T other = this.findDuplicate(node, predicate);
if (other != null) {
if (other.getNodeSourcePosition() == null) {
other.setNodeSourcePosition(node.getNodeSourcePosition());
Expand Down Expand Up @@ -802,18 +821,23 @@ Node findNodeInCache(Node node) {
return result;
}

@SuppressWarnings("unchecked")
public <T extends Node> T findDuplicate(T node) {
return findDuplicate(node, null);
}

/**
* Returns a possible duplicate for the given node in the graph or {@code null} if no such
* duplicate exists.
* duplicate exists. The predicate parameter is used to filter potential results.
*/
@SuppressWarnings("unchecked")
public <T extends Node> T findDuplicate(T node) {
public <T extends Node> T findDuplicate(T node, NodePredicate predicate) {
NodeClass<?> nodeClass = node.getNodeClass();
assert nodeClass.valueNumberable();
if (nodeClass.isLeafNode()) {
// Leaf node: look up in cache
Node cachedNode = findNodeInCache(node);
if (cachedNode != null && cachedNode != node) {
if (cachedNode != null && cachedNode != node && (predicate == null || predicate.apply(cachedNode))) {
return (T) cachedNode;
} else {
return null;
Expand All @@ -840,7 +864,7 @@ public <T extends Node> T findDuplicate(T node) {
}
if (minCountNode != null) {
for (Node usage : minCountNode.usages()) {
if (usage != node && nodeClass == usage.getNodeClass() && node.valueEquals(usage) && nodeClass.equalInputs(node, usage) &&
if (usage != node && nodeClass == usage.getNodeClass() && (predicate == null || predicate.apply(usage)) && node.valueEquals(usage) && nodeClass.equalInputs(node, usage) &&
nodeClass.equalSuccessors(node, usage)) {
return (T) usage;
}
Expand Down Expand Up @@ -910,13 +934,7 @@ public Mark getMark() {
*/
public NodeIterable<Node> getNewNodes(Mark mark) {
final int index = mark == null ? 0 : mark.getValue();
return new NodeIterable<>() {

@Override
public Iterator<Node> iterator() {
return new GraphNodeIterator(Graph.this, index);
}
};
return () -> new GraphNodeIterator(Graph.this, index);
}

/**
Expand Down
91 changes: 78 additions & 13 deletions ....graalvm.compiler.phases.common/src/org/graalvm/compiler/phases/common/FixReadsPhase.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2017, 2023, 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
Expand Down Expand Up @@ -36,10 +36,13 @@
import org.graalvm.compiler.core.common.type.StampFactory;
import org.graalvm.compiler.debug.CounterKey;
import org.graalvm.compiler.debug.DebugContext;
import org.graalvm.compiler.graph.Graph;
import org.graalvm.compiler.graph.Node;
import org.graalvm.compiler.graph.NodeBitMap;
import org.graalvm.compiler.graph.NodeMap;
import org.graalvm.compiler.graph.NodeStack;
import org.graalvm.compiler.graph.Position;
import org.graalvm.compiler.graph.iterators.NodePredicate;
import org.graalvm.compiler.nodeinfo.InputType;
import org.graalvm.compiler.nodes.AbstractBeginNode;
import org.graalvm.compiler.nodes.AbstractMergeNode;
Expand Down Expand Up @@ -91,6 +94,14 @@

/**
* This phase lowers {@link FloatingReadNode FloatingReadNodes} into corresponding fixed reads.
* After this operation, there are no longer any nodes in the graph that have to remain below a
* control flow split to be considered "safe". Therefore, this phase subsequently removes all
* {@link PiNode} instances from the graph. Then it runs a raw conditional elimination
* {@link RawConditionalEliminationVisitor} that aggressively uses stamps for values based on
* control flow. For every if node, the logic node is inspected and a stamp is derived for the true
* and false branch. Stamps for a value are combined based on all previous knowledge about that
* value. For merge points, a union of the stamps of a value is constructed. When a value is used,
* the corresponding best derived stamp is provided to the canonicalizer.
*/
public class FixReadsPhase extends BasePhase<CoreProviders> {

Expand All @@ -107,8 +118,27 @@ public float codeSizeIncrease() {

private static class FixReadsClosure extends ScheduledNodeIterator {

/**
* Bitmap that is used to schedule nodes for inferring a new stamp when they are visited.
* After removing the pi nodes from the graph, the stamp information injected by the pi
* nodes is cleared this way.
*/
private final NodeBitMap inferStampBitmap;

FixReadsClosure(StructuredGraph graph) {
inferStampBitmap = graph.createNodeBitMap();
}

@Override
protected void processNode(Node node, HIRBlock block, ScheduleResult schedule, ListIterator<Node> iter) {
if (inferStampBitmap.isMarked(node) && node instanceof ValueNode) {
ValueNode valueNode = (ValueNode) node;
if (valueNode.inferStamp()) {
for (Node n : valueNode.usages()) {
inferStampBitmap.mark(n);
}
}
}
if (node instanceof AbstractMergeNode) {
AbstractMergeNode mergeNode = (AbstractMergeNode) node;
for (MemoryPhiNode memoryPhi : mergeNode.memoryPhis().snapshot()) {
Expand All @@ -129,7 +159,11 @@ protected void processNode(Node node, HIRBlock block, ScheduleResult schedule, L
} else if (node instanceof PiNode) {
PiNode piNode = (PiNode) node;
if (piNode.stamp(NodeView.DEFAULT).isCompatible(piNode.getOriginalNode().stamp(NodeView.DEFAULT))) {
// Pi nodes are no longer necessary at this point.
// Pi nodes are no longer necessary at this point. Make sure to infer stamps
// for all usages to clear out the stamp information added by the pi node.
for (Node n : piNode.usages()) {
inferStampBitmap.mark(n);
}
piNode.replaceAndDelete(piNode.getOriginalNode());
}
} else if (node instanceof MemoryAccess) {
Expand All @@ -152,6 +186,7 @@ public static class RawConditionalEliminationVisitor implements RecursiveVisitor
private final EconomicMap<MergeNode, EconomicMap<ValueNode, Stamp>> endMaps;
private final DebugContext debug;
private final RawCanonicalizerTool rawCanonicalizerTool;
private final EconomicMap<Node, HIRBlock> nodeToBlockMap;

private class RawCanonicalizerTool extends CoreProvidersDelegate implements NodeView, CanonicalizerTool {

Expand Down Expand Up @@ -211,6 +246,7 @@ public RawConditionalEliminationVisitor(StructuredGraph graph, ScheduleResult sc
stampMap = graph.createNodeMap();
undoOperations = new NodeStack();
replaceConstantInputs = replaceInputsWithConstants && GraalOptions.ReplaceInputsWithConstantsBasedOnStamps.getValue(graph.getOptions());
nodeToBlockMap = EconomicMap.create();
}

protected void replaceInput(Position p, Node oldInput, Node newConstantInput) {
Expand Down Expand Up @@ -251,13 +287,29 @@ protected int replaceConstantInputs(Node node) {
return replacements;
}

protected void processNode(Node node) {
private static boolean nonNullAndDominates(HIRBlock a, HIRBlock b) {
if (a == null) {
return false;
} else {
return a.dominates(b);
}
}

protected void processNode(Node node, HIRBlock b, NodePredicate nodePredicate) {
assert node.isAlive();

if (replaceConstantInputs) {
replaceConstantInputs(node);
}

if (node.getNodeClass().valueNumberable()) {
Node dominatingDuplicate = graph.findDuplicate(node, nodePredicate);
if (dominatingDuplicate != null) {
node.replaceAndDelete(dominatingDuplicate);
return;
}
}

if (node instanceof MergeNode) {
registerCombinedStamps((MergeNode) node);
}
Expand All @@ -269,14 +321,18 @@ protected void processNode(Node node) {
} else if (node instanceof IntegerSwitchNode) {
processIntegerSwitch((IntegerSwitchNode) node);
} else if (node instanceof BinaryNode) {
processBinary((BinaryNode) node);
processBinary((BinaryNode) node, b, nodePredicate);
} else if (node instanceof ConditionalNode) {
processConditional((ConditionalNode) node);
} else if (node instanceof UnaryNode) {
processUnary((UnaryNode) node);
processUnary((UnaryNode) node, b, nodePredicate);
} else if (node instanceof EndNode) {
processEnd((EndNode) node);
}

if (node.getNodeClass().valueNumberable() && node.isAlive()) {
nodeToBlockMap.put(node, b);
}
}

protected void registerCombinedStamps(MergeNode node) {
Expand Down Expand Up @@ -378,7 +434,7 @@ private static HIRBlock getBlock(ValueNode node, NodeMap<HIRBlock> blockToNodeMa
return blockToNodeMap.get(node);
}

protected void processUnary(UnaryNode node) {
protected void processUnary(UnaryNode node, HIRBlock block, NodePredicate gvnPredicate) {
ValueNode value = node.getValue();
Stamp bestStamp = getBestStamp(value);
Stamp newStamp = node.foldStamp(bestStamp);
Expand All @@ -388,7 +444,7 @@ protected void processUnary(UnaryNode node) {
if (newNode != node) {
// Canonicalization successfully triggered.
if (newNode != null && !newNode.isAlive()) {
newNode = graph.addWithoutUniqueWithInputs(newNode);
newNode = addHelper(newNode, block, gvnPredicate);
}
node.replaceAndDelete(newNode);
GraphUtil.tryKillUnused(value);
Expand All @@ -399,6 +455,15 @@ protected void processUnary(UnaryNode node) {
}
}

private ValueNode addHelper(ValueNode newNode, HIRBlock block, NodePredicate gvnPredicate) {
Graph.Mark m = graph.getMark();
ValueNode result = graph.addOrUniqueWithInputs(newNode, gvnPredicate);
for (Node n : graph.getNewNodes(m)) {
nodeToBlockMap.put(n, block);
}
return result;
}

protected boolean checkReplaceWithConstant(Stamp newStamp, ValueNode node) {
Constant constant = newStamp.asConstant();
if (constant != null && !(node instanceof ConstantNode)) {
Expand All @@ -411,7 +476,7 @@ protected boolean checkReplaceWithConstant(Stamp newStamp, ValueNode node) {
return false;
}

protected void processBinary(BinaryNode node) {
protected void processBinary(BinaryNode node, HIRBlock b, NodePredicate nodePredicate) {

ValueNode x = node.getX();
ValueNode y = node.getY();
Expand All @@ -428,7 +493,7 @@ protected void processBinary(BinaryNode node) {
if (newNode != node) {
// Canonicalization successfully triggered.
if (newNode != null && !newNode.isAlive()) {
newNode = graph.addWithoutUniqueWithInputs(newNode);
newNode = addHelper(newNode, b, nodePredicate);
}
node.replaceAndDelete(newNode);
GraphUtil.tryKillUnused(x);
Expand Down Expand Up @@ -568,9 +633,10 @@ protected Stamp getBestStamp(ValueNode value) {
public Integer enter(HIRBlock b) {
int mark = undoOperations.size();
blockActionStart.put(b, mark);
NodePredicate nodePredicate = n -> nonNullAndDominates(nodeToBlockMap.get(n), b);
for (Node n : schedule.getBlockToNodesMap().get(b)) {
if (n.isAlive()) {
processNode(n);
processNode(n, b, nodePredicate);
}
}
return mark;
Expand Down Expand Up @@ -608,7 +674,7 @@ protected void run(StructuredGraph graph, CoreProviders context) {
assert graph.verify();
schedulePhase.apply(graph, context);
ScheduleResult schedule = graph.getLastSchedule();
FixReadsClosure fixReadsClosure = new FixReadsClosure();
FixReadsClosure fixReadsClosure = new FixReadsClosure(graph);
for (HIRBlock block : schedule.getCFG().getBlocks()) {
fixReadsClosure.processNodes(block, schedule);
}
Expand Down Expand Up @@ -652,7 +718,7 @@ public String toString() {
result.append(stamp);
if (this.parent != null) {
result.append(" (");
result.append(this.parent.toString());
result.append(this.parent);
result.append(")");
}
return result.toString();
Expand All @@ -662,5 +728,4 @@ public String toString() {
public BasePhase<? super CoreProviders> getSchedulePhase() {
return schedulePhase;
}

}

0 comments on commit c051ca9

Please sign in to comment.