From b59dba05697b4ac6c86cb4f45c9222c9c6ad852b Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Wed, 11 Mar 2015 18:13:21 +0000 Subject: [PATCH] Fix a bug in the SSA builder. 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 | 20 ++++++++------ test/459-dead-phi/expected.txt | 1 + test/459-dead-phi/info.txt | 1 + test/459-dead-phi/smali/EquivalentPhi.smali | 41 +++++++++++++++++++++++++++++ test/459-dead-phi/src/Main.java | 29 ++++++++++++++++++++ 5 files changed, 84 insertions(+), 8 deletions(-) create mode 100644 test/459-dead-phi/expected.txt create mode 100644 test/459-dead-phi/info.txt create mode 100644 test/459-dead-phi/smali/EquivalentPhi.smali create mode 100644 test/459-dead-phi/src/Main.java diff --git a/compiler/optimizing/ssa_builder.cc b/compiler/optimizing/ssa_builder.cc index 1a8e78436..ba11e90d9 100644 --- a/compiler/optimizing/ssa_builder.cc +++ b/compiler/optimizing/ssa_builder.cc @@ -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 index 000000000..ba66466c2 --- /dev/null +++ b/test/459-dead-phi/expected.txt @@ -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 index 000000000..3f82ecb66 --- /dev/null +++ b/test/459-dead-phi/info.txt @@ -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 index 000000000..4fa88a9e8 --- /dev/null +++ b/test/459-dead-phi/smali/EquivalentPhi.smali @@ -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 index 000000000..0ecc0bdcc --- /dev/null +++ b/test/459-dead-phi/src/Main.java @@ -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 })); + } +} -- 2.11.0