From 3d26dcef22fcf20238f09ffff3912c89bb5cc658 Mon Sep 17 00:00:00 2001 From: Tim Northover Date: Wed, 13 Apr 2016 23:08:27 +0000 Subject: [PATCH] ARM: override cost function to re-enable ConstantHoisting (& fix it). At some point, ARM stopped getting any benefit from ConstantHoisting because the pass called a different variant of getIntImmCost. Reimplementing the correct variant revealed some problems, however: + ConstantHoisting was modifying switch statements. This is simply invalid, the cases must remain integer constants no matter the notional cost. + ConstantHoisting was mangling alloca instructions in the entry block. These should be handled by FrameLowering, so constants actually have a cost of 0. Worse, the resulting bitcasts meant they became dynamic allocas. rdar://25707382 llvm-svn: 266260 --- lib/Target/ARM/ARMTargetTransformInfo.cpp | 10 ++-- lib/Target/ARM/ARMTargetTransformInfo.h | 4 ++ lib/Transforms/Scalar/ConstantHoisting.cpp | 12 +++++ test/CodeGen/ARM/lsr-code-insertion.ll | 4 +- test/CodeGen/ARM/static-addr-hoisting.ll | 22 +++++++++ .../ConstantHoisting/ARM/bad-cases.ll | 47 +++++++++++++++++++ .../ConstantHoisting/ARM/lit.local.cfg | 2 + 7 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 test/CodeGen/ARM/static-addr-hoisting.ll create mode 100644 test/Transforms/ConstantHoisting/ARM/bad-cases.ll create mode 100644 test/Transforms/ConstantHoisting/ARM/lit.local.cfg diff --git a/lib/Target/ARM/ARMTargetTransformInfo.cpp b/lib/Target/ARM/ARMTargetTransformInfo.cpp index c1520119ef2..932ec2d46da 100644 --- a/lib/Target/ARM/ARMTargetTransformInfo.cpp +++ b/lib/Target/ARM/ARMTargetTransformInfo.cpp @@ -18,12 +18,12 @@ using namespace llvm; int ARMTTIImpl::getIntImmCost(const APInt &Imm, Type *Ty) { assert(Ty->isIntegerTy()); - unsigned Bits = Ty->getPrimitiveSizeInBits(); - if (Bits == 0 || Bits > 32) - return 4; + unsigned Bits = Ty->getPrimitiveSizeInBits(); + if (Bits == 0 || Bits > 64) + return 4; - int32_t SImmVal = Imm.getSExtValue(); - uint32_t ZImmVal = Imm.getZExtValue(); + int64_t SImmVal = Imm.getSExtValue(); + uint64_t ZImmVal = Imm.getZExtValue(); if (!ST->isThumb()) { if ((SImmVal >= 0 && SImmVal < 65536) || (ARM_AM::getSOImmVal(ZImmVal) != -1) || diff --git a/lib/Target/ARM/ARMTargetTransformInfo.h b/lib/Target/ARM/ARMTargetTransformInfo.h index 7d8d2381c98..7808587c205 100644 --- a/lib/Target/ARM/ARMTargetTransformInfo.h +++ b/lib/Target/ARM/ARMTargetTransformInfo.h @@ -60,6 +60,10 @@ public: using BaseT::getIntImmCost; int getIntImmCost(const APInt &Imm, Type *Ty); + int getIntImmCost(unsigned Opcode, unsigned Idx, const APInt &Imm, Type *Ty) { + return getIntImmCost(Imm, Ty); + } + /// @} /// \name Vector TTI Implementations diff --git a/lib/Transforms/Scalar/ConstantHoisting.cpp b/lib/Transforms/Scalar/ConstantHoisting.cpp index 84f7f5fff5b..d549228757f 100644 --- a/lib/Transforms/Scalar/ConstantHoisting.cpp +++ b/lib/Transforms/Scalar/ConstantHoisting.cpp @@ -320,6 +320,18 @@ void ConstantHoisting::collectConstantCandidates(ConstCandMapType &ConstCandMap, if (isa(Call->getCalledValue())) return; + // Switch cases must remain constant, and if the value being tested is + // constant the entire thing should disappear. + if (isa(Inst)) + return; + + // Static allocas (constant size in the entry block) are handled by + // prologue/epilogue insertion so they're free anyway. We definitely don't + // want to make them non-constant. + auto AI = dyn_cast(Inst); + if (AI && AI->isStaticAlloca()) + return; + // Scan all operands. for (unsigned Idx = 0, E = Inst->getNumOperands(); Idx != E; ++Idx) { Value *Opnd = Inst->getOperand(Idx); diff --git a/test/CodeGen/ARM/lsr-code-insertion.ll b/test/CodeGen/ARM/lsr-code-insertion.ll index aa2b2d26d12..766710fd1d6 100644 --- a/test/CodeGen/ARM/lsr-code-insertion.ll +++ b/test/CodeGen/ARM/lsr-code-insertion.ll @@ -9,8 +9,8 @@ ; ; CHECK: ldr [[R6:r[0-9*]+]], LCP ; CHECK: cmp {{.*}}, [[R6]] -; CHECK: ldrle -; CHECK-NEXT: strle +; CHECK-NOT: lt +; CHECK: strlt target triple = "arm-apple-darwin8" diff --git a/test/CodeGen/ARM/static-addr-hoisting.ll b/test/CodeGen/ARM/static-addr-hoisting.ll new file mode 100644 index 00000000000..3d47e02f965 --- /dev/null +++ b/test/CodeGen/ARM/static-addr-hoisting.ll @@ -0,0 +1,22 @@ +; RUN: llc -mtriple=thumbv7-apple-ios %s -o - | FileCheck %s + +define void @multiple_store() { +; CHECK-LABEL: multiple_store: +; CHECK: movw r[[BASE1:[0-9]+]], #16960 +; CHECK: movs [[VAL:r[0-9]+]], #42 +; CHECK: movt r[[BASE1]], #15 + +; CHECK: str [[VAL]], [r[[BASE1]]] +; CHECK: str [[VAL]], [r[[BASE1]], #24] +; CHECK: str.w [[VAL]], [r[[BASE1]], #42] + +; CHECK: movw r[[BASE2:[0-9]+]], #20394 +; CHECK: movt r[[BASE2]], #18 + +; CHECK: str [[VAL]], [r[[BASE2]]] + store i32 42, i32* inttoptr(i32 1000000 to i32*) + store i32 42, i32* inttoptr(i32 1000024 to i32*) + store i32 42, i32* inttoptr(i32 1000042 to i32*) + store i32 42, i32* inttoptr(i32 1200042 to i32*) + ret void +} diff --git a/test/Transforms/ConstantHoisting/ARM/bad-cases.ll b/test/Transforms/ConstantHoisting/ARM/bad-cases.ll new file mode 100644 index 00000000000..3602eb9f3fd --- /dev/null +++ b/test/Transforms/ConstantHoisting/ARM/bad-cases.ll @@ -0,0 +1,47 @@ +; RUN: opt -consthoist -S < %s | FileCheck %s +target triple = "thumbv6m-none-eabi" + +; Allocas in the entry block get handled (for free) by +; prologue/epilogue. Elsewhere they're fair game though. +define void @avoid_allocas() { +; CHECK-LABEL: @avoid_allocas +; CHECK: %addr1 = alloca i8, i32 1000 +; CHECK: %addr2 = alloca i8, i32 1020 + + %addr1 = alloca i8, i32 1000 + %addr2 = alloca i8, i32 1020 + br label %elsewhere + +elsewhere: +; CHECK: [[BASE:%.*]] = bitcast i32 1000 to i32 +; CHECK: alloca i8, i32 [[BASE]] +; CHECK: [[NEXT:%.*]] = add i32 [[BASE]], 20 +; CHECK: alloca i8, i32 [[NEXT]] + + %addr3 = alloca i8, i32 1000 + %addr4 = alloca i8, i32 1020 + + ret void +} + +; The case values of switch instructions are required to be constants. +define void @avoid_switch(i32 %in) { +; CHECK-LABEL: @avoid_switch +; CHECK: switch i32 %in, label %default [ +; CHECK: i32 1000, label %bb1 +; CHECK: i32 1020, label %bb2 +; CHECK: ] + + switch i32 %in, label %default + [ i32 1000, label %bb1 + i32 1020, label %bb2 ] + +bb1: + ret void + +bb2: + ret void + +default: + ret void +} diff --git a/test/Transforms/ConstantHoisting/ARM/lit.local.cfg b/test/Transforms/ConstantHoisting/ARM/lit.local.cfg new file mode 100644 index 00000000000..236e1d34416 --- /dev/null +++ b/test/Transforms/ConstantHoisting/ARM/lit.local.cfg @@ -0,0 +1,2 @@ +if not 'ARM' in config.root.targets: + config.unsupported = True