# HG changeset patch # User Doug Simon # Date 1372194352 -7200 # Node ID 1194e94f9c16f8a8c592094ac31ead45d56a1183 # Parent 74cbc5d6e38ff9229dda76e8135069bc0b9c5131 fixed bug in ConditionalEliminationPhase (GRAAL-346) diff -r 74cbc5d6e38f -r 1194e94f9c16 graal/com.oracle.graal.compiler.test/src/com/oracle/graal/compiler/test/ConditionalEliminationTest.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/graal/com.oracle.graal.compiler.test/src/com/oracle/graal/compiler/test/ConditionalEliminationTest.java Tue Jun 25 23:05:52 2013 +0200 @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2011, 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 + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ +package com.oracle.graal.compiler.test; + +import org.junit.*; + +import com.oracle.graal.phases.common.*; + +/** + * Collection of tests for {@link ConditionalEliminationPhase} including those that triggered bugs + * in this phase. + */ +public class ConditionalEliminationTest extends GraalCompilerTest { + + static class Entry { + + final String name; + + public Entry(String name) { + this.name = name; + } + } + + static class EntryWithNext extends Entry { + + public EntryWithNext(String name, Entry next) { + super(name); + this.next = next; + } + + final Entry next; + } + + public static Entry search(Entry start, String name, Entry alternative) { + Entry current = start; + do { + while (current instanceof EntryWithNext) { + if (name != null && current.name == name) { + current = null; + } else { + Entry next = ((EntryWithNext) current).next; + current = next; + } + } + + if (current != null) { + if (current.name.equals(name)) { + return current; + } + } + if (current == alternative) { + return null; + } + current = alternative; + + } while (true); + } + + /** + * This test presents a code pattern that triggered a bug where a (non-eliminated) checkcast + * caused an enclosing instanceof (for the same object and target type) to be incorrectly + * eliminated. + */ + @Test + public void testReanchoringIssue() { + Entry end = new Entry("end"); + EntryWithNext e1 = new EntryWithNext("e1", end); + EntryWithNext e2 = new EntryWithNext("e2", e1); + + test("search", e2, "e3", new Entry("e4")); + } +} diff -r 74cbc5d6e38f -r 1194e94f9c16 graal/com.oracle.graal.phases.common/src/com/oracle/graal/phases/common/ConditionalEliminationPhase.java --- a/graal/com.oracle.graal.phases.common/src/com/oracle/graal/phases/common/ConditionalEliminationPhase.java Tue Jun 25 23:05:01 2013 +0200 +++ b/graal/com.oracle.graal.phases.common/src/com/oracle/graal/phases/common/ConditionalEliminationPhase.java Tue Jun 25 23:05:52 2013 +0200 @@ -27,6 +27,7 @@ import com.oracle.graal.api.meta.*; import com.oracle.graal.debug.*; import com.oracle.graal.graph.*; +import com.oracle.graal.graph.iterators.*; import com.oracle.graal.nodes.*; import com.oracle.graal.nodes.PhiNode.PhiType; import com.oracle.graal.nodes.calc.*; @@ -493,12 +494,52 @@ } else if (node instanceof IfNode) { IfNode ifNode = (IfNode) node; LogicNode compare = ifNode.condition(); - LogicNode replacement = evaluateCondition(compare, trueConstant, falseConstant); + + LogicNode replacement = null; + ValueNode replacementAnchor = null; + AbstractBeginNode survivingSuccessor = null; + if (state.trueConditions.containsKey(compare)) { + replacement = trueConstant; + replacementAnchor = state.trueConditions.get(compare); + survivingSuccessor = ifNode.trueSuccessor(); + } else if (state.falseConditions.containsKey(compare)) { + replacement = falseConstant; + replacementAnchor = state.falseConditions.get(compare); + survivingSuccessor = ifNode.falseSuccessor(); + } else { + replacement = evaluateCondition(compare, trueConstant, falseConstant); + if (replacement != null) { + if (replacement == trueConstant) { + survivingSuccessor = ifNode.trueSuccessor(); + } else { + assert replacement == falseConstant; + survivingSuccessor = ifNode.falseSuccessor(); + } + } + } if (replacement != null) { - ifNode.setCondition(replacement); - if (compare.usages().isEmpty()) { - GraphUtil.killWithUnusedFloatingInputs(compare); + NodeIterable anchored = survivingSuccessor.anchored(); + if (!anchored.isEmpty() && replacementAnchor == null) { + // Cannot simplify an IfNode unless we have a new anchor point + // for any nodes currently anchored to the surviving branch + } else { + if (!anchored.isEmpty()) { + // Ideally we'd simply want to re-anchor to replacementAnchor. However, + // this can cause guards currently anchored to the surviving branch + // to float too high in the graph. So, we insert a new anchor between + // the guards and replacementAnchor. + ValueAnchorNode valueAnchor = graph.add(new ValueAnchorNode()); + for (Node a : anchored.snapshot()) { + a.replaceFirstInput(survivingSuccessor, valueAnchor); + } + valueAnchor.addAnchoredNode(replacementAnchor); + graph.addBeforeFixed(ifNode, valueAnchor); + } + ifNode.setCondition(replacement); + if (compare.usages().isEmpty()) { + GraphUtil.killWithUnusedFloatingInputs(compare); + } } } } else if (node instanceof AbstractEndNode) { @@ -522,5 +563,4 @@ } } } - }