changeset 10532:1194e94f9c16

fixed bug in ConditionalEliminationPhase (GRAAL-346)
author Doug Simon <doug.simon@oracle.com>
date Tue, 25 Jun 2013 23:05:52 +0200
parents 74cbc5d6e38f
children 2faa1e7ef4f3
files graal/com.oracle.graal.compiler.test/src/com/oracle/graal/compiler/test/ConditionalEliminationTest.java graal/com.oracle.graal.phases.common/src/com/oracle/graal/phases/common/ConditionalEliminationPhase.java
diffstat 2 files changed, 137 insertions(+), 5 deletions(-) [+]
line wrap: on
line diff
--- /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"));
+    }
+}
--- 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<Node> 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 @@
             }
         }
     }
-
 }