[ARM] Check all terms in emitPopInst when clearing Restored for LR. #75527
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
emitPopInst checks a single function exit MBB. If other paths also exit the function and any of there terminators uses LR implicitly, it is not save to clear the Restored bit. Check all terminators for the function before clearing Restored.
fhahn
requested review from
MatzeB,
ahmedbougacha,
TNorthover,
qcolombet,
kparzysz and
efriedma-quic
|
@llvm/pr-subscribers-backend-arm Author: Florian Hahn (fhahn) ChangesemitPopInst checks a single function exit MBB. If other paths also exit the function and any of there terminators uses LR implicitly, it is not save to clear the Restored bit. Check all terminators for the function before clearing Restored. This fixes a mis-compile in outlined-fn-may-clobber-lr-in-caller.ll Alternative to #73553 Full diff: https://github.com/llvm/llvm-project/pull/75527.diff 4 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMFrameLowering.cpp b/llvm/lib/Target/ARM/ARMFrameLowering.cpp
index a3a71a8ec09a45..79b70cab82f145 100644
--- a/llvm/lib/Target/ARM/ARMFrameLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMFrameLowering.cpp
@@ -1645,9 +1645,21 @@ void ARMFrameLowering::emitPopInst(MachineBasicBlock &MBB,
// Fold the return instruction into the LDM.
DeleteRet = true;
LdmOpc = AFI->isThumbFunction() ? ARM::t2LDMIA_RET : ARM::LDMIA_RET;
- // We 'restore' LR into PC so it is not live out of the return block:
- // Clear Restored bit.
- Info.setRestored(false);
+ // Check if all terminators do not implicitly use LR. Then we can
+ // 'restore' LR into PC so it is not live out of the return block: Clear
+ // Restored bit.
+ if (all_of(MF, [MI](const MachineBasicBlock &MBB) {
+ return all_of(MBB.terminators(), [MI](const MachineInstr &Term) {
+ // MI's terminator is to be re-written, don't check the old
+ // opcode.
+ if (&*MI == &Term)
+ return true;
+ return Term.getOpcode() == ARM::LDMIA_RET ||
+ Term.getOpcode() == ARM::t2LDMIA_RET ||
+ Term.getOpcode() == ARM::tPOP_RET;
+ });
+ }))
+ Info.setRestored(false);
}
// If NoGap is true, pop consecutive registers and then leave the rest
diff --git a/llvm/test/CodeGen/Thumb2/mve-float16regloops.ll b/llvm/test/CodeGen/Thumb2/mve-float16regloops.ll
index 1c95d28b5eed1b..a75f445097f28b 100644
--- a/llvm/test/CodeGen/Thumb2/mve-float16regloops.ll
+++ b/llvm/test/CodeGen/Thumb2/mve-float16regloops.ll
@@ -831,6 +831,7 @@ define void @arm_fir_f32_1_4_mve(ptr nocapture readonly %S, ptr nocapture readon
; CHECK-NEXT: mov r0, r1
; CHECK-NEXT: .LBB15_10: @ %while.end55
; CHECK-NEXT: ands r1, r9, #3
+; CHECK-NEXT: @ implicit-def: $lr
; CHECK-NEXT: beq .LBB15_12
; CHECK-NEXT: @ %bb.11: @ %if.then59
; CHECK-NEXT: vldrw.u32 q0, [r0]
diff --git a/llvm/test/CodeGen/Thumb2/mve-float32regloops.ll b/llvm/test/CodeGen/Thumb2/mve-float32regloops.ll
index 808626d9a0aebe..c29653e6827263 100644
--- a/llvm/test/CodeGen/Thumb2/mve-float32regloops.ll
+++ b/llvm/test/CodeGen/Thumb2/mve-float32regloops.ll
@@ -822,6 +822,7 @@ define void @arm_fir_f32_1_4_mve(ptr nocapture readonly %S, ptr nocapture readon
; CHECK-NEXT: mov r0, r1
; CHECK-NEXT: .LBB15_10: @ %while.end55
; CHECK-NEXT: ands r1, r10, #3
+; CHECK-NEXT: @ implicit-def: $lr
; CHECK-NEXT: beq .LBB15_12
; CHECK-NEXT: @ %bb.11: @ %if.then59
; CHECK-NEXT: vldrw.u32 q0, [r0]
diff --git a/llvm/test/CodeGen/Thumb2/outlined-fn-may-clobber-lr-in-caller.ll b/llvm/test/CodeGen/Thumb2/outlined-fn-may-clobber-lr-in-caller.ll
index d81d008b44bed8..34d93c985e7204 100644
--- a/llvm/test/CodeGen/Thumb2/outlined-fn-may-clobber-lr-in-caller.ll
+++ b/llvm/test/CodeGen/Thumb2/outlined-fn-may-clobber-lr-in-caller.ll
@@ -22,11 +22,19 @@ define void @test(ptr nocapture noundef writeonly %arg, i32 noundef %arg1, i8 no
; CHECK-NEXT: cmp r1, #1
; CHECK-NEXT: bne .LBB0_5
; CHECK-NEXT: @ %bb.2: @ %bb4
-; CHECK-NEXT: bl OUTLINED_FUNCTION_0
+; CHECK-NEXT: movs r1, #1
+; CHECK-NEXT: strb.w r1, [r0, #36]
+; CHECK-NEXT: movs r1, #30
+; CHECK-NEXT: strb.w r1, [r0, #34]
+; CHECK-NEXT: add.w r1, r2, r2, lsl #3
; CHECK-NEXT: ldr r2, .LCPI0_1
; CHECK-NEXT: b .LBB0_4
; CHECK-NEXT: .LBB0_3: @ %bb14
-; CHECK-NEXT: bl OUTLINED_FUNCTION_0
+; CHECK-NEXT: movs r1, #1
+; CHECK-NEXT: strb.w r1, [r0, #36]
+; CHECK-NEXT: movs r1, #30
+; CHECK-NEXT: strb.w r1, [r0, #34]
+; CHECK-NEXT: add.w r1, r2, r2, lsl #3
; CHECK-NEXT: ldr r2, .LCPI0_0
; CHECK-NEXT: .LBB0_4: @ %bb4
; CHECK-NEXT: add.w r1, r2, r1, lsl #2
|
| return true; | ||
| return Term.getOpcode() == ARM::LDMIA_RET || | ||
| Term.getOpcode() == ARM::t2LDMIA_RET || | ||
| Term.getOpcode() == ARM::tPOP_RET; |
There was a problem hiding this comment.
Should we checking for isReturn()? This is going to reject any block containing a branch.
Checking all blocks here is O(N^2) in the number of epilogues; can we move this later somehow?
There's corresponding code in Thumb1FrameLowering.cpp which needs a similar fix.
There was a problem hiding this comment.
Thanks, I added an isReturn check in the latest version and moved setting IsRestored to processFunctionBeforeFrameFinalized, which runs just after spilling the callee-saved registers
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Jan 4, 2024
…lvm#75527) emitPopInst checks a single function exit MBB. If other paths also exit the function and any of there terminators uses LR implicitly, it is not save to clear the Restored bit. Check all terminators for the function before clearing Restored. This fixes a mis-compile in outlined-fn-may-clobber-lr-in-caller.ll where the machine-outliner previously introduced BLs that clobbered LR which in turn is used by the tail call return. Alternative to llvm#73553 (cherry-picked from b1a5ee1)
nikic
pushed a commit
to nikic/llvm-project
that referenced
this pull request
Jan 9, 2024
…lvm#75527) emitPopInst checks a single function exit MBB. If other paths also exit the function and any of there terminators uses LR implicitly, it is not save to clear the Restored bit. Check all terminators for the function before clearing Restored. This fixes a mis-compile in outlined-fn-may-clobber-lr-in-caller.ll where the machine-outliner previously introduced BLs that clobbered LR which in turn is used by the tail call return. Alternative to llvm#73553 (cherry picked from commit b1a5ee1)
ostannard
added a commit
to ostannard/llvm-project
that referenced
this pull request
Feb 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.
ostannard
added a commit
to ostannard/llvm-project
that referenced
this pull request
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
added a commit
that referenced
this pull request
Feb 26, 2024
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.
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)
MingcongBai
pushed a commit
to AOSC-Tracking/llvm-project
that referenced
this pull request
Mar 26, 2024
…lvm#75527) emitPopInst checks a single function exit MBB. If other paths also exit the function and any of there terminators uses LR implicitly, it is not save to clear the Restored bit. Check all terminators for the function before clearing Restored. This fixes a mis-compile in outlined-fn-may-clobber-lr-in-caller.ll where the machine-outliner previously introduced BLs that clobbered LR which in turn is used by the tail call return. Alternative to llvm#73553 (cherry picked from commit b1a5ee1)
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.
emitPopInst checks a single function exit MBB. If other paths also exit the function and any of there terminators uses LR implicitly, it is not save to clear the Restored bit.
Check all terminators for the function before clearing Restored.
This fixes a mis-compile in outlined-fn-may-clobber-lr-in-caller.ll
where the machine-outliner previously introduced BLs that clobbered LR
which in turn is used by the tail call return.
Alternative to #73553