From 1f26a47630b190056716b0a3ba062f230d255bcf Mon Sep 17 00:00:00 2001 From: John Brawn Date: Fri, 20 Mar 2015 17:20:07 +0000 Subject: [PATCH] [ARM] Fix handling of thumb1 out-of-range frame offsets LocalStackSlotPass assumes that isFrameOffsetLegal doesn't change its answer when the base register changes. Unfortunately this isn't true in thumb1, where SP-based loads allow a larger offset than non-SP-based loads, and this causes the base register reuse code to generate instructions that are unencodable, causing an assertion failure. Solve this by adding a BaseReg parameter to isFrameOffsetLegal, which ARMBaseRegisterInfo can then make use of to give the correct answer. Differential Revision: http://reviews.llvm.org/D8419 llvm-svn: 232825 --- llvm/include/llvm/Target/TargetRegisterInfo.h | 6 ++-- llvm/lib/CodeGen/LocalStackSlotAllocation.cpp | 12 ++++---- llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp | 5 ++-- llvm/lib/Target/AArch64/AArch64RegisterInfo.h | 2 +- llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp | 9 +++--- llvm/lib/Target/ARM/ARMBaseRegisterInfo.h | 2 +- llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp | 3 +- llvm/lib/Target/PowerPC/PPCRegisterInfo.h | 2 +- llvm/test/CodeGen/Thumb/stack-access.ll | 37 +++++++++++++++++++++++++ 9 files changed, 59 insertions(+), 19 deletions(-) diff --git a/llvm/include/llvm/Target/TargetRegisterInfo.h b/llvm/include/llvm/Target/TargetRegisterInfo.h index 08309e0..4184052 100644 --- a/llvm/include/llvm/Target/TargetRegisterInfo.h +++ b/llvm/include/llvm/Target/TargetRegisterInfo.h @@ -799,9 +799,9 @@ public: llvm_unreachable("resolveFrameIndex does not exist on this target"); } - /// isFrameOffsetLegal - Determine whether a given offset immediate is - /// encodable to resolve a frame index. - virtual bool isFrameOffsetLegal(const MachineInstr *MI, + /// isFrameOffsetLegal - Determine whether a given base register plus offset + /// immediate is encodable to resolve a frame index. + virtual bool isFrameOffsetLegal(const MachineInstr *MI, unsigned BaseReg, int64_t Offset) const { llvm_unreachable("isFrameOffsetLegal does not exist on this target"); } diff --git a/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp b/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp index e8bf687..8378429 100644 --- a/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp +++ b/llvm/lib/CodeGen/LocalStackSlotAllocation.cpp @@ -252,7 +252,8 @@ void LocalStackSlotPass::calculateFrameObjectOffsets(MachineFunction &Fn) { } static inline bool -lookupCandidateBaseReg(int64_t BaseOffset, +lookupCandidateBaseReg(unsigned BaseReg, + int64_t BaseOffset, int64_t FrameSizeAdjust, int64_t LocalFrameOffset, const MachineInstr *MI, @@ -260,7 +261,7 @@ lookupCandidateBaseReg(int64_t BaseOffset, // Check if the relative offset from the where the base register references // to the target address is in range for the instruction. int64_t Offset = FrameSizeAdjust + LocalFrameOffset - BaseOffset; - return TRI->isFrameOffsetLegal(MI, Offset); + return TRI->isFrameOffsetLegal(MI, BaseReg, Offset); } bool LocalStackSlotPass::insertFrameReferenceRegisters(MachineFunction &Fn) { @@ -362,8 +363,9 @@ bool LocalStackSlotPass::insertFrameReferenceRegisters(MachineFunction &Fn) { // instruction itself will be taken into account by the target, // so we don't have to adjust for it here when reusing a base // register. - if (UsedBaseReg && lookupCandidateBaseReg(BaseOffset, FrameSizeAdjust, - LocalOffset, MI, TRI)) { + if (UsedBaseReg && lookupCandidateBaseReg(BaseReg, BaseOffset, + FrameSizeAdjust, LocalOffset, MI, + TRI)) { DEBUG(dbgs() << " Reusing base register " << BaseReg << "\n"); // We found a register to reuse. Offset = FrameSizeAdjust + LocalOffset - BaseOffset; @@ -382,7 +384,7 @@ bool LocalStackSlotPass::insertFrameReferenceRegisters(MachineFunction &Fn) { // then don't bother creating it. if (ref + 1 >= e || !lookupCandidateBaseReg( - BaseOffset, FrameSizeAdjust, + BaseReg, BaseOffset, FrameSizeAdjust, FrameReferenceInsns[ref + 1].getLocalOffset(), FrameReferenceInsns[ref + 1].getMachineInstr(), TRI)) { BaseOffset = PrevBaseOffset; diff --git a/llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp b/llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp index 6f5de36..33c11fe 100644 --- a/llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp +++ b/llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp @@ -271,7 +271,7 @@ bool AArch64RegisterInfo::needsFrameBaseReg(MachineInstr *MI, // The FP is only available if there is no dynamic realignment. We // don't know for sure yet whether we'll need that, so we guess based // on whether there are any local variables that would trigger it. - if (TFI->hasFP(MF) && isFrameOffsetLegal(MI, FPOffset)) + if (TFI->hasFP(MF) && isFrameOffsetLegal(MI, AArch64::FP, FPOffset)) return false; // If we can reference via the stack pointer or base pointer, try that. @@ -279,7 +279,7 @@ bool AArch64RegisterInfo::needsFrameBaseReg(MachineInstr *MI, // to only disallow SP relative references in the live range of // the VLA(s). In practice, it's unclear how much difference that // would make, but it may be worth doing. - if (isFrameOffsetLegal(MI, Offset)) + if (isFrameOffsetLegal(MI, AArch64::SP, Offset)) return false; // The offset likely isn't legal; we want to allocate a virtual base register. @@ -287,6 +287,7 @@ bool AArch64RegisterInfo::needsFrameBaseReg(MachineInstr *MI, } bool AArch64RegisterInfo::isFrameOffsetLegal(const MachineInstr *MI, + unsigned BaseReg, int64_t Offset) const { assert(Offset <= INT_MAX && "Offset too big to fit in int."); assert(MI && "Unable to get the legal offset for nil instruction."); diff --git a/llvm/lib/Target/AArch64/AArch64RegisterInfo.h b/llvm/lib/Target/AArch64/AArch64RegisterInfo.h index cb752b3..c01bfa5 100644 --- a/llvm/lib/Target/AArch64/AArch64RegisterInfo.h +++ b/llvm/lib/Target/AArch64/AArch64RegisterInfo.h @@ -72,7 +72,7 @@ public: bool requiresFrameIndexScavenging(const MachineFunction &MF) const override; bool needsFrameBaseReg(MachineInstr *MI, int64_t Offset) const override; - bool isFrameOffsetLegal(const MachineInstr *MI, + bool isFrameOffsetLegal(const MachineInstr *MI, unsigned BaseReg, int64_t Offset) const override; void materializeFrameBaseRegister(MachineBasicBlock *MBB, unsigned BaseReg, int FrameIdx, diff --git a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp index 49efdc3..a8c7657 100644 --- a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp +++ b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.cpp @@ -536,9 +536,8 @@ needsFrameBaseReg(MachineInstr *MI, int64_t Offset) const { // on whether there are any local variables that would trigger it. unsigned StackAlign = TFI->getStackAlignment(); if (TFI->hasFP(MF) && - (MI->getDesc().TSFlags & ARMII::AddrModeMask) != ARMII::AddrModeT1_s && !((MFI->getLocalFrameMaxAlign() > StackAlign) && canRealignStack(MF))) { - if (isFrameOffsetLegal(MI, FPOffset)) + if (isFrameOffsetLegal(MI, getFrameRegister(MF), FPOffset)) return false; } // If we can reference via the stack pointer, try that. @@ -546,7 +545,7 @@ needsFrameBaseReg(MachineInstr *MI, int64_t Offset) const { // to only disallow SP relative references in the live range of // the VLA(s). In practice, it's unclear how much difference that // would make, but it may be worth doing. - if (!MFI->hasVarSizedObjects() && isFrameOffsetLegal(MI, Offset)) + if (!MFI->hasVarSizedObjects() && isFrameOffsetLegal(MI, ARM::SP, Offset)) return false; // The offset likely isn't legal, we want to allocate a virtual base register. @@ -609,7 +608,7 @@ void ARMBaseRegisterInfo::resolveFrameIndex(MachineInstr &MI, unsigned BaseReg, (void)Done; } -bool ARMBaseRegisterInfo::isFrameOffsetLegal(const MachineInstr *MI, +bool ARMBaseRegisterInfo::isFrameOffsetLegal(const MachineInstr *MI, unsigned BaseReg, int64_t Offset) const { const MCInstrDesc &Desc = MI->getDesc(); unsigned AddrMode = (Desc.TSFlags & ARMII::AddrModeMask); @@ -653,7 +652,7 @@ bool ARMBaseRegisterInfo::isFrameOffsetLegal(const MachineInstr *MI, NumBits = 8; break; case ARMII::AddrModeT1_s: - NumBits = 8; + NumBits = (BaseReg == ARM::SP ? 8 : 5); Scale = 4; isSigned = false; break; diff --git a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h index af2b9a4..fdc1ef9 100644 --- a/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h +++ b/llvm/lib/Target/ARM/ARMBaseRegisterInfo.h @@ -143,7 +143,7 @@ public: int64_t Offset) const override; void resolveFrameIndex(MachineInstr &MI, unsigned BaseReg, int64_t Offset) const override; - bool isFrameOffsetLegal(const MachineInstr *MI, + bool isFrameOffsetLegal(const MachineInstr *MI, unsigned BaseReg, int64_t Offset) const override; bool cannotEliminateFrame(const MachineFunction &MF) const; diff --git a/llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp b/llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp index b0c3d60..8653734 100644 --- a/llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp +++ b/llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp @@ -993,7 +993,7 @@ needsFrameBaseReg(MachineInstr *MI, int64_t Offset) const { // The frame pointer will point to the end of the stack, so estimate the // offset as the difference between the object offset and the FP location. - return !isFrameOffsetLegal(MI, Offset); + return !isFrameOffsetLegal(MI, getBaseRegister(MF), Offset); } /// Insert defining instruction(s) for BaseReg to @@ -1045,6 +1045,7 @@ void PPCRegisterInfo::resolveFrameIndex(MachineInstr &MI, unsigned BaseReg, } bool PPCRegisterInfo::isFrameOffsetLegal(const MachineInstr *MI, + unsigned BaseReg, int64_t Offset) const { unsigned FIOperandNum = 0; while (!MI->getOperand(FIOperandNum).isFI()) { diff --git a/llvm/lib/Target/PowerPC/PPCRegisterInfo.h b/llvm/lib/Target/PowerPC/PPCRegisterInfo.h index 0342174..765d574 100644 --- a/llvm/lib/Target/PowerPC/PPCRegisterInfo.h +++ b/llvm/lib/Target/PowerPC/PPCRegisterInfo.h @@ -94,7 +94,7 @@ public: int64_t Offset) const override; void resolveFrameIndex(MachineInstr &MI, unsigned BaseReg, int64_t Offset) const override; - bool isFrameOffsetLegal(const MachineInstr *MI, + bool isFrameOffsetLegal(const MachineInstr *MI, unsigned BaseReg, int64_t Offset) const override; // Debug information queries. diff --git a/llvm/test/CodeGen/Thumb/stack-access.ll b/llvm/test/CodeGen/Thumb/stack-access.ll index bc5ecc1..fded410 100644 --- a/llvm/test/CodeGen/Thumb/stack-access.ll +++ b/llvm/test/CodeGen/Thumb/stack-access.ll @@ -88,3 +88,40 @@ define void @test7() { ret void } + +; Check that loads/stores with out-of-range offsets are handled correctly +define void @test8() { + %arr3 = alloca [224 x i32], align 4 + %arr2 = alloca [224 x i32], align 4 + %arr1 = alloca [224 x i32], align 4 + +; CHECK: movs [[REG:r[0-9]+]], #1 +; CHECK: str [[REG]], [sp] + %arr1idx1 = getelementptr inbounds [224 x i32], [224 x i32]* %arr1, i32 0, i32 0 + store i32 1, i32* %arr1idx1, align 4 + +; Offset in range for sp-based store, but not for non-sp-based store +; CHECK: str [[REG]], [sp, #128] + %arr1idx2 = getelementptr inbounds [224 x i32], [224 x i32]* %arr1, i32 0, i32 32 + store i32 1, i32* %arr1idx2, align 4 + +; CHECK: str [[REG]], [sp, #896] + %arr2idx1 = getelementptr inbounds [224 x i32], [224 x i32]* %arr2, i32 0, i32 0 + store i32 1, i32* %arr2idx1, align 4 + +; %arr2 is in range, but this element of it is not +; CHECK: str [[REG]], [{{r[0-9]+}}] + %arr2idx2 = getelementptr inbounds [224 x i32], [224 x i32]* %arr2, i32 0, i32 32 + store i32 1, i32* %arr2idx2, align 4 + +; %arr3 is not in range +; CHECK: str [[REG]], [{{r[0-9]+}}] + %arr3idx1 = getelementptr inbounds [224 x i32], [224 x i32]* %arr3, i32 0, i32 0 + store i32 1, i32* %arr3idx1, align 4 + +; CHECK: str [[REG]], [{{r[0-9]+}}] + %arr3idx2 = getelementptr inbounds [224 x i32], [224 x i32]* %arr3, i32 0, i32 32 + store i32 1, i32* %arr3idx2, align 4 + + ret void +} -- 2.7.4