OSDN Git Service

Fix a bug in the SSA builder.
authorNicolas Geoffray <ngeoffray@google.com>
Wed, 11 Mar 2015 18:13:21 +0000 (18:13 +0000)
committerNicolas Geoffray <ngeoffray@google.com>
Wed, 11 Mar 2015 18:13:21 +0000 (18:13 +0000)
The build would leave behind phis with incompatible input types
(for example float and int). We need another dead phi run after
the type propagation to ensure all such phis are dead.

Change-Id: I6ef1da725c7d4a1ebaf6b52dd7eb0c7bacd261b2

compiler/optimizing/ssa_builder.cc
test/459-dead-phi/expected.txt [new file with mode: 0644]
test/459-dead-phi/info.txt [new file with mode: 0644]
test/459-dead-phi/smali/EquivalentPhi.smali [new file with mode: 0644]
test/459-dead-phi/src/Main.java [new file with mode: 0644]

index 1a8e784..ba11e90 100644 (file)
@@ -199,9 +199,9 @@ void SsaBuilder::BuildSsa() {
   // our code generator will complain if the inputs of a phi do not have the same
   // type. The marking allows the type propagation to know which phis it needs
   // to handle. We mark but do not eliminate: the elimination will be done in
-  // step 8).
-  SsaDeadPhiElimination dead_phis(GetGraph());
-  dead_phis.MarkDeadPhis();
+  // step 9).
+  SsaDeadPhiElimination dead_phis_for_type_propagation(GetGraph());
+  dead_phis_for_type_propagation.MarkDeadPhis();
 
   // 4) Propagate types of phis. At this point, phis are typed void in the general
   // case, or float/double/reference when we created an equivalent phi. So we
@@ -209,7 +209,11 @@ void SsaBuilder::BuildSsa() {
   PrimitiveTypePropagation type_propagation(GetGraph());
   type_propagation.Run();
 
-  // 5) Now that the graph is correclty typed, we can get rid of redundant phis.
+  // 5) Mark dead phis again. Steph 4) may have introduced new phis.
+  SsaDeadPhiElimination dead_phis(GetGraph());
+  dead_phis.MarkDeadPhis();
+
+  // 6) Now that the graph is correclty typed, we can get rid of redundant phis.
   // Note that we cannot do this phase before type propagation, otherwise
   // we could get rid of phi equivalents, whose presence is a requirement for the
   // type propagation phase. Note that this is to satisfy statement (a) of the
@@ -217,7 +221,7 @@ void SsaBuilder::BuildSsa() {
   SsaRedundantPhiElimination redundant_phi(GetGraph());
   redundant_phi.Run();
 
-  // 6) Make sure environments use the right phi "equivalent": a phi marked dead
+  // 7) Make sure environments use the right phi "equivalent": a phi marked dead
   // can have a phi equivalent that is not dead. We must therefore update
   // all environment uses of the dead phi to use its equivalent. Note that there
   // can be multiple phis for the same Dex register that are live (for example
@@ -244,7 +248,7 @@ void SsaBuilder::BuildSsa() {
     }
   }
 
-  // 7) Deal with phis to guarantee liveness of phis in case of a debuggable
+  // 8) Deal with phis to guarantee liveness of phis in case of a debuggable
   // application. This is for satisfying statement (c) of the SsaBuilder
   // (see ssa_builder.h).
   if (GetGraph()->IsDebuggable()) {
@@ -252,7 +256,7 @@ void SsaBuilder::BuildSsa() {
     dead_phi_handler.Run();
   }
 
-  // 8) Now that the right phis are used for the environments, and we
+  // 9) Now that the right phis are used for the environments, and we
   // have potentially revive dead phis in case of a debuggable application,
   // we can eliminate phis we do not need. Regardless of the debuggable status,
   // this phase is necessary for statement (b) of the SsaBuilder (see ssa_builder.h),
@@ -260,7 +264,7 @@ void SsaBuilder::BuildSsa() {
   // input types.
   dead_phis.EliminateDeadPhis();
 
-  // 9) Clear locals.
+  // 10) Clear locals.
   for (HInstructionIterator it(GetGraph()->GetEntryBlock()->GetInstructions());
        !it.Done();
        it.Advance()) {
diff --git a/test/459-dead-phi/expected.txt b/test/459-dead-phi/expected.txt
new file mode 100644 (file)
index 0000000..ba66466
--- /dev/null
@@ -0,0 +1 @@
+0.0
diff --git a/test/459-dead-phi/info.txt b/test/459-dead-phi/info.txt
new file mode 100644 (file)
index 0000000..3f82ecb
--- /dev/null
@@ -0,0 +1 @@
+Regression test for optimizing when it is building the SSA form.
diff --git a/test/459-dead-phi/smali/EquivalentPhi.smali b/test/459-dead-phi/smali/EquivalentPhi.smali
new file mode 100644 (file)
index 0000000..4fa88a9
--- /dev/null
@@ -0,0 +1,41 @@
+# Copyright (C) 2015 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+.class public LEquivalentPhi;
+
+.super Ljava/lang/Object;
+
+.method public static equivalentPhi([F)F
+   .registers 5
+   const/4 v0, 0x0
+   # aget is initally expected to be an int, but will
+   # rightly become a float after type propagation.
+   aget v1, p0, v0
+   move v2, v1
+   if-eq v0, v0, :else
+   move v2, v0
+   :else
+   # v2 will be a phi with (int, int) as input
+   move v3, v2
+   if-eq v0, v0, :else2
+   move v3, v0
+   # v3 will be a phi with (int, int) as input.
+   : else2
+   # This instruction will lead to creating a phi equivalent
+   # for v3 with float type, which in turn will lead to creating
+   # a phi equivalent for v2 of type float. We used to forget to
+   # delete the old phi, which ends up having incompatible input
+   # types.
+   return v3
+.end method
diff --git a/test/459-dead-phi/src/Main.java b/test/459-dead-phi/src/Main.java
new file mode 100644 (file)
index 0000000..0ecc0bd
--- /dev/null
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.lang.reflect.Method;
+
+public class Main {
+
+  // Workaround for b/18051191.
+  class InnerClass {}
+
+  public static void main(String[] args) throws Exception {
+    Class<?> c = Class.forName("EquivalentPhi");
+    Method m = c.getMethod("equivalentPhi", float[].class);
+    System.out.println(m.invoke(null, new float[] { 0.0f }));
+  }
+}