Skip to content

Commit

Permalink
New assertions when replacing nodes. Change PiNode canonicalization. …
Browse files Browse the repository at this point in the history 
…Fix conditional elimination to currectly work with unproxified input.
  • Loading branch information
@thomaswue
thomaswue committed Feb 11, 2017
1 parent 582bd97 commit c07bcb5
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 33 deletions.
4 changes: 3 additions & 1 deletion graal/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Node.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,9 @@ public void replaceAtPredecessor(Node other) {
public void replaceAndDelete(Node other) {
assert checkReplaceWith(other);
assert other != null;
replaceAtUsages(other);
if (this.hasUsages()) {
replaceAtUsages(other);
}
replaceAtPredecessor(other);
this.safeDelete();
}
Expand Down
2 changes: 1 addition & 1 deletion ...ompiler.hotspot/src/org/graalvm/compiler/hotspot/meta/DefaultHotSpotLoweringProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ private ReadNode createReadVirtualMethod(StructuredGraph graph, ValueNode hub, i
assert vtableEntryOffset > 0;
// We use LocationNode.ANY_LOCATION for the reads that access the vtable
// entry as HotSpot does not guarantee that this is a final value.
Stamp methodStamp = MethodPointerStamp.method();
Stamp methodStamp = MethodPointerStamp.methodNonNull();
AddressNode address = createOffsetAddress(graph, hub, vtableEntryOffset);
ReadNode metaspaceMethod = graph.add(new ReadNode(address, any(), methodStamp, BarrierType.NONE));
return metaspaceMethod;
Expand Down
15 changes: 5 additions & 10 deletions graal/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/PiNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@
import org.graalvm.compiler.graph.spi.CanonicalizerTool;
import org.graalvm.compiler.nodeinfo.NodeInfo;
import org.graalvm.compiler.nodes.extended.GuardingNode;
import org.graalvm.compiler.nodes.extended.UnsafeLoadNode;
import org.graalvm.compiler.nodes.java.LoadFieldNode;
import org.graalvm.compiler.nodes.memory.ReadNode;
import org.graalvm.compiler.nodes.spi.LIRLowerable;
import org.graalvm.compiler.nodes.spi.NodeLIRBuilderTool;
import org.graalvm.compiler.nodes.spi.ValueProxy;
Expand Down Expand Up @@ -148,14 +147,10 @@ public Node canonical(CanonicalizerTool tool) {
if (g == null) {

// Try to merge the pi node with a load node.
if (o instanceof LoadFieldNode) {
LoadFieldNode loadFieldNode = (LoadFieldNode) o;
loadFieldNode.setStamp(loadFieldNode.stamp().improveWith(this.piStamp));
return loadFieldNode;
} else if (o instanceof UnsafeLoadNode) {
UnsafeLoadNode unsafeLoadNode = (UnsafeLoadNode) o;
unsafeLoadNode.setStamp(unsafeLoadNode.stamp().improveWith(this.piStamp));
return unsafeLoadNode;
if (o instanceof ReadNode) {
ReadNode readNode = (ReadNode) o;
readNode.setStamp(readNode.stamp().improveWith(this.piStamp));
return readNode;
}
} else {
for (Node n : g.asNode().usages()) {
Expand Down
19 changes: 19 additions & 0 deletions graal/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/ValueNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@
*/
package org.graalvm.compiler.nodes;

import java.util.function.Predicate;

import org.graalvm.compiler.core.common.type.Stamp;
import org.graalvm.compiler.graph.Node;
import org.graalvm.compiler.graph.NodeClass;
import org.graalvm.compiler.graph.iterators.NodePredicate;
import org.graalvm.compiler.nodeinfo.InputType;
import org.graalvm.compiler.nodeinfo.NodeInfo;
import org.graalvm.compiler.nodeinfo.Verbosity;
import org.graalvm.compiler.nodes.spi.NodeValueMap;

import jdk.vm.ci.meta.Constant;
Expand Down Expand Up @@ -182,4 +185,20 @@ public boolean hasUsagesOtherThan(ValueNode node, NodeValueMap nodeValueMap) {
return false;
}

@Override
protected void replaceAtUsages(Node other, Predicate<Node> filter, Node toBeDeleted) {
super.replaceAtUsages(other, filter, toBeDeleted);
assert checkReplaceAtUsagesInvariants(other);
}

private boolean checkReplaceAtUsagesInvariants(Node other) {
assert other == null || other instanceof ValueNode;
if (this.hasUsages() && !this.stamp().isEmpty() && !(other instanceof PhiNode) && other != null) {
assert ((ValueNode) other).stamp().getClass() == stamp().getClass() : "stamp have to be of same class";
boolean morePrecise = ((ValueNode) other).stamp().join(stamp()).equals(((ValueNode) other).stamp());
assert morePrecise : "stamp can only get more precise " + toString(Verbosity.All) + " " +
other.toString(Verbosity.All);
}
return true;
}
}
9 changes: 0 additions & 9 deletions graal/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/memory/FloatingReadNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,11 @@
import org.graalvm.compiler.core.common.type.ObjectStamp;
import org.graalvm.compiler.core.common.type.Stamp;
import org.graalvm.compiler.debug.DebugCloseable;
import org.graalvm.compiler.debug.TTY;
import org.graalvm.compiler.graph.Node;
import org.graalvm.compiler.graph.NodeClass;
import org.graalvm.compiler.graph.spi.Canonicalizable;
import org.graalvm.compiler.graph.spi.CanonicalizerTool;
import org.graalvm.compiler.nodeinfo.NodeInfo;
import org.graalvm.compiler.nodeinfo.Verbosity;
import org.graalvm.compiler.nodes.ValueNodeUtil;
import org.graalvm.compiler.nodes.extended.GuardingNode;
import org.graalvm.compiler.nodes.memory.address.AddressNode;
Expand Down Expand Up @@ -65,13 +63,6 @@ public FloatingReadNode(AddressNode address, LocationIdentity location, MemoryNo
super(TYPE, address, location, stamp, guard, barrierType);
this.lastLocationAccess = lastLocationAccess;

if (!(guard != null || !(address.getBase().stamp() instanceof ObjectStamp) || ((ObjectStamp) address.getBase().stamp()).nonNull())) {
for (Node n : address.getBase().graph().getNodes()) {
TTY.println(n.toString(Verbosity.All));
TTY.println("inputs: " + n.inputs());
}
}

// The input to floating reads must be always non-null or have at least a guard.
assert guard != null || !(address.getBase().stamp() instanceof ObjectStamp) || ((ObjectStamp) address.getBase().stamp()).nonNull() : address.getBase();
}
Expand Down
35 changes: 23 additions & 12 deletions ....phases.common/src/org/graalvm/compiler/phases/common/NewConditionalEliminationPhase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,14 @@ protected void processConditionAnchor(ConditionAnchorNode node) {
});
}

private static void rewirePiNodes(GuardingNode node, Stamp guardedValueStamp, ValueProxy newInput) {
private static void rewirePiNodes(GuardingNode node, Stamp guardedValueStamp, ValueNode newInput) {
ValueNode unproxified = GraphUtil.unproxify(newInput);
for (Node usage : node.asNode().usages().snapshot()) {
if (usage instanceof PiNode) {
PiNode piNode = (PiNode) usage;

if (piNode.getOriginalNode() != newInput && GraphUtil.unproxify(piNode.getOriginalNode()) == unproxified) {
piNode.setOriginalNode((ValueNode) newInput.asNode());
piNode.setOriginalNode(newInput);
}

if (piNode.getOriginalNode() == newInput) {
Expand Down Expand Up @@ -506,7 +506,7 @@ protected InfoElement getInfoElements(ValueNode proxiedValue) {
return map.get(value);
}

protected boolean rewireGuards(GuardingNode guard, boolean result, ValueProxy proxifiedInput, Stamp guardedValueStamp, GuardRewirer rewireGuardFunction) {
protected boolean rewireGuards(GuardingNode guard, boolean result, ValueNode proxifiedInput, Stamp guardedValueStamp, GuardRewirer rewireGuardFunction) {
counterStampsFound.increment();
return rewireGuardFunction.rewire(guard, result, guardedValueStamp, proxifiedInput);
}
Expand All @@ -519,7 +519,9 @@ protected boolean tryProveGuardCondition(DeoptimizingGuard thisGuard, LogicNode
InfoElement infoElement = getInfoElements(node);
if (infoElement != null) {
assert infoElement.getStamp() == StampFactory.tautology() || infoElement.getStamp() == StampFactory.contradiction();
return rewireGuards(infoElement.getGuard(), infoElement.getStamp() == StampFactory.tautology(), infoElement.getProxifiedInput(), infoElement.getStamp(), rewireGuardFunction);
// No proxified input required.
ValueNode proxifiedInput = null;
return rewireGuards(infoElement.getGuard(), infoElement.getStamp() == StampFactory.tautology(), proxifiedInput, infoElement.getStamp(), rewireGuardFunction);
}
if (node instanceof UnaryOpLogicNode) {
UnaryOpLogicNode unaryLogicNode = (UnaryOpLogicNode) node;
Expand Down Expand Up @@ -648,8 +650,18 @@ protected boolean tryProveGuardCondition(DeoptimizingGuard thisGuard, LogicNode
return rewireGuards(guard, true, newInput, guardedValueStamp, rewireGuardFunction);
} else {
return tryProveCondition(shortCircuitOrNode.getY(), (innerGuard, innerResult, innerGuardedValueStamp, innerNewInput) -> {
if (innerGuard == guard && newInput == innerNewInput) {
return rewireGuards(guard, innerResult ^ shortCircuitOrNode.isYNegated(), newInput, innerGuardedValueStamp, rewireGuardFunction);
ValueNode proxifiedInput = newInput;
if (proxifiedInput == null) {
proxifiedInput = innerNewInput;
} else if (innerNewInput != null) {
if (innerNewInput != newInput) {
// Cannot canonicalize due to different proxied inputs.
return false;
}
}
// Can only canonicalize if the guards are equal.
if (innerGuard == guard) {
return rewireGuards(guard, innerResult ^ shortCircuitOrNode.isYNegated(), proxifiedInput, guardedValueStamp, rewireGuardFunction);
}
return false;
});
Expand All @@ -665,9 +677,8 @@ protected void registerNewStamp(ValueNode maybeProxiedValue, Stamp newStamp, Gua
assert guard != null;
if (newStamp != null) {
ValueNode value = maybeProxiedValue;
ValueProxy proxiedValue = null;
ValueNode proxiedValue = value;
if (value instanceof ValueProxy) {
proxiedValue = (ValueProxy) value;
value = GraphUtil.unproxify(value);
}
counterStampsRegistered.increment();
Expand Down Expand Up @@ -803,7 +814,7 @@ protected interface GuardRewirer {
* @param newInput new input to pi nodes depending on the new guard
* @return whether the transformation could be applied
*/
boolean rewire(GuardingNode guard, boolean result, Stamp guardedValueStamp, ValueProxy newInput);
boolean rewire(GuardingNode guard, boolean result, Stamp guardedValueStamp, ValueNode newInput);
}

protected static class PendingTest {
Expand All @@ -819,10 +830,10 @@ public PendingTest(LogicNode condition, DeoptimizingGuard guard) {
protected static final class InfoElement {
private final Stamp stamp;
private final GuardingNode guard;
private final ValueProxy proxifiedInput;
private final ValueNode proxifiedInput;
private final InfoElement parent;

public InfoElement(Stamp stamp, GuardingNode guard, ValueProxy proxifiedInput, InfoElement parent) {
public InfoElement(Stamp stamp, GuardingNode guard, ValueNode proxifiedInput, InfoElement parent) {
this.stamp = stamp;
this.guard = guard;
this.proxifiedInput = proxifiedInput;
Expand All @@ -841,7 +852,7 @@ public GuardingNode getGuard() {
return guard;
}

public ValueProxy getProxifiedInput() {
public ValueNode getProxifiedInput() {
return proxifiedInput;
}

Expand Down

0 comments on commit c07bcb5

Please sign in to comment.