From: Tim Northover Date: Wed, 13 Apr 2016 23:08:27 +0000 (+0000) Subject: ARM: override cost function to re-enable ConstantHoisting (& fix it). X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=5c02f9ad28b02eccd04e10c5c1574b97bc9564fb;p=platform%2Fupstream%2Fllvm.git 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 --- diff --git a/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp b/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp index c152011..932ec2d 100644 --- a/llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp +++ b/llvm/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/llvm/lib/Target/ARM/ARMTargetTransformInfo.h b/llvm/lib/Target/ARM/ARMTargetTransformInfo.h index 7d8d238..7808587 100644 --- a/llvm/lib/Target/ARM/ARMTargetTransformInfo.h +++ b/llvm/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/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp index 84f7f5f..d549228 100644 --- a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp +++ b/llvm/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/llvm/test/CodeGen/ARM/lsr-code-insertion.ll b/llvm/test/CodeGen/ARM/lsr-code-insertion.ll index aa2b2d2..766710f 100644 --- a/llvm/test/CodeGen/ARM/lsr-code-insertion.ll +++ b/llvm/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/llvm/test/CodeGen/ARM/static-addr-hoisting.ll b/llvm/test/CodeGen/ARM/static-addr-hoisting.ll new file mode 100644 index 0000000..3d47e02 --- /dev/null +++ b/llvm/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/llvm/test/Transforms/ConstantHoisting/ARM/bad-cases.ll b/llvm/test/Transforms/ConstantHoisting/ARM/bad-cases.ll new file mode 100644 index 0000000..3602eb9 --- /dev/null +++ b/llvm/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/llvm/test/Transforms/ConstantHoisting/ARM/lit.local.cfg b/llvm/test/Transforms/ConstantHoisting/ARM/lit.local.cfg new file mode 100644 index 0000000..236e1d3 --- /dev/null +++ b/llvm/test/Transforms/ConstantHoisting/ARM/lit.local.cfg @@ -0,0 +1,2 @@ +if not 'ARM' in config.root.targets: + config.unsupported = True