[llvm] [ARM] Update IsRestored for LR based on all returns (PR #82745)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 23 08:54:44 PST 2024
https://github.com/ostannard updated https://github.com/llvm/llvm-project/pull/82745
>From 3a73ee453d27dff3b08dab82083ef17372bf47a7 Mon Sep 17 00:00:00 2001
From: Oliver Stannard <oliver.stannard at arm.com>
Date: Fri, 23 Feb 2024 09:31:25 +0000
Subject: [PATCH 1/3] Pre-commit test showing bug #80287
This test shows the bug where LR is used as a general-purpose register
on a code path where it is not spilled to the stack.
---
llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll | 55 +++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
diff --git a/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll b/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
new file mode 100644
index 00000000000000..883a812139ded1
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
@@ -0,0 +1,55 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: llc -mtriple thumbv7a-none-eabi < %s | FileCheck %s
+
+ at val0 = global i32 0, align 4
+ at val1 = global i32 0, align 4
+ at val2 = global i32 0, align 4
+
+define i32 @foo(ptr %ctx) {
+; CHECK-LABEL: foo:
+; CHECK: @ %bb.0: @ %entry
+; CHECK-NEXT: cbz r0, .LBB0_2
+; CHECK-NEXT: @ %bb.1: @ %if.end
+; 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: str r0, [r1, #16]
+; CHECK-NEXT: bx lr
+; CHECK-NEXT: .LBB0_2: @ %if.then
+; CHECK-NEXT: .save {r7, lr}
+; CHECK-NEXT: push {r7, lr}
+; CHECK-NEXT: bl bar
+; CHECK-NEXT: mov.w r0, #-1
+; CHECK-NEXT: pop {r7, pc}
+entry:
+ %tobool.not = icmp eq ptr %ctx, null
+ br i1 %tobool.not, label %if.then, label %if.end
+
+if.then: ; preds = %entry
+ tail call void @bar() #2
+ br label %return
+
+if.end: ; preds = %entry
+ %cmd_a = getelementptr inbounds i8, ptr %ctx, i32 4
+ store ptr @val0, ptr %cmd_a, align 4
+ %cmd_b = getelementptr inbounds i8, ptr %ctx, i32 8
+ store ptr @val1, ptr %cmd_b, align 4
+ %cmd_c = getelementptr inbounds i8, ptr %ctx, i32 12
+ store ptr @val2, ptr %cmd_c, align 4
+ %cmd_d = getelementptr inbounds i8, ptr %ctx, i32 16
+ store ptr null, ptr %cmd_d, align 4
+ br label %return
+
+return: ; preds = %if.end, %if.then
+ %retval.0 = phi i32 [ 0, %if.end ], [ -1, %if.then ]
+ ret i32 %retval.0
+}
+
+declare void @bar()
>From b1d72c26f1117300707b0f9a8624dc7b749baae1 Mon Sep 17 00:00:00 2001
From: Oliver Stannard <oliver.stannard at arm.com>
Date: Fri, 23 Feb 2024 09:31:30 +0000
Subject: [PATCH 2/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 eeb7f64aa5810e..7f6674934aae31 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 8d2b8beb9a58fb..13c48f5dc61743 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 ed9d30c3c3ab90..5716e022e6711d 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<ARMSubtarget>().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 883a812139ded1..9494880f990258 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 594c696ad94a90ab08525a1a14869ae9089f376e Mon Sep 17 00:00:00 2001
From: Oliver Stannard <oliver.stannard at arm.com>
Date: Fri, 23 Feb 2024 16:54:30 +0000
Subject: [PATCH 3/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 13c48f5dc61743..b58752b8659fca 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;
More information about the llvm-commits
mailing list