Fix ARM code generator for Select.
Handle the case the output aliases with an input of an "emit-at-use-site"
condition.
Test: 684-select-condition
bug: 116169970
(cherry picked from commit 7b05c5f99a6caa9c05c6b79c2519ea7df159570c)
Change-Id: I2e43dd63112289d040e390d38331f279d58de51f
diff --git a/compiler/optimizing/code_generator_arm_vixl.cc b/compiler/optimizing/code_generator_arm_vixl.cc
index 2f495fc..2452139 100644
--- a/compiler/optimizing/code_generator_arm_vixl.cc
+++ b/compiler/optimizing/code_generator_arm_vixl.cc
@@ -3078,6 +3078,18 @@
const Location first = locations->InAt(0);
const Location out = locations->Out();
const Location second = locations->InAt(1);
+
+ // In the unlucky case the output of this instruction overlaps
+ // with an input of an "emitted-at-use-site" condition, and
+ // the output of this instruction is not one of its inputs, we'll
+ // need to fallback to branches instead of conditional ARM instructions.
+ bool output_overlaps_with_condition_inputs =
+ !IsBooleanValueOrMaterializedCondition(condition) &&
+ !out.Equals(first) &&
+ !out.Equals(second) &&
+ (condition->GetLocations()->InAt(0).Equals(out) ||
+ condition->GetLocations()->InAt(1).Equals(out));
+ DCHECK(!output_overlaps_with_condition_inputs || condition->IsCondition());
Location src;
if (condition->IsIntConstant()) {
@@ -3091,7 +3103,7 @@
return;
}
- if (!DataType::IsFloatingPointType(type)) {
+ if (!DataType::IsFloatingPointType(type) && !output_overlaps_with_condition_inputs) {
bool invert = false;
if (out.Equals(second)) {
@@ -3163,6 +3175,7 @@
vixl32::Label* false_target = nullptr;
vixl32::Label* true_target = nullptr;
vixl32::Label select_end;
+ vixl32::Label other_case;
vixl32::Label* const target = codegen_->GetFinalLabel(select, &select_end);
if (out.Equals(second)) {
@@ -3173,12 +3186,21 @@
src = second;
if (!out.Equals(first)) {
- codegen_->MoveLocation(out, first, type);
+ if (output_overlaps_with_condition_inputs) {
+ false_target = &other_case;
+ } else {
+ codegen_->MoveLocation(out, first, type);
+ }
}
}
GenerateTestAndBranch(select, 2, true_target, false_target, /* far_target */ false);
codegen_->MoveLocation(out, src, type);
+ if (output_overlaps_with_condition_inputs) {
+ __ B(target);
+ __ Bind(&other_case);
+ codegen_->MoveLocation(out, first, type);
+ }
if (select_end.IsReferenced()) {
__ Bind(&select_end);
@@ -3277,31 +3299,16 @@
void LocationsBuilderARMVIXL::HandleCondition(HCondition* cond) {
LocationSummary* locations =
new (GetGraph()->GetAllocator()) LocationSummary(cond, LocationSummary::kNoCall);
- // Handle the long/FP comparisons made in instruction simplification.
- switch (cond->InputAt(0)->GetType()) {
- case DataType::Type::kInt64:
- locations->SetInAt(0, Location::RequiresRegister());
- locations->SetInAt(1, Location::RegisterOrConstant(cond->InputAt(1)));
- if (!cond->IsEmittedAtUseSite()) {
- locations->SetOut(Location::RequiresRegister(), Location::kNoOutputOverlap);
- }
- break;
-
- case DataType::Type::kFloat32:
- case DataType::Type::kFloat64:
- locations->SetInAt(0, Location::RequiresFpuRegister());
- locations->SetInAt(1, ArithmeticZeroOrFpuRegister(cond->InputAt(1)));
- if (!cond->IsEmittedAtUseSite()) {
- locations->SetOut(Location::RequiresRegister(), Location::kNoOutputOverlap);
- }
- break;
-
- default:
- locations->SetInAt(0, Location::RequiresRegister());
- locations->SetInAt(1, Location::RegisterOrConstant(cond->InputAt(1)));
- if (!cond->IsEmittedAtUseSite()) {
- locations->SetOut(Location::RequiresRegister(), Location::kNoOutputOverlap);
- }
+ const DataType::Type type = cond->InputAt(0)->GetType();
+ if (DataType::IsFloatingPointType(type)) {
+ locations->SetInAt(0, Location::RequiresFpuRegister());
+ locations->SetInAt(1, ArithmeticZeroOrFpuRegister(cond->InputAt(1)));
+ } else {
+ locations->SetInAt(0, Location::RequiresRegister());
+ locations->SetInAt(1, Location::RegisterOrConstant(cond->InputAt(1)));
+ }
+ if (!cond->IsEmittedAtUseSite()) {
+ locations->SetOut(Location::RequiresRegister(), Location::kNoOutputOverlap);
}
}
diff --git a/test/684-select-condition/expected.txt b/test/684-select-condition/expected.txt
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/test/684-select-condition/expected.txt
diff --git a/test/684-select-condition/info.txt b/test/684-select-condition/info.txt
new file mode 100644
index 0000000..f9d4acd
--- /dev/null
+++ b/test/684-select-condition/info.txt
@@ -0,0 +1 @@
+Regression test for a bug in ARM's code generator for HSelect.
diff --git a/test/684-select-condition/src/Main.java b/test/684-select-condition/src/Main.java
new file mode 100644
index 0000000..196ff1a
--- /dev/null
+++ b/test/684-select-condition/src/Main.java
@@ -0,0 +1,83 @@
+/*
+ * Copyright (C) 2018 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.
+ */
+
+public class Main {
+
+ public static void main(String args[]) {
+ doFloatingPointTest("1", "1.0");
+ doFloatingPointTest("4", "2.0");
+ checkValue(String.valueOf(doIntegerTest1(4)), "0");
+ checkValue(String.valueOf(doIntegerTest2(4)), "4");
+
+ // Another variant of the floating point test, but less brittle.
+ staticField = 1;
+ checkValue(String.valueOf($noinline$test()), "1.0");
+ staticField = 4;
+ checkValue(String.valueOf($noinline$test()), "2.0");
+ }
+
+ // This code is a reduced version of the original reproducer. The arm
+ // code generator used to generate wrong code for it. Note that this
+ // test is very brittle and a simple change in it could cause the compiler
+ // to not trip.
+ public static void doFloatingPointTest(String s, String expected) {
+ float a = (float)Integer.valueOf(s);
+ a = a < 2.0f ? a : 2.0f;
+ checkValue("" + a, expected);
+ }
+
+ // The compiler used to trip on the two following methods. The test there
+ // is very brittle and requires not running constant folding after
+ // load/store elimination.
+ public static int doIntegerTest1(int param) {
+ Main main = new Main();
+ main.field = 0;
+ return (main.field == 0) ? 0 : param;
+ }
+
+ public static int doIntegerTest2(int param) {
+ Main main = new Main();
+ main.field = 0;
+ return (main.field != 0) ? 0 : param;
+ }
+
+ public static void checkValue(String actual, String expected) {
+ if (!expected.equals(actual)) {
+ throw new Error("Expected " + expected + ", got " + actual);
+ }
+ }
+
+ static void $noinline$nothing() {}
+ static int $noinline$getField() { return staticField; }
+
+ static float $noinline$test() {
+ // The 2.0f shall be materialized for GreaterThanOrEqual at the beginning of the method;
+ // since the following call clobbers caller-saves, it is allocated to s16.
+ // r0(field) = InvokeStaticOrDirect[]
+ int one = $noinline$getField();
+ // s0(a_1) = TypeConversion[r0(one)]
+ float a = (float)one;
+ // s16(a_2) = Select[s0(a_1), C(2.0f), GreaterThanOrEqual[s0(a_1), s16(2.0f)]]
+ a = a < 2.0f ? a : 2.0f;
+ // The following call is added to clobber caller-saves, forcing the output of the Select
+ // to be allocated to s16.
+ $noinline$nothing();
+ return a;
+ }
+
+ int field;
+ static int staticField;
+}
