diff -r 13588c901957 -r 9cf78a70fa4f src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.phases.common/src/org/graalvm/compiler/phases/common/ConditionalEliminationPhase.java --- a/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.phases.common/src/org/graalvm/compiler/phases/common/ConditionalEliminationPhase.java Thu Oct 17 20:27:44 2019 +0100 +++ b/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.phases.common/src/org/graalvm/compiler/phases/common/ConditionalEliminationPhase.java Thu Oct 17 20:53:35 2019 +0100 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2019, 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 @@ -56,6 +56,7 @@ import org.graalvm.compiler.nodes.AbstractMergeNode; import org.graalvm.compiler.nodes.BinaryOpLogicNode; import org.graalvm.compiler.nodes.ConditionAnchorNode; +import org.graalvm.compiler.nodes.DeoptimizeNode; import org.graalvm.compiler.nodes.DeoptimizingGuard; import org.graalvm.compiler.nodes.EndNode; import org.graalvm.compiler.nodes.FixedGuardNode; @@ -90,20 +91,20 @@ import org.graalvm.compiler.nodes.extended.ValueAnchorNode; import org.graalvm.compiler.nodes.java.InstanceOfNode; import org.graalvm.compiler.nodes.java.TypeSwitchNode; +import org.graalvm.compiler.nodes.spi.CoreProviders; import org.graalvm.compiler.nodes.spi.NodeWithState; import org.graalvm.compiler.nodes.spi.StampInverter; import org.graalvm.compiler.nodes.util.GraphUtil; import org.graalvm.compiler.phases.BasePhase; import org.graalvm.compiler.phases.schedule.SchedulePhase; import org.graalvm.compiler.phases.schedule.SchedulePhase.SchedulingStrategy; -import org.graalvm.compiler.phases.tiers.PhaseContext; import jdk.vm.ci.meta.DeoptimizationAction; import jdk.vm.ci.meta.JavaConstant; import jdk.vm.ci.meta.SpeculationLog.Speculation; import jdk.vm.ci.meta.TriState; -public class ConditionalEliminationPhase extends BasePhase { +public class ConditionalEliminationPhase extends BasePhase { private static final CounterKey counterStampsRegistered = DebugContext.counter("StampsRegistered"); private static final CounterKey counterStampsFound = DebugContext.counter("StampsFound"); @@ -123,7 +124,7 @@ @Override @SuppressWarnings("try") - protected void run(StructuredGraph graph, PhaseContext context) { + protected void run(StructuredGraph graph, CoreProviders context) { try (DebugContext.Scope s = graph.getDebug().scope("DominatorConditionalElimination")) { BlockMap> blockToNodes = null; NodeMap nodeToBlock = null; @@ -154,7 +155,7 @@ } protected ControlFlowGraph.RecursiveVisitor createVisitor(StructuredGraph graph, @SuppressWarnings("unused") ControlFlowGraph cfg, BlockMap> blockToNodes, - NodeMap nodeToBlock, PhaseContext context) { + NodeMap nodeToBlock, CoreProviders context) { return new Instance(graph, blockToNodes, nodeToBlock, context); } @@ -174,14 +175,9 @@ AbstractBeginNode beginNode = b.getBeginNode(); if (beginNode instanceof AbstractMergeNode && anchorBlock != b) { AbstractMergeNode mergeNode = (AbstractMergeNode) beginNode; - for (GuardNode guard : mergeNode.guards().snapshot()) { - try (DebugCloseable closeable = guard.withNodeSourcePosition()) { - GuardNode newlyCreatedGuard = new GuardNode(guard.getCondition(), anchorBlock.getBeginNode(), guard.getReason(), guard.getAction(), guard.isNegated(), guard.getSpeculation(), - guard.getNoDeoptSuccessorPosition()); - GuardNode newGuard = mergeNode.graph().unique(newlyCreatedGuard); - guard.replaceAndDelete(newGuard); - } - } + mergeNode.replaceAtUsages(InputType.Anchor, anchorBlock.getBeginNode()); + mergeNode.replaceAtUsages(InputType.Guard, anchorBlock.getBeginNode()); + assert mergeNode.anchored().isEmpty(); } FixedNode endNode = b.getEndNode(); @@ -302,7 +298,7 @@ */ private Deque pendingTests; - public Instance(StructuredGraph graph, BlockMap> blockToNodes, NodeMap nodeToBlock, PhaseContext context) { + public Instance(StructuredGraph graph, BlockMap> blockToNodes, NodeMap nodeToBlock, CoreProviders context) { this.graph = graph; this.debug = graph.getDebug(); this.blockToNodes = blockToNodes; @@ -313,7 +309,7 @@ this.conditions = new ArrayDeque<>(); tool = GraphUtil.getDefaultSimplifier(context.getMetaAccess(), context.getConstantReflection(), context.getConstantFieldProvider(), false, graph.getAssumptions(), graph.getOptions(), context.getLowerer()); - mergeMaps = EconomicMap.create(); + mergeMaps = EconomicMap.create(Equivalence.IDENTITY); } protected void processConditionAnchor(ConditionAnchorNode node) { @@ -335,19 +331,26 @@ if (!tryProveGuardCondition(node, node.getCondition(), (guard, result, guardedValueStamp, newInput) -> { if (result != node.isNegated()) { node.replaceAndDelete(guard.asNode()); + if (guard instanceof DeoptimizingGuard && !((DeoptimizingGuard) guard).isNegated()) { + rebuildPiNodes((DeoptimizingGuard) guard); + } } else { - /* - * Don't kill this branch immediately because `killCFG` can have complex - * implications in the presence of loops: it might replace or delete nodes in - * other branches or even above the kill point. Instead of killing immediately, - * just leave the graph in a state that is easy to simplify by a subsequent - * canonicalizer phase. - */ - FixedGuardNode deopt = new FixedGuardNode(LogicConstantNode.forBoolean(result, node.graph()), node.getReason(), node.getAction(), node.getSpeculation(), node.isNegated(), - node.getNodeSourcePosition()); AbstractBeginNode beginNode = (AbstractBeginNode) node.getAnchor(); - graph.addAfterFixed(beginNode, node.graph().add(deopt)); + if (beginNode.next() instanceof DeoptimizeNode) { + // This branch is already dead. + } else { + /* + * Don't kill this branch immediately because `killCFG` can have complex + * implications in the presence of loops: it might replace or delete nodes + * in other branches or even above the kill point. Instead of killing + * immediately, just leave the graph in a state that is easy to simplify by + * a subsequent canonicalizer phase. + */ + FixedGuardNode deopt = new FixedGuardNode(LogicConstantNode.forBoolean(result, node.graph()), node.getReason(), node.getAction(), node.getSpeculation(), node.isNegated(), + node.getNodeSourcePosition()); + graph.addAfterFixed(beginNode, node.graph().add(deopt)); + } } return true; })) { @@ -361,41 +364,14 @@ node.replaceAtUsages(guard.asNode()); GraphUtil.unlinkFixedNode(node); GraphUtil.killWithUnusedFloatingInputs(node); + if (guard instanceof DeoptimizingGuard && !((DeoptimizingGuard) guard).isNegated()) { + rebuildPiNodes((DeoptimizingGuard) guard); + } } else { node.setCondition(LogicConstantNode.forBoolean(result, node.graph()), node.isNegated()); // Don't kill this branch immediately, see `processGuard`. } - if (guard instanceof DeoptimizingGuard && !node.isNegated() && !((DeoptimizingGuard) guard).isNegated()) { - LogicNode newCondition = ((DeoptimizingGuard) guard.asNode()).getCondition(); - if (newCondition instanceof InstanceOfNode) { - InstanceOfNode inst = (InstanceOfNode) newCondition; - ValueNode originalValue = GraphUtil.skipPi(inst.getValue()); - PiNode pi = null; - // Ensure that any Pi that's weaker than what the instanceof proves is - // replaced by one derived from the instanceof itself. - for (PiNode existing : guard.asNode().usages().filter(PiNode.class).snapshot()) { - if (!existing.isAlive()) { - continue; - } - if (originalValue != GraphUtil.skipPi(existing.object())) { - // Somehow these are unrelated values so leave it alone - continue; - } - // If the pi has a weaker stamp or the same stamp but a different input - // then replace it. - boolean strongerStamp = !existing.piStamp().join(inst.getCheckedStamp()).equals(inst.getCheckedStamp()); - boolean differentStamp = !existing.piStamp().equals(inst.getCheckedStamp()); - boolean differentObject = existing.object() != inst.getValue(); - if (!strongerStamp && (differentStamp || differentObject)) { - if (pi == null) { - pi = graph.unique(new PiNode(inst.getValue(), inst.getCheckedStamp(), (ValueNode) guard)); - } - existing.replaceAndDelete(pi); - } - } - } - } debug.log("Kill fixed guard %s", node); return true; })) { @@ -403,6 +379,59 @@ } } + private void rebuildPiNodes(DeoptimizingGuard guard) { + LogicNode newCondition = guard.getCondition(); + if (newCondition instanceof InstanceOfNode) { + InstanceOfNode inst = (InstanceOfNode) newCondition; + ValueNode originalValue = GraphUtil.skipPi(inst.getValue()); + PiNode pi = null; + // Ensure that any Pi that's weaker than what the instanceof proves is + // replaced by one derived from the instanceof itself. + for (PiNode existing : guard.asNode().usages().filter(PiNode.class).snapshot()) { + if (!existing.isAlive()) { + continue; + } + if (originalValue != GraphUtil.skipPi(existing.object())) { + // Somehow these are unrelated values so leave it alone + continue; + } + // If the pi has a weaker stamp or the same stamp but a different input + // then replace it. + boolean strongerStamp = !existing.piStamp().join(inst.getCheckedStamp()).equals(inst.getCheckedStamp()); + boolean differentCheckedStamp = !existing.piStamp().equals(inst.getCheckedStamp()); + boolean differentObject = existing.object() != inst.getValue(); + if (!strongerStamp && (differentCheckedStamp || differentObject)) { + if (pi == null) { + pi = graph.unique(new PiNode(inst.getValue(), inst.getCheckedStamp(), (ValueNode) guard)); + } + if (!pi.stamp(NodeView.DEFAULT).join(existing.stamp(NodeView.DEFAULT)).equals(pi.stamp(NodeView.DEFAULT))) { + /* + * With a code sequence like null check, type check, null check of type + * checked value, CE will use the first null check to prove the second + * null check so the graph ends up a Pi guarded by the first null check + * but consuming the output Pi from the type check check. In this case + * we should still canonicalize the checked stamp for consistency. + */ + if (differentCheckedStamp) { + PiNode alternatePi = graph.unique(new PiNode(existing.object(), inst.getCheckedStamp(), (ValueNode) guard)); + /* + * If the resulting stamp is as good or better then do the + * replacement. However when interface types are involved it's + * possible that improving the checked stamp merges types which + * appear unrelated so there's we must skip the replacement. + */ + if (alternatePi.stamp(NodeView.DEFAULT).join(existing.stamp(NodeView.DEFAULT)).equals(alternatePi.stamp(NodeView.DEFAULT))) { + existing.replaceAndDelete(alternatePi); + } + } + continue; + } + existing.replaceAndDelete(pi); + } + } + } + } + protected void processIf(IfNode node) { tryProveCondition(node.condition(), (guard, result, guardedValueStamp, newInput) -> { node.setCondition(LogicConstantNode.forBoolean(result, node.graph())); @@ -587,7 +616,7 @@ Stamp newStamp = infoElement.getStamp(); if (phi.stamp(NodeView.DEFAULT).tryImproveWith(newStamp) != null) { if (mergeMap == null) { - mergeMap = EconomicMap.create(); + mergeMap = EconomicMap.create(Equivalence.IDENTITY); mergeMaps.put(merge, mergeMap); }