From 08505775f92063b512562ba0bd3a1c0b4b45dd03 Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Fri, 23 Feb 2024 09:31:30 +0000 Subject: [PATCH 1/3] [ARM] Update IsRestored for LR based on all returns PR #75527 fixed ARMFrameLowering to set the IsRestored flag for LR based on all of the return instructions in the function, not just one. However, there is also code in ARMLoadStoreOptimizer which changes return instructions, but it set IsRestored based on the one instruction it changed, not the whole function. The fix is to factor out the code added in #75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes #80287. --- llvm/lib/Target/ARM/ARMFrameLowering.cpp | 11 +++++---- llvm/lib/Target/ARM/ARMFrameLowering.h | 1 + llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp | 23 ++++++++----------- llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll | 11 +++++---- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.cpp b/llvm/lib/Target/ARM/ARMFrameLowering.cpp index eeb7f64aa5810..7f6674934aae3 100644 --- a/llvm/lib/Target/ARM/ARMFrameLowering.cpp +++ b/llvm/lib/Target/ARM/ARMFrameLowering.cpp @@ -2781,10 +2781,7 @@ void ARMFrameLowering::determineCalleeSaves(MachineFunction &MF, AFI->setLRIsSpilled(SavedRegs.test(ARM::LR)); } -void ARMFrameLowering::processFunctionBeforeFrameFinalized( - MachineFunction &MF, RegScavenger *RS) const { - TargetFrameLowering::processFunctionBeforeFrameFinalized(MF, RS); - +void ARMFrameLowering::updateLRRestored(MachineFunction &MF) const { MachineFrameInfo &MFI = MF.getFrameInfo(); if (!MFI.isCalleeSavedInfoValid()) return; @@ -2808,6 +2805,12 @@ void ARMFrameLowering::processFunctionBeforeFrameFinalized( } } +void ARMFrameLowering::processFunctionBeforeFrameFinalized( + MachineFunction &MF, RegScavenger *RS) const { + TargetFrameLowering::processFunctionBeforeFrameFinalized(MF, RS); + updateLRRestored(MF); +} + void ARMFrameLowering::getCalleeSaves(const MachineFunction &MF, BitVector &SavedRegs) const { TargetFrameLowering::getCalleeSaves(MF, SavedRegs); diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.h b/llvm/lib/Target/ARM/ARMFrameLowering.h index 8d2b8beb9a58f..13c48f5dc6174 100644 --- a/llvm/lib/Target/ARM/ARMFrameLowering.h +++ b/llvm/lib/Target/ARM/ARMFrameLowering.h @@ -59,6 +59,7 @@ class ARMFrameLowering : public TargetFrameLowering { void determineCalleeSaves(MachineFunction &MF, BitVector &SavedRegs, RegScavenger *RS) const override; + void updateLRRestored(MachineFunction &MF) const; void processFunctionBeforeFrameFinalized( MachineFunction &MF, RegScavenger *RS = nullptr) const override; diff --git a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp index ed9d30c3c3ab9..5716e022e6711 100644 --- a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp +++ b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp @@ -2062,17 +2062,6 @@ bool ARMLoadStoreOpt::MergeReturnIntoLDM(MachineBasicBlock &MBB) { MO.setReg(ARM::PC); PrevMI.copyImplicitOps(*MBB.getParent(), *MBBI); MBB.erase(MBBI); - // We now restore LR into PC so it is not live-out of the return block - // anymore: Clear the CSI Restored bit. - MachineFrameInfo &MFI = MBB.getParent()->getFrameInfo(); - // CSI should be fixed after PrologEpilog Insertion - assert(MFI.isCalleeSavedInfoValid() && "CSI should be valid"); - for (CalleeSavedInfo &Info : MFI.getCalleeSavedInfo()) { - if (Info.getReg() == ARM::LR) { - Info.setRestored(false); - break; - } - } return true; } } @@ -2120,14 +2109,22 @@ bool ARMLoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) { isThumb2 = AFI->isThumb2Function(); isThumb1 = AFI->isThumbFunction() && !isThumb2; - bool Modified = false; + bool Modified = false, ModifiedLDMReturn = false; for (MachineBasicBlock &MBB : Fn) { Modified |= LoadStoreMultipleOpti(MBB); if (STI->hasV5TOps() && !AFI->shouldSignReturnAddress()) - Modified |= MergeReturnIntoLDM(MBB); + ModifiedLDMReturn |= MergeReturnIntoLDM(MBB); if (isThumb1) Modified |= CombineMovBx(MBB); } + Modified |= ModifiedLDMReturn; + + // If we merged a BX instruction into an LDM, we need to re-calculate whether + // LR is restored. This check needs to consider the whole function, not just + // the instruction(s) we changed, because there may be other BX returns which + // still need LR to be restored. + if (ModifiedLDMReturn) + Fn.getSubtarget().getFrameLowering()->updateLRRestored(Fn); Allocator.DestroyAll(); return Modified; diff --git a/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll b/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll index 883a812139ded..9494880f99025 100644 --- a/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll +++ b/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll @@ -10,16 +10,17 @@ define i32 @foo(ptr %ctx) { ; CHECK: @ %bb.0: @ %entry ; CHECK-NEXT: cbz r0, .LBB0_2 ; CHECK-NEXT: @ %bb.1: @ %if.end +; CHECK-NEXT: movw r12, :lower16:val2 +; CHECK-NEXT: movw r3, :lower16:val1 ; CHECK-NEXT: movw r2, :lower16:val0 ; CHECK-NEXT: mov r1, r0 ; CHECK-NEXT: movs r0, #0 -; CHECK-NEXT: movw r12, :lower16:val2 -; CHECK-NEXT: movw r3, :lower16:val1 -; CHECK-NEXT: movt r2, :upper16:val0 -; CHECK-NEXT: add.w lr, r1, #4 ; CHECK-NEXT: movt r12, :upper16:val2 ; CHECK-NEXT: movt r3, :upper16:val1 -; CHECK-NEXT: stm.w lr, {r2, r3, r12} +; CHECK-NEXT: movt r2, :upper16:val0 +; CHECK-NEXT: str r2, [r1, #4] +; CHECK-NEXT: str r3, [r1, #8] +; CHECK-NEXT: str.w r12, [r1, #12] ; CHECK-NEXT: str r0, [r1, #16] ; CHECK-NEXT: bx lr ; CHECK-NEXT: .LBB0_2: @ %if.then From 5d888bfa4cf25d7d606ef563f376442d1d9bc047 Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Fri, 23 Feb 2024 16:54:30 +0000 Subject: [PATCH 2/3] Add doc comment for updateLRRestored --- llvm/lib/Target/ARM/ARMFrameLowering.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.h b/llvm/lib/Target/ARM/ARMFrameLowering.h index 13c48f5dc6174..b58752b8659fc 100644 --- a/llvm/lib/Target/ARM/ARMFrameLowering.h +++ b/llvm/lib/Target/ARM/ARMFrameLowering.h @@ -59,7 +59,10 @@ class ARMFrameLowering : public TargetFrameLowering { void determineCalleeSaves(MachineFunction &MF, BitVector &SavedRegs, RegScavenger *RS) const override; + /// Update the IsRestored flag on LR if it is spilled, based on the return + /// instructions. void updateLRRestored(MachineFunction &MF) const; + void processFunctionBeforeFrameFinalized( MachineFunction &MF, RegScavenger *RS = nullptr) const override; From 9fafb08d63dd9e806f8a4589bccc307c5c2652ef Mon Sep 17 00:00:00 2001 From: Oliver Stannard Date: Mon, 26 Feb 2024 09:16:47 +0000 Subject: [PATCH 3/3] Make updateLRRestored a static function --- llvm/lib/Target/ARM/ARMFrameLowering.cpp | 2 +- llvm/lib/Target/ARM/ARMFrameLowering.h | 2 +- llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.cpp b/llvm/lib/Target/ARM/ARMFrameLowering.cpp index 7f6674934aae3..9b54dd4e4e618 100644 --- a/llvm/lib/Target/ARM/ARMFrameLowering.cpp +++ b/llvm/lib/Target/ARM/ARMFrameLowering.cpp @@ -2781,7 +2781,7 @@ void ARMFrameLowering::determineCalleeSaves(MachineFunction &MF, AFI->setLRIsSpilled(SavedRegs.test(ARM::LR)); } -void ARMFrameLowering::updateLRRestored(MachineFunction &MF) const { +void ARMFrameLowering::updateLRRestored(MachineFunction &MF) { MachineFrameInfo &MFI = MF.getFrameInfo(); if (!MFI.isCalleeSavedInfoValid()) return; diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.h b/llvm/lib/Target/ARM/ARMFrameLowering.h index b58752b8659fc..3c7358d8cd53e 100644 --- a/llvm/lib/Target/ARM/ARMFrameLowering.h +++ b/llvm/lib/Target/ARM/ARMFrameLowering.h @@ -61,7 +61,7 @@ class ARMFrameLowering : public TargetFrameLowering { /// Update the IsRestored flag on LR if it is spilled, based on the return /// instructions. - void updateLRRestored(MachineFunction &MF) const; + static void updateLRRestored(MachineFunction &MF); void processFunctionBeforeFrameFinalized( MachineFunction &MF, RegScavenger *RS = nullptr) const override; diff --git a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp index 5716e022e6711..6121055eb0217 100644 --- a/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp +++ b/llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp @@ -2124,7 +2124,7 @@ bool ARMLoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) { // the instruction(s) we changed, because there may be other BX returns which // still need LR to be restored. if (ModifiedLDMReturn) - Fn.getSubtarget().getFrameLowering()->updateLRRestored(Fn); + ARMFrameLowering::updateLRRestored(Fn); Allocator.DestroyAll(); return Modified;