[ARM] Update IsRestored for LR based on all returns #82745
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-arm Author: None (ostannard) Changes[ARM] Update IsRestored for LR based on all returns PR #75527 fixed ARMFrameLowering to set the IsRestored flag for LR based The fix is to factor out the code added in #75527, and also call it from Fixes #80287. Full diff: https://github.com/llvm/llvm-project/pull/82745.diff 4 Files Affected:
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
new file mode 100644
index 00000000000000..9494880f990258
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/ldst-opt-lr-restored.ll
@@ -0,0 +1,56 @@
+; 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
+
+@val0 = global i32 0, align 4
+@val1 = global i32 0, align 4
+@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 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: movt r12, :upper16:val2
+; CHECK-NEXT: movt r3, :upper16:val1
+; 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
+; 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()
|
fhahn
reviewed
Feb 23, 2024
| @@ -59,6 +59,7 @@ class ARMFrameLowering : public TargetFrameLowering { | |||
| void determineCalleeSaves(MachineFunction &MF, BitVector &SavedRegs, | |||
| RegScavenger *RS) const override; | |||
|
|
|||
| void updateLRRestored(MachineFunction &MF) const; | |||
There was a problem hiding this comment.
Does this function still use anything from ARMFrameLowering? If not, could it be made static? Would also be good to document the new dedicated function
There was a problem hiding this comment.
It can't be static because it's used from ARMLoadStoreOptimizer, but I'll add a doc comment for it.
There was a problem hiding this comment.
I might be missing something, but making it a static member of ARMFrameLowering should allow it to be called using ARMFrameLowering::updateLRRestored(Fn); in ARMLoadStoreOptimizer
fhahn
approved these changes
Feb 26, 2024
PR llvm#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 llvm#75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes llvm#80287.
ostannard
force-pushed
the
ldst-opt-lr-restored
branch
from
6cb2f82
to
9fafb08
Compare
|
Pushed the pre-committed test as 8779cf6, and rebased onto it. |
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Feb 27, 2024
PR llvm#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 llvm#75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes llvm#80287. (cherry picked from commit 749384c)
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Mar 23, 2024
PR llvm#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 llvm#75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes llvm#80287. (cherry picked from commit 749384c)
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.