From 3bcc8ea079d867f26622defd0611d134a3b4ae49 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Fri, 28 Nov 2014 15:00:02 +0000 Subject: [PATCH] Don't use CanHoldArm in the code generator. CanHoldArm was ARM32 specific. Instead use a virtual Assembler::ShifterOperandCanHold that both thumb2 and arm32 implement. Change-Id: I33794a93caf02ee5d78d32a8471d9fd6fe4f0a00 --- compiler/optimizing/code_generator_arm.cc | 22 ++++++------- compiler/utils/arm/assembler_arm.cc | 30 ----------------- compiler/utils/arm/assembler_arm.h | 41 ++++++++--------------- compiler/utils/arm/assembler_arm32.cc | 51 ++++++++++++++++++++++------ compiler/utils/arm/assembler_arm32.h | 7 ++++ compiler/utils/arm/assembler_thumb2.cc | 55 +++++++++++++++++++++++++------ compiler/utils/arm/assembler_thumb2.h | 6 ++++ test/434-shifter-operand/expected.txt | 3 ++ test/434-shifter-operand/info.txt | 2 ++ test/434-shifter-operand/src/Main.java | 27 +++++++++++++++ 10 files changed, 156 insertions(+), 88 deletions(-) create mode 100644 test/434-shifter-operand/expected.txt create mode 100644 test/434-shifter-operand/info.txt create mode 100644 test/434-shifter-operand/src/Main.java diff --git a/compiler/optimizing/code_generator_arm.cc b/compiler/optimizing/code_generator_arm.cc index 7d6d82738..dc861144c 100644 --- a/compiler/optimizing/code_generator_arm.cc +++ b/compiler/optimizing/code_generator_arm.cc @@ -949,20 +949,20 @@ void InstructionCodeGeneratorARM::VisitIf(HIf* if_instr) { // Condition has not been materialized, use its inputs as the // comparison and its condition as the branch condition. LocationSummary* locations = cond->GetLocations(); + Register left = locations->InAt(0).AsRegister(); if (locations->InAt(1).IsRegister()) { - __ cmp(locations->InAt(0).AsRegister(), - ShifterOperand(locations->InAt(1).AsRegister())); + __ cmp(left, ShifterOperand(locations->InAt(1).AsRegister())); } else { DCHECK(locations->InAt(1).IsConstant()); int32_t value = locations->InAt(1).GetConstant()->AsIntConstant()->GetValue(); ShifterOperand operand; - if (ShifterOperand::CanHoldArm(value, &operand)) { - __ cmp(locations->InAt(0).AsRegister(), ShifterOperand(value)); + if (GetAssembler()->ShifterOperandCanHold(R0, left, CMP, value, &operand)) { + __ cmp(left, operand); } else { Register temp = IP; __ LoadImmediate(temp, value); - __ cmp(locations->InAt(0).AsRegister(), ShifterOperand(temp)); + __ cmp(left, ShifterOperand(temp)); } } __ b(codegen_->GetLabelOf(if_instr->IfTrueSuccessor()), @@ -988,21 +988,21 @@ void LocationsBuilderARM::VisitCondition(HCondition* comp) { void InstructionCodeGeneratorARM::VisitCondition(HCondition* comp) { if (!comp->NeedsMaterialization()) return; - LocationSummary* locations = comp->GetLocations(); + Register left = locations->InAt(0).AsRegister(); + if (locations->InAt(1).IsRegister()) { - __ cmp(locations->InAt(0).AsRegister(), - ShifterOperand(locations->InAt(1).AsRegister())); + __ cmp(left, ShifterOperand(locations->InAt(1).AsRegister())); } else { DCHECK(locations->InAt(1).IsConstant()); int32_t value = locations->InAt(1).GetConstant()->AsIntConstant()->GetValue(); ShifterOperand operand; - if (ShifterOperand::CanHoldArm(value, &operand)) { - __ cmp(locations->InAt(0).AsRegister(), ShifterOperand(value)); + if (GetAssembler()->ShifterOperandCanHold(R0, left, CMP, value, &operand)) { + __ cmp(left, operand); } else { Register temp = IP; __ LoadImmediate(temp, value); - __ cmp(locations->InAt(0).AsRegister(), ShifterOperand(temp)); + __ cmp(left, ShifterOperand(temp)); } } __ it(ARMCondition(comp->GetCondition()), kItElse); diff --git a/compiler/utils/arm/assembler_arm.cc b/compiler/utils/arm/assembler_arm.cc index 0f2859177..05287732c 100644 --- a/compiler/utils/arm/assembler_arm.cc +++ b/compiler/utils/arm/assembler_arm.cc @@ -165,36 +165,6 @@ uint32_t ShifterOperand::encodingThumb() const { return 0; } -bool ShifterOperand::CanHoldThumb(Register rd, Register rn, Opcode opcode, - uint32_t immediate, ShifterOperand* shifter_op) { - shifter_op->type_ = kImmediate; - shifter_op->immed_ = immediate; - shifter_op->is_shift_ = false; - shifter_op->is_rotate_ = false; - switch (opcode) { - case ADD: - case SUB: - if (rn == SP) { - if (rd == SP) { - return immediate < (1 << 9); // 9 bits allowed. - } else { - return immediate < (1 << 12); // 12 bits. - } - } - if (immediate < (1 << 12)) { // Less than (or equal to) 12 bits can always be done. - return true; - } - return ArmAssembler::ModifiedImmediate(immediate) != kInvalidModifiedImmediate; - - case MOV: - // TODO: Support less than or equal to 12bits. - return ArmAssembler::ModifiedImmediate(immediate) != kInvalidModifiedImmediate; - case MVN: - default: - return ArmAssembler::ModifiedImmediate(immediate) != kInvalidModifiedImmediate; - } -} - uint32_t Address::encodingArm() const { CHECK(IsAbsoluteUint(12, offset_)); uint32_t encoding; diff --git a/compiler/utils/arm/assembler_arm.h b/compiler/utils/arm/assembler_arm.h index d288b700e..c86ec4b3d 100644 --- a/compiler/utils/arm/assembler_arm.h +++ b/compiler/utils/arm/assembler_arm.h @@ -30,6 +30,9 @@ namespace art { namespace arm { +class Arm32Assembler; +class Thumb2Assembler; + class ShifterOperand { public: ShifterOperand() : type_(kUnknown), rm_(kNoRegister), rs_(kNoRegister), @@ -103,33 +106,6 @@ class ShifterOperand { kImmediate }; - static bool CanHoldArm(uint32_t immediate, ShifterOperand* shifter_op) { - // Avoid the more expensive test for frequent small immediate values. - if (immediate < (1 << kImmed8Bits)) { - shifter_op->type_ = kImmediate; - shifter_op->is_rotate_ = true; - shifter_op->rotate_ = 0; - shifter_op->immed_ = immediate; - return true; - } - // Note that immediate must be unsigned for the test to work correctly. - for (int rot = 0; rot < 16; rot++) { - uint32_t imm8 = (immediate << 2*rot) | (immediate >> (32 - 2*rot)); - if (imm8 < (1 << kImmed8Bits)) { - shifter_op->type_ = kImmediate; - shifter_op->is_rotate_ = true; - shifter_op->rotate_ = rot; - shifter_op->immed_ = imm8; - return true; - } - } - return false; - } - - static bool CanHoldThumb(Register rd, Register rn, Opcode opcode, - uint32_t immediate, ShifterOperand* shifter_op); - - private: Type type_; Register rm_; @@ -140,6 +116,9 @@ class ShifterOperand { uint32_t rotate_; uint32_t immed_; + friend class Arm32Assembler; + friend class Thumb2Assembler; + #ifdef SOURCE_ASSEMBLER_SUPPORT friend class BinaryAssembler; #endif @@ -611,6 +590,14 @@ class ArmAssembler : public Assembler { virtual void Ror(Register rd, Register rm, Register rn, bool setcc = false, Condition cond = AL) = 0; + // Returns whether the `immediate` can fit in a `ShifterOperand`. If yes, + // `shifter_op` contains the operand. + virtual bool ShifterOperandCanHold(Register rd, + Register rn, + Opcode opcode, + uint32_t immediate, + ShifterOperand* shifter_op) = 0; + static bool IsInstructionForExceptionHandling(uintptr_t pc); virtual void Bind(Label* label) = 0; diff --git a/compiler/utils/arm/assembler_arm32.cc b/compiler/utils/arm/assembler_arm32.cc index a54176388..8f6d45ab5 100644 --- a/compiler/utils/arm/assembler_arm32.cc +++ b/compiler/utils/arm/assembler_arm32.cc @@ -25,6 +25,37 @@ namespace art { namespace arm { +bool Arm32Assembler::ShifterOperandCanHoldArm32(uint32_t immediate, ShifterOperand* shifter_op) { + // Avoid the more expensive test for frequent small immediate values. + if (immediate < (1 << kImmed8Bits)) { + shifter_op->type_ = ShifterOperand::kImmediate; + shifter_op->is_rotate_ = true; + shifter_op->rotate_ = 0; + shifter_op->immed_ = immediate; + return true; + } + // Note that immediate must be unsigned for the test to work correctly. + for (int rot = 0; rot < 16; rot++) { + uint32_t imm8 = (immediate << 2*rot) | (immediate >> (32 - 2*rot)); + if (imm8 < (1 << kImmed8Bits)) { + shifter_op->type_ = ShifterOperand::kImmediate; + shifter_op->is_rotate_ = true; + shifter_op->rotate_ = rot; + shifter_op->immed_ = imm8; + return true; + } + } + return false; +} + +bool Arm32Assembler::ShifterOperandCanHold(Register rd ATTRIBUTE_UNUSED, + Register rn ATTRIBUTE_UNUSED, + Opcode opcode ATTRIBUTE_UNUSED, + uint32_t immediate, + ShifterOperand* shifter_op) { + return ShifterOperandCanHoldArm32(immediate, shifter_op); +} + void Arm32Assembler::and_(Register rd, Register rn, const ShifterOperand& so, Condition cond) { EmitType01(cond, so.type(), AND, 0, rn, rd, so); @@ -1291,16 +1322,16 @@ void Arm32Assembler::AddConstant(Register rd, Register rn, int32_t value, // positive values and sub for negatives ones, which would slightly improve // the readability of generated code for some constants. ShifterOperand shifter_op; - if (ShifterOperand::CanHoldArm(value, &shifter_op)) { + if (ShifterOperandCanHoldArm32(value, &shifter_op)) { add(rd, rn, shifter_op, cond); - } else if (ShifterOperand::CanHoldArm(-value, &shifter_op)) { + } else if (ShifterOperandCanHoldArm32(-value, &shifter_op)) { sub(rd, rn, shifter_op, cond); } else { CHECK(rn != IP); - if (ShifterOperand::CanHoldArm(~value, &shifter_op)) { + if (ShifterOperandCanHoldArm32(~value, &shifter_op)) { mvn(IP, shifter_op, cond); add(rd, rn, ShifterOperand(IP), cond); - } else if (ShifterOperand::CanHoldArm(~(-value), &shifter_op)) { + } else if (ShifterOperandCanHoldArm32(~(-value), &shifter_op)) { mvn(IP, shifter_op, cond); sub(rd, rn, ShifterOperand(IP), cond); } else { @@ -1318,16 +1349,16 @@ void Arm32Assembler::AddConstant(Register rd, Register rn, int32_t value, void Arm32Assembler::AddConstantSetFlags(Register rd, Register rn, int32_t value, Condition cond) { ShifterOperand shifter_op; - if (ShifterOperand::CanHoldArm(value, &shifter_op)) { + if (ShifterOperandCanHoldArm32(value, &shifter_op)) { adds(rd, rn, shifter_op, cond); - } else if (ShifterOperand::CanHoldArm(-value, &shifter_op)) { + } else if (ShifterOperandCanHoldArm32(-value, &shifter_op)) { subs(rd, rn, shifter_op, cond); } else { CHECK(rn != IP); - if (ShifterOperand::CanHoldArm(~value, &shifter_op)) { + if (ShifterOperandCanHoldArm32(~value, &shifter_op)) { mvn(IP, shifter_op, cond); adds(rd, rn, ShifterOperand(IP), cond); - } else if (ShifterOperand::CanHoldArm(~(-value), &shifter_op)) { + } else if (ShifterOperandCanHoldArm32(~(-value), &shifter_op)) { mvn(IP, shifter_op, cond); subs(rd, rn, ShifterOperand(IP), cond); } else { @@ -1343,9 +1374,9 @@ void Arm32Assembler::AddConstantSetFlags(Register rd, Register rn, int32_t value void Arm32Assembler::LoadImmediate(Register rd, int32_t value, Condition cond) { ShifterOperand shifter_op; - if (ShifterOperand::CanHoldArm(value, &shifter_op)) { + if (ShifterOperandCanHoldArm32(value, &shifter_op)) { mov(rd, shifter_op, cond); - } else if (ShifterOperand::CanHoldArm(~value, &shifter_op)) { + } else if (ShifterOperandCanHoldArm32(~value, &shifter_op)) { mvn(rd, shifter_op, cond); } else { movw(rd, Low16Bits(value), cond); diff --git a/compiler/utils/arm/assembler_arm32.h b/compiler/utils/arm/assembler_arm32.h index 0b009e16d..6c8d41587 100644 --- a/compiler/utils/arm/assembler_arm32.h +++ b/compiler/utils/arm/assembler_arm32.h @@ -273,6 +273,12 @@ class Arm32Assembler FINAL : public ArmAssembler { int32_t offset, Condition cond = AL) OVERRIDE; + bool ShifterOperandCanHold(Register rd, + Register rn, + Opcode opcode, + uint32_t immediate, + ShifterOperand* shifter_op) OVERRIDE; + static bool IsInstructionForExceptionHandling(uintptr_t pc); @@ -359,6 +365,7 @@ class Arm32Assembler FINAL : public ArmAssembler { static int DecodeBranchOffset(int32_t inst); int32_t EncodeTstOffset(int offset, int32_t inst); int DecodeTstOffset(int32_t inst); + bool ShifterOperandCanHoldArm32(uint32_t immediate, ShifterOperand* shifter_op); }; } // namespace arm diff --git a/compiler/utils/arm/assembler_thumb2.cc b/compiler/utils/arm/assembler_thumb2.cc index dac2f2b31..479186c5d 100644 --- a/compiler/utils/arm/assembler_thumb2.cc +++ b/compiler/utils/arm/assembler_thumb2.cc @@ -25,6 +25,39 @@ namespace art { namespace arm { +bool Thumb2Assembler::ShifterOperandCanHold(Register rd, + Register rn, + Opcode opcode, + uint32_t immediate, + ShifterOperand* shifter_op) { + shifter_op->type_ = ShifterOperand::kImmediate; + shifter_op->immed_ = immediate; + shifter_op->is_shift_ = false; + shifter_op->is_rotate_ = false; + switch (opcode) { + case ADD: + case SUB: + if (rn == SP) { + if (rd == SP) { + return immediate < (1 << 9); // 9 bits allowed. + } else { + return immediate < (1 << 12); // 12 bits. + } + } + if (immediate < (1 << 12)) { // Less than (or equal to) 12 bits can always be done. + return true; + } + return ArmAssembler::ModifiedImmediate(immediate) != kInvalidModifiedImmediate; + + case MOV: + // TODO: Support less than or equal to 12bits. + return ArmAssembler::ModifiedImmediate(immediate) != kInvalidModifiedImmediate; + case MVN: + default: + return ArmAssembler::ModifiedImmediate(immediate) != kInvalidModifiedImmediate; + } +} + void Thumb2Assembler::and_(Register rd, Register rn, const ShifterOperand& so, Condition cond) { EmitDataProcessing(cond, AND, 0, rn, rd, so); @@ -2375,16 +2408,16 @@ void Thumb2Assembler::AddConstant(Register rd, Register rn, int32_t value, // positive values and sub for negatives ones, which would slightly improve // the readability of generated code for some constants. ShifterOperand shifter_op; - if (ShifterOperand::CanHoldThumb(rd, rn, ADD, value, &shifter_op)) { + if (ShifterOperandCanHold(rd, rn, ADD, value, &shifter_op)) { add(rd, rn, shifter_op, cond); - } else if (ShifterOperand::CanHoldThumb(rd, rn, SUB, -value, &shifter_op)) { + } else if (ShifterOperandCanHold(rd, rn, SUB, -value, &shifter_op)) { sub(rd, rn, shifter_op, cond); } else { CHECK(rn != IP); - if (ShifterOperand::CanHoldThumb(rd, rn, MVN, ~value, &shifter_op)) { + if (ShifterOperandCanHold(rd, rn, MVN, ~value, &shifter_op)) { mvn(IP, shifter_op, cond); add(rd, rn, ShifterOperand(IP), cond); - } else if (ShifterOperand::CanHoldThumb(rd, rn, MVN, ~(-value), &shifter_op)) { + } else if (ShifterOperandCanHold(rd, rn, MVN, ~(-value), &shifter_op)) { mvn(IP, shifter_op, cond); sub(rd, rn, ShifterOperand(IP), cond); } else { @@ -2402,16 +2435,16 @@ void Thumb2Assembler::AddConstant(Register rd, Register rn, int32_t value, void Thumb2Assembler::AddConstantSetFlags(Register rd, Register rn, int32_t value, Condition cond) { ShifterOperand shifter_op; - if (ShifterOperand::CanHoldThumb(rd, rn, ADD, value, &shifter_op)) { + if (ShifterOperandCanHold(rd, rn, ADD, value, &shifter_op)) { adds(rd, rn, shifter_op, cond); - } else if (ShifterOperand::CanHoldThumb(rd, rn, ADD, -value, &shifter_op)) { + } else if (ShifterOperandCanHold(rd, rn, ADD, -value, &shifter_op)) { subs(rd, rn, shifter_op, cond); } else { CHECK(rn != IP); - if (ShifterOperand::CanHoldThumb(rd, rn, MVN, ~value, &shifter_op)) { + if (ShifterOperandCanHold(rd, rn, MVN, ~value, &shifter_op)) { mvn(IP, shifter_op, cond); adds(rd, rn, ShifterOperand(IP), cond); - } else if (ShifterOperand::CanHoldThumb(rd, rn, MVN, ~(-value), &shifter_op)) { + } else if (ShifterOperandCanHold(rd, rn, MVN, ~(-value), &shifter_op)) { mvn(IP, shifter_op, cond); subs(rd, rn, ShifterOperand(IP), cond); } else { @@ -2425,11 +2458,12 @@ void Thumb2Assembler::AddConstantSetFlags(Register rd, Register rn, int32_t valu } } + void Thumb2Assembler::LoadImmediate(Register rd, int32_t value, Condition cond) { ShifterOperand shifter_op; - if (ShifterOperand::CanHoldThumb(rd, R0, MOV, value, &shifter_op)) { + if (ShifterOperandCanHold(rd, R0, MOV, value, &shifter_op)) { mov(rd, shifter_op, cond); - } else if (ShifterOperand::CanHoldThumb(rd, R0, MVN, ~value, &shifter_op)) { + } else if (ShifterOperandCanHold(rd, R0, MVN, ~value, &shifter_op)) { mvn(rd, shifter_op, cond); } else { movw(rd, Low16Bits(value), cond); @@ -2440,6 +2474,7 @@ void Thumb2Assembler::LoadImmediate(Register rd, int32_t value, Condition cond) } } + // Implementation note: this method must emit at most one instruction when // Address::CanHoldLoadOffsetThumb. void Thumb2Assembler::LoadFromOffset(LoadOperandType type, diff --git a/compiler/utils/arm/assembler_thumb2.h b/compiler/utils/arm/assembler_thumb2.h index cfa251acf..48a3a7eeb 100644 --- a/compiler/utils/arm/assembler_thumb2.h +++ b/compiler/utils/arm/assembler_thumb2.h @@ -304,6 +304,12 @@ class Thumb2Assembler FINAL : public ArmAssembler { int32_t offset, Condition cond = AL) OVERRIDE; + bool ShifterOperandCanHold(Register rd, + Register rn, + Opcode opcode, + uint32_t immediate, + ShifterOperand* shifter_op) OVERRIDE; + static bool IsInstructionForExceptionHandling(uintptr_t pc); diff --git a/test/434-shifter-operand/expected.txt b/test/434-shifter-operand/expected.txt new file mode 100644 index 000000000..52289c68f --- /dev/null +++ b/test/434-shifter-operand/expected.txt @@ -0,0 +1,3 @@ +false +false +true diff --git a/test/434-shifter-operand/info.txt b/test/434-shifter-operand/info.txt new file mode 100644 index 000000000..1ec9adc5a --- /dev/null +++ b/test/434-shifter-operand/info.txt @@ -0,0 +1,2 @@ +Regression test for the arm backend of the optimizing +compiler, that used to misuse ShifterOperand::CanHold. diff --git a/test/434-shifter-operand/src/Main.java b/test/434-shifter-operand/src/Main.java new file mode 100644 index 000000000..4d188eb4f --- /dev/null +++ b/test/434-shifter-operand/src/Main.java @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2014 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) { + System.out.println(foo(42)); + System.out.println(foo(0xffffffff)); + System.out.println(foo(0xf0000000)); + } + + public static boolean foo(int a) { + return a < 0xf000000b; + } +} -- 2.11.0