From 817bce7658918b7a70c17b70aa5e6a46b1ae8b3d Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Tue, 24 Feb 2015 13:35:38 +0000 Subject: [PATCH] Fix inlining in the presence of multiple returns. One return could actually return a phi, so doing a phi check for knowing if the result phi was already created was bogus. Bug: 19454010 Change-Id: Iee703a2d1071ae263092354465eda368e5d6770d --- compiler/optimizing/nodes.cc | 38 +++++++++++--------- test/452-multiple-returns2/expected.txt | 0 test/452-multiple-returns2/info.txt | 2 ++ .../smali/MultipleReturns.smali | 40 ++++++++++++++++++++++ test/452-multiple-returns2/src/Main.java | 32 +++++++++++++++++ 5 files changed, 95 insertions(+), 17 deletions(-) create mode 100644 test/452-multiple-returns2/expected.txt create mode 100644 test/452-multiple-returns2/info.txt create mode 100644 test/452-multiple-returns2/smali/MultipleReturns.smali create mode 100644 test/452-multiple-returns2/src/Main.java diff --git a/compiler/optimizing/nodes.cc b/compiler/optimizing/nodes.cc index 93787b8bf..e51bbc330 100644 --- a/compiler/optimizing/nodes.cc +++ b/compiler/optimizing/nodes.cc @@ -893,28 +893,32 @@ void HGraph::InlineInto(HGraph* outer_graph, HInvoke* invoke) { exit_block_->ReplaceWith(to); // Update all predecessors of the exit block (now the `to` block) - // to not `HReturn` but `HGoto` instead. Also collect the return - // values if any, and potentially make it a phi if there are multiple - // predecessors. + // to not `HReturn` but `HGoto` instead. HInstruction* return_value = nullptr; - for (size_t i = 0, e = to->GetPredecessors().Size(); i < e; ++i) { - HBasicBlock* predecessor = to->GetPredecessors().Get(i); + bool returns_void = to->GetPredecessors().Get(0)->GetLastInstruction()->IsReturnVoid(); + if (to->GetPredecessors().Size() == 1) { + HBasicBlock* predecessor = to->GetPredecessors().Get(0); HInstruction* last = predecessor->GetLastInstruction(); - if (!last->IsReturnVoid()) { - if (return_value != nullptr) { - if (!return_value->IsPhi()) { - HPhi* phi = new (allocator) HPhi(allocator, kNoRegNumber, 0, invoke->GetType()); - to->AddPhi(phi); - phi->AddInput(return_value); - return_value = phi; - } - return_value->AsPhi()->AddInput(last->InputAt(0)); - } else { - return_value = last->InputAt(0); - } + if (!returns_void) { + return_value = last->InputAt(0); } predecessor->AddInstruction(new (allocator) HGoto()); predecessor->RemoveInstruction(last); + } else { + if (!returns_void) { + // There will be multiple returns. + return_value = new (allocator) HPhi(allocator, kNoRegNumber, 0, invoke->GetType()); + to->AddPhi(return_value->AsPhi()); + } + for (size_t i = 0, e = to->GetPredecessors().Size(); i < e; ++i) { + HBasicBlock* predecessor = to->GetPredecessors().Get(i); + HInstruction* last = predecessor->GetLastInstruction(); + if (!returns_void) { + return_value->AsPhi()->AddInput(last->InputAt(0)); + } + predecessor->AddInstruction(new (allocator) HGoto()); + predecessor->RemoveInstruction(last); + } } if (return_value != nullptr) { diff --git a/test/452-multiple-returns2/expected.txt b/test/452-multiple-returns2/expected.txt new file mode 100644 index 000000000..e69de29bb diff --git a/test/452-multiple-returns2/info.txt b/test/452-multiple-returns2/info.txt new file mode 100644 index 000000000..cdd354bbd --- /dev/null +++ b/test/452-multiple-returns2/info.txt @@ -0,0 +1,2 @@ +Tests inlining of a pattern not generated by DX: multiple +returns in a single method. diff --git a/test/452-multiple-returns2/smali/MultipleReturns.smali b/test/452-multiple-returns2/smali/MultipleReturns.smali new file mode 100644 index 000000000..ff2b9b0ca --- /dev/null +++ b/test/452-multiple-returns2/smali/MultipleReturns.smali @@ -0,0 +1,40 @@ +# 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 LMultipleReturns; + +.super Ljava/lang/Object; + +.method public static caller()I + .registers 1 + invoke-static {}, LMultipleReturns;->$opt$CalleeReturnInt()I + move-result v0 + return v0 +.end method + +.method public static $opt$CalleeReturnInt()I + .registers 2 + const/4 v0, 0x0 + const/4 v1, 0x1 + if-eq v1, v0, :else + if-eq v1, v0, :else2 + const/4 v0, 0x4 + :else2 + return v0 + :else + if-eq v1, v0, :else3 + const/4 v1, 0x3 + :else3 + return v1 +.end method diff --git a/test/452-multiple-returns2/src/Main.java b/test/452-multiple-returns2/src/Main.java new file mode 100644 index 000000000..8904c45a3 --- /dev/null +++ b/test/452-multiple-returns2/src/Main.java @@ -0,0 +1,32 @@ +/* + * 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("MultipleReturns"); + Method m = c.getMethod("caller"); + int result = (Integer)m.invoke(null); + if (result != 4) { + throw new Error("Expected 4, got " + result); + } + } +} -- 2.11.0