From c010a3819d6c4c631fca6ff33287eca6fccedc0a Mon Sep 17 00:00:00 2001 From: Martyn Capewell Date: Fri, 1 Dec 2017 11:00:37 +0000 Subject: [PATCH] [arm] Don't overwrite input in atomic ops The Arm atomic operations used an input register as a temporary, corrupting it for future uses. Fix this by adding another temporary, the impact of which is partly reduced by removing the "unique" requirement on the address base register. Bug: Change-Id: I99a7bc3c14100d64cb7478e2053cf83ab6dcea50 Reviewed-on: https://chromium-review.googlesource.com/795953 Commit-Queue: Martyn Capewell Reviewed-by: Benedikt Meurer Reviewed-by: Ben Smith Cr-Commit-Position: refs/heads/master@{#49785} --- src/compiler/arm/code-generator-arm.cc | 108 ++++++++++--------- src/compiler/arm/instruction-selector-arm.cc | 25 ++--- 2 files changed, 70 insertions(+), 63 deletions(-) diff --git a/src/compiler/arm/code-generator-arm.cc b/src/compiler/arm/code-generator-arm.cc index 1a66e5b7d4..dd5973d4b1 100644 --- a/src/compiler/arm/code-generator-arm.cc +++ b/src/compiler/arm/code-generator-arm.cc @@ -432,51 +432,51 @@ Condition FlagsConditionToCondition(FlagsCondition condition) { __ dmb(ISH); \ } while (0) -#define ASSEMBLE_ATOMIC_EXCHANGE_INTEGER(load_instr, store_instr) \ - do { \ - Label exchange; \ - __ add(i.InputRegister(0), i.InputRegister(0), i.InputRegister(1)); \ - __ dmb(ISH); \ - __ bind(&exchange); \ - __ load_instr(i.OutputRegister(0), i.InputRegister(0)); \ - __ store_instr(i.TempRegister(0), i.InputRegister(2), i.InputRegister(0)); \ - __ teq(i.TempRegister(0), Operand(0)); \ - __ b(ne, &exchange); \ - __ dmb(ISH); \ - } while (0) - -#define ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(load_instr, store_instr) \ - do { \ - Label compareExchange; \ - Label exit; \ - __ add(i.InputRegister(0), i.InputRegister(0), i.InputRegister(1)); \ - __ dmb(ISH); \ - __ bind(&compareExchange); \ - __ load_instr(i.OutputRegister(0), i.InputRegister(0)); \ - __ teq(i.InputRegister(2), Operand(i.OutputRegister(0))); \ - __ b(ne, &exit); \ - __ store_instr(i.TempRegister(0), i.InputRegister(3), i.InputRegister(0)); \ - __ teq(i.TempRegister(0), Operand(0)); \ - __ b(ne, &compareExchange); \ - __ bind(&exit); \ - __ dmb(ISH); \ - } while (0) - -#define ASSEMBLE_ATOMIC_BINOP(load_instr, store_instr, bin_instr) \ +#define ASSEMBLE_ATOMIC_EXCHANGE_INTEGER(load_instr, store_instr) \ do { \ - Label binop; \ - __ add(i.InputRegister(0), i.InputRegister(0), i.InputRegister(1)); \ + Label exchange; \ + __ add(i.TempRegister(1), i.InputRegister(0), i.InputRegister(1)); \ __ dmb(ISH); \ - __ bind(&binop); \ - __ load_instr(i.OutputRegister(0), i.InputRegister(0)); \ - __ bin_instr(i.TempRegister(0), i.OutputRegister(0), \ - Operand(i.InputRegister(2))); \ - __ store_instr(i.TempRegister(1), i.TempRegister(0), i.InputRegister(0)); \ - __ teq(i.TempRegister(1), Operand(0)); \ - __ b(ne, &binop); \ + __ bind(&exchange); \ + __ load_instr(i.OutputRegister(0), i.TempRegister(1)); \ + __ store_instr(i.TempRegister(0), i.InputRegister(2), i.TempRegister(1)); \ + __ teq(i.TempRegister(0), Operand(0)); \ + __ b(ne, &exchange); \ __ dmb(ISH); \ } while (0) +#define ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(load_instr, store_instr, \ + cmp_reg) \ + do { \ + Label compareExchange; \ + Label exit; \ + __ dmb(ISH); \ + __ bind(&compareExchange); \ + __ load_instr(i.OutputRegister(0), i.TempRegister(1)); \ + __ teq(cmp_reg, Operand(i.OutputRegister(0))); \ + __ b(ne, &exit); \ + __ store_instr(i.TempRegister(0), i.InputRegister(3), i.TempRegister(1)); \ + __ teq(i.TempRegister(0), Operand(0)); \ + __ b(ne, &compareExchange); \ + __ bind(&exit); \ + __ dmb(ISH); \ + } while (0) + +#define ASSEMBLE_ATOMIC_BINOP(load_instr, store_instr, bin_instr) \ + do { \ + Label binop; \ + __ add(i.TempRegister(1), i.InputRegister(0), i.InputRegister(1)); \ + __ dmb(ISH); \ + __ bind(&binop); \ + __ load_instr(i.OutputRegister(0), i.TempRegister(1)); \ + __ bin_instr(i.TempRegister(0), i.OutputRegister(0), \ + Operand(i.InputRegister(2))); \ + __ store_instr(i.TempRegister(2), i.TempRegister(0), i.TempRegister(1)); \ + __ teq(i.TempRegister(2), Operand(0)); \ + __ b(ne, &binop); \ + __ dmb(ISH); \ + } while (0) + #define ASSEMBLE_IEEE754_BINOP(name) \ do { \ /* TODO(bmeurer): We should really get rid of this special instruction, */ \ @@ -2642,25 +2642,35 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction( ASSEMBLE_ATOMIC_EXCHANGE_INTEGER(ldrex, strex); break; case kAtomicCompareExchangeInt8: - __ uxtb(i.InputRegister(2), i.InputRegister(2)); - ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexb, strexb); + __ add(i.TempRegister(1), i.InputRegister(0), i.InputRegister(1)); + __ uxtb(i.TempRegister(2), i.InputRegister(2)); + ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexb, strexb, + i.TempRegister(2)); __ sxtb(i.OutputRegister(0), i.OutputRegister(0)); break; case kAtomicCompareExchangeUint8: - __ uxtb(i.InputRegister(2), i.InputRegister(2)); - ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexb, strexb); + __ add(i.TempRegister(1), i.InputRegister(0), i.InputRegister(1)); + __ uxtb(i.TempRegister(2), i.InputRegister(2)); + ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexb, strexb, + i.TempRegister(2)); break; case kAtomicCompareExchangeInt16: - __ uxth(i.InputRegister(2), i.InputRegister(2)); - ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexh, strexh); + __ add(i.TempRegister(1), i.InputRegister(0), i.InputRegister(1)); + __ uxth(i.TempRegister(2), i.InputRegister(2)); + ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexh, strexh, + i.TempRegister(2)); __ sxth(i.OutputRegister(0), i.OutputRegister(0)); break; case kAtomicCompareExchangeUint16: - __ uxth(i.InputRegister(2), i.InputRegister(2)); - ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexh, strexh); + __ add(i.TempRegister(1), i.InputRegister(0), i.InputRegister(1)); + __ uxth(i.TempRegister(2), i.InputRegister(2)); + ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrexh, strexh, + i.TempRegister(2)); break; case kAtomicCompareExchangeWord32: - ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrex, strex); + __ add(i.TempRegister(1), i.InputRegister(0), i.InputRegister(1)); + ASSEMBLE_ATOMIC_COMPARE_EXCHANGE_INTEGER(ldrex, strex, + i.InputRegister(2)); break; #define ATOMIC_BINOP_CASE(op, inst) \ case kAtomic##op##Int8: \ diff --git a/src/compiler/arm/instruction-selector-arm.cc b/src/compiler/arm/instruction-selector-arm.cc index 4ded82fa5b..2f4dbe38b5 100644 --- a/src/compiler/arm/instruction-selector-arm.cc +++ b/src/compiler/arm/instruction-selector-arm.cc @@ -2276,15 +2276,14 @@ void InstructionSelector::VisitAtomicExchange(Node* node) { AddressingMode addressing_mode = kMode_Offset_RR; InstructionOperand inputs[3]; size_t input_count = 0; - inputs[input_count++] = g.UseUniqueRegister(base); + inputs[input_count++] = g.UseRegister(base); inputs[input_count++] = g.UseRegister(index); inputs[input_count++] = g.UseUniqueRegister(value); InstructionOperand outputs[1]; outputs[0] = g.DefineAsRegister(node); - InstructionOperand temp[1]; - temp[0] = g.TempRegister(); + InstructionOperand temps[] = {g.TempRegister(), g.TempRegister()}; InstructionCode code = opcode | AddressingModeField::encode(addressing_mode); - Emit(code, 1, outputs, input_count, inputs, 1, temp); + Emit(code, 1, outputs, input_count, inputs, arraysize(temps), temps); } void InstructionSelector::VisitAtomicCompareExchange(Node* node) { @@ -2313,16 +2312,16 @@ void InstructionSelector::VisitAtomicCompareExchange(Node* node) { AddressingMode addressing_mode = kMode_Offset_RR; InstructionOperand inputs[4]; size_t input_count = 0; - inputs[input_count++] = g.UseUniqueRegister(base); + inputs[input_count++] = g.UseRegister(base); inputs[input_count++] = g.UseRegister(index); inputs[input_count++] = g.UseUniqueRegister(old_value); inputs[input_count++] = g.UseUniqueRegister(new_value); InstructionOperand outputs[1]; outputs[0] = g.DefineAsRegister(node); - InstructionOperand temp[1]; - temp[0] = g.TempRegister(); + InstructionOperand temps[] = {g.TempRegister(), g.TempRegister(), + g.TempRegister()}; InstructionCode code = opcode | AddressingModeField::encode(addressing_mode); - Emit(code, 1, outputs, input_count, inputs, 1, temp); + Emit(code, 1, outputs, input_count, inputs, arraysize(temps), temps); } void InstructionSelector::VisitAtomicBinaryOperation( @@ -2352,17 +2351,15 @@ void InstructionSelector::VisitAtomicBinaryOperation( AddressingMode addressing_mode = kMode_Offset_RR; InstructionOperand inputs[3]; size_t input_count = 0; - inputs[input_count++] = g.UseUniqueRegister(base); + inputs[input_count++] = g.UseRegister(base); inputs[input_count++] = g.UseRegister(index); inputs[input_count++] = g.UseUniqueRegister(value); InstructionOperand outputs[1]; outputs[0] = g.DefineAsRegister(node); - InstructionOperand temps[2]; - size_t temp_count = 0; - temps[temp_count++] = g.TempRegister(); - temps[temp_count++] = g.TempRegister(); + InstructionOperand temps[] = {g.TempRegister(), g.TempRegister(), + g.TempRegister()}; InstructionCode code = opcode | AddressingModeField::encode(addressing_mode); - Emit(code, 1, outputs, input_count, inputs, temp_count, temps); + Emit(code, 1, outputs, input_count, inputs, arraysize(temps), temps); } #define VISIT_ATOMIC_BINOP(op) \