# HG changeset patch # User Doug Simon # Date 1351848098 -3600 # Node ID 370674104c1ce07a2935a62c6b9296cada691a61 # Parent 8e9ffa7c714bf9e9243f552d84b9972e35dd22e4 simplified and improved the re-use of an instanceof snippet instantiation across all of the usages of the InstanceOfNode diff -r 8e9ffa7c714b -r 370674104c1c graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/snippets/InstanceOfSnippets.java --- a/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/snippets/InstanceOfSnippets.java Thu Nov 01 17:33:48 2012 +0100 +++ b/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/snippets/InstanceOfSnippets.java Fri Nov 02 10:21:38 2012 +0100 @@ -24,6 +24,7 @@ import static com.oracle.graal.hotspot.snippets.CheckCastSnippets.*; import static com.oracle.graal.hotspot.snippets.CheckCastSnippets.Templates.*; import static com.oracle.graal.hotspot.snippets.HotSpotSnippetUtils.*; +import static com.oracle.graal.nodes.MaterializeNode.*; import static com.oracle.graal.snippets.Snippet.Varargs.*; import static com.oracle.graal.snippets.SnippetTemplate.Arguments.*; @@ -188,26 +189,29 @@ List usages = instanceOf.usages().snapshot(); int nUsages = usages.size(); - Map materializations = nUsages == 1 ? null : new HashMap(nUsages); + Instantiation instantiation = new Instantiation(); for (Node usage : usages) { final StructuredGraph graph = (StructuredGraph) usage.graph(); - UsageReplacer replacer; - MaterializationKey mkey; + InstanceOfUsageReplacer replacer; + ValueNode trueValue; + ValueNode falseValue; if (usage instanceof IfNode) { - mkey = new MaterializationKey(ConstantNode.forInt(1, graph), ConstantNode.forInt(0, graph)); - replacer = new IfUsageReplacer(materializations, mkey, instanceOf, (IfNode) usage, tool); + trueValue = ConstantNode.forInt(1, graph); + falseValue = ConstantNode.forInt(0, graph); + replacer = new IfUsageReplacer(instantiation, trueValue, falseValue, instanceOf, (IfNode) usage, nUsages == 1, tool); } else { assert usage instanceof ConditionalNode : "unexpected usage of " + instanceOf + ": " + usage; ConditionalNode conditional = (ConditionalNode) usage; - mkey = new MaterializationKey(conditional.trueValue(), conditional.falseValue()); - replacer = new ConditionalUsageReplacer(materializations, mkey, instanceOf, conditional); + trueValue = conditional.trueValue(); + falseValue = conditional.falseValue(); + replacer = new ConditionalUsageReplacer(instantiation, trueValue, falseValue, instanceOf, conditional); } - Materialization materialization = materializations == null ? null : materializations.get(mkey); - if (materialization != null) { - usage.replaceFirstInput(instanceOf, materialization.condition(mkey)); + if (instantiation.isInitialized()) { + // No need to re-instantiate the snippet - just re-use its result + replacer.replaceUsingInstantiation(); } else { Arguments arguments; Key key; @@ -215,14 +219,14 @@ HotSpotKlassOop[] hints = createHints(hintInfo); assert hints.length == 1; key = new Key(instanceofExact).add("checkNull", checkNull); - arguments = arguments("object", object).add("exactHub", hints[0]).add("trueValue", mkey.trueValue).add("falseValue", mkey.falseValue); + arguments = arguments("object", object).add("exactHub", hints[0]).add("trueValue", trueValue).add("falseValue", falseValue); } else if (type.isPrimaryType()) { key = new Key(instanceofPrimary).add("checkNull", checkNull).add("superCheckOffset", type.superCheckOffset()); - arguments = arguments("hub", hub).add("object", object).add("trueValue", mkey.trueValue).add("falseValue", mkey.falseValue); + arguments = arguments("hub", hub).add("object", object).add("trueValue", trueValue).add("falseValue", falseValue); } else { HotSpotKlassOop[] hints = createHints(hintInfo); key = new Key(instanceofSecondary).add("hints", vargargs(Object.class, hints.length)).add("checkNull", checkNull); - arguments = arguments("hub", hub).add("object", object).add("hints", hints).add("trueValue", mkey.trueValue).add("falseValue", mkey.falseValue); + arguments = arguments("hub", hub).add("object", object).add("hints", hints).add("trueValue", trueValue).add("falseValue", falseValue); } SnippetTemplate template = cache.get(key); @@ -237,77 +241,104 @@ } /** - * The materialized result of an instanceof snippet. - * All usages of an {@link InstanceOfNode} that have the same key can share the result. + * The result of an instantiating an instanceof snippet. + * This enables a snippet instantiation to be re-used which reduces compile time and produces better code. */ - static final class Materialization { - private final PhiNode phi; + static final class Instantiation { + private PhiNode result; private CompareNode condition; + private ValueNode trueValue; + private ValueNode falseValue; - - public Materialization(PhiNode phi) { - this.phi = phi; + /** + * Determines if the instantiation has occurred. + */ + boolean isInitialized() { + return result != null; } - CompareNode condition(MaterializationKey key) { - if (condition == null) { - StructuredGraph graph = (StructuredGraph) phi.graph(); - condition = graph.add(new IntegerEqualsNode(phi, key.trueValue)); + void initialize(PhiNode phi, ValueNode t, ValueNode f) { + assert !isInitialized(); + this.result = phi; + this.trueValue = t; + this.falseValue = f; + } + + /** + * Gets the result of this instantiation as a condition. + * + * @param testValue the returned condition is true if the result is equal to this value + */ + CompareNode asCondition(ValueNode testValue) { + assert isInitialized(); + if (condition == null || condition.y() != testValue) { + // Re-use previously generated condition if the trueValue for the test is the same + condition = createCompareNode(Condition.EQ, result, testValue); } return condition; } + + /** + * Gets the result of the instantiation as a materialized value. + * + * @param t the true value for the materialization + * @param f the false value for the materialization + */ + ValueNode asMaterialization(ValueNode t, ValueNode f) { + assert isInitialized(); + if (t == this.trueValue && f == this.falseValue) { + // Can simply use the phi result if the same materialized values are expected. + return result; + } else { + return MaterializeNode.create(asCondition(trueValue), t, f); + } + } } - static final class MaterializationKey { - final ValueNode trueValue; - final ValueNode falseValue; - public MaterializationKey(ValueNode trueValue, ValueNode falseValue) { + /** + * Replaces a usage of an {@link InstanceOfNode}. + */ + abstract static class InstanceOfUsageReplacer implements UsageReplacer { + protected final Instantiation instantiation; + protected final InstanceOfNode instanceOf; + protected final ValueNode trueValue; + protected final ValueNode falseValue; + + public InstanceOfUsageReplacer(Instantiation instantiation, InstanceOfNode instanceOf, ValueNode trueValue, ValueNode falseValue) { + this.instantiation = instantiation; + this.instanceOf = instanceOf; this.trueValue = trueValue; this.falseValue = falseValue; } - @Override - public int hashCode() { - return trueValue.hashCode(); - } - - @Override - public boolean equals(Object obj) { - if (obj instanceof MaterializationKey) { - MaterializationKey mk = (MaterializationKey) obj; - return mk.trueValue == trueValue && mk.falseValue == falseValue; - } - return false; - } - - @Override - public String toString() { - return "[true=" + trueValue + ", false=" + falseValue + "]"; - } + /** + * Does the replacement based on a previously snippet instantiation. + */ + abstract void replaceUsingInstantiation(); } /** * Replaces an {@link IfNode} usage of an {@link InstanceOfNode}. */ - static final class IfUsageReplacer implements UsageReplacer { + static final class IfUsageReplacer extends InstanceOfUsageReplacer { private final boolean solitaryUsage; - private final InstanceOfNode instanceOf; private final IfNode usage; - private final Map materializations; - private final MaterializationKey key; private final boolean sameBlock; - private IfUsageReplacer(Map materializations, MaterializationKey key, InstanceOfNode instanceOf, IfNode usage, LoweringTool tool) { - this.materializations = materializations; - this.key = key; + private IfUsageReplacer(Instantiation instantiation, ValueNode trueValue, ValueNode falseValue, InstanceOfNode instanceOf, IfNode usage, boolean solitaryUsage, LoweringTool tool) { + super(instantiation, instanceOf, trueValue, falseValue); this.sameBlock = tool.getBlockFor(usage) == tool.getBlockFor(instanceOf); - this.solitaryUsage = materializations == null; - this.instanceOf = instanceOf; + this.solitaryUsage = solitaryUsage; this.usage = usage; } @Override + void replaceUsingInstantiation() { + usage.replaceFirstInput(instanceOf, instantiation.asCondition(trueValue)); + } + + @Override public void replace(ValueNode oldNode, ValueNode newNode) { assert newNode instanceof PhiNode; assert oldNode == instanceOf; @@ -315,12 +346,8 @@ removeIntermediateMaterialization(newNode); } else { newNode.inferStamp(); - Materialization m = new Materialization((PhiNode) newNode); - if (materializations != null) { - Materialization oldValue = materializations.put(key, m); - assert oldValue == null; - } - usage.replaceFirstInput(oldNode, m.condition(key)); + instantiation.initialize((PhiNode) newNode, trueValue, falseValue); + usage.replaceFirstInput(oldNode, instantiation.asCondition(trueValue)); } } @@ -343,10 +370,10 @@ int endIndex = 0; for (EndNode end : mergePredecessors) { ValueNode endValue = phi.valueAt(endIndex++); - if (endValue == key.trueValue) { + if (endValue == trueValue) { trueEnds.add(end); } else { - assert endValue == key.falseValue; + assert endValue == falseValue; falseEnds.add(end); } } @@ -387,18 +414,22 @@ /** * Replaces a {@link ConditionalNode} usage of an {@link InstanceOfNode}. */ - static final class ConditionalUsageReplacer implements UsageReplacer { + static final class ConditionalUsageReplacer extends InstanceOfUsageReplacer { - private final InstanceOfNode instanceOf; private final ConditionalNode usage; - private final Map materializations; - private final MaterializationKey key; + + private ConditionalUsageReplacer(Instantiation instantiation, ValueNode trueValue, ValueNode falseValue, InstanceOfNode instanceOf, ConditionalNode usage) { + super(instantiation, instanceOf, trueValue, falseValue); + this.usage = usage; + } - private ConditionalUsageReplacer(Map materializations, MaterializationKey key, InstanceOfNode instanceOf, ConditionalNode usage) { - this.materializations = materializations; - this.key = key; - this.instanceOf = instanceOf; - this.usage = usage; + @Override + void replaceUsingInstantiation() { + ValueNode newValue = instantiation.asMaterialization(trueValue, falseValue); + usage.replaceAtUsages(newValue); + usage.clearInputs(); + assert usage.usages().isEmpty(); + GraphUtil.killWithUnusedFloatingInputs(usage); } @Override @@ -406,11 +437,7 @@ assert newNode instanceof PhiNode; assert oldNode == instanceOf; newNode.inferStamp(); - if (materializations != null) { - Materialization m = new Materialization((PhiNode) newNode); - Materialization oldValue = materializations.put(key, m); - assert oldValue == null; - } + instantiation.initialize((PhiNode) newNode, trueValue, falseValue); usage.replaceAtUsages(newNode); usage.clearInputs(); assert usage.usages().isEmpty(); diff -r 8e9ffa7c714b -r 370674104c1c graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/MaterializeNode.java --- a/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/MaterializeNode.java Thu Nov 01 17:33:48 2012 +0100 +++ b/graal/com.oracle.graal.nodes/src/com/oracle/graal/nodes/MaterializeNode.java Fri Nov 02 10:21:38 2012 +0100 @@ -27,7 +27,7 @@ public final class MaterializeNode extends ConditionalNode { - private static CompareNode createCompareNode(Condition condition, ValueNode x, ValueNode y) { + public static CompareNode createCompareNode(Condition condition, ValueNode x, ValueNode y) { assert x.kind() == y.kind(); assert condition.isCanonical() : "condition is not canonical: " + condition;