[llvm] [mcp-frameinst: 3/3]: [MCP] Never eliminate frame-setup/destroy instructions (PR #186237)
Scott Linder via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 16 09:22:22 PDT 2026
https://github.com/slinder1 updated https://github.com/llvm/llvm-project/pull/186237
>From 2f8d3ea375d59368000721ea24e155448749378a Mon Sep 17 00:00:00 2001
From: Scott Linder <Scott.Linder at amd.com>
Date: Thu, 12 Mar 2026 15:26:38 +0000
Subject: [PATCH 1/3] [MCP][NFC] Cleanup and prepare to preserve
frame-setup/destroy
This mixes renames, removing redundant code, avoiding
`else`-after-`return`, etc. with factoring out the `isNeverRedundant`
concept.
Change-Id: I43a62a9415019cdd63c68fd3b915ebb7505d317a
>From 4dd16324158cd9cb329d02a30f1d2f4bd11c5971 Mon Sep 17 00:00:00 2001
From: Scott Linder <Scott.Linder at amd.com>
Date: Thu, 12 Mar 2026 16:29:22 +0000
Subject: [PATCH 2/3] [MCP][NFC] Opinionated refactoring
There are a few minor inconsistencies across the pass which I found mildly
distracting:
* The use of `Def`/`Dest`/`Dst` to refer to the same thing
* Inconsistent declaration order of `Dst`/`Src` vs `Src`/`Dst`
* Lots of `->getReg()->asMCReg()`, and uses of `Register` when the pass
is always running after RA anyway.
* Some places explicitly `assert(isCopyInstr)` while others just deref
the `optional`.
Standardize on `Dst`/`Src` to match the metaphor and ordering of
`DestSourcePair`.
Assume `std::optional::operator*` will assert in any reasonable
implementation, even though this may technically be undefined behavior.
When asserts are disabled it would be anyway.
The refactor uses structured bindings for a couple reasons:
* Naturally enforces consistent order of `Dst`-then-`Src`
* Requires the use of `auto`, which ensures the declaration is not
implicitly converting from `MCRegister` back to `Register`.
In both cases the explicitness of the name `getDstSrcMCRegs` hopefully
makes the meaning at the callsite clear (`Dst, Src = DstSrc`, and
explicitly mentioning `MCReg`).
Change-Id: Ic58f555e03535d726cdad38dbe3f9c6df1b86460
>From 92faf5298f6c45a8b81e02b3bd1bee3dfff9c6c5 Mon Sep 17 00:00:00 2001
From: Scott Linder <Scott.Linder at amd.com>
Date: Thu, 12 Mar 2026 18:08:00 +0000
Subject: [PATCH 3/3] [MCP] Never eliminate frame-setup/destroy instructions
Presumably targets only insert frame instructions which are significant,
and there may be effects MCP doesn't model. Similar to reserved registers this
is probably overly conservative, but as this causes no codegen change in
any lit test I think it is benign.
The motivation is just to clean up #183149 for AMDGPU, as we can spill
to physical registers, and currently have to spill the EXEC mask purely
to enable debug-info.
Change-Id: I9ea4a09b34464c43322edd2900361bf635efd9f7
---
llvm/lib/CodeGen/MachineCopyPropagation.cpp | 16 ++++++++++-----
llvm/test/CodeGen/X86/machine-copy-prop.mir | 22 +++++++++++++++++++++
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index fbd833abc724c..4d2a66f41df93 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -490,6 +490,12 @@ class MachineCopyPropagation {
// zero).
return MRI->isReserved(CopyOperand);
}
+ /// Returns true iff the @p Copy instruction must never be eliminated as
+ /// redundant. This overload does not consider the operands of @p Copy.
+ bool isNeverRedundant(const MachineInstr &Copy) {
+ return Copy.getFlag(MachineInstr::FrameSetup) ||
+ Copy.getFlag(MachineInstr::FrameDestroy);
+ }
bool hasImplicitOverlap(const MachineInstr &MI, const MachineOperand &Use);
bool hasOverlappingMultipleDef(const MachineInstr &MI,
const MachineOperand &MODef, MCRegister Def);
@@ -598,7 +604,7 @@ static bool isNopCopy(const MachineInstr &PreviousCopy, MCRegister Src,
/// copying the super registers.
bool MachineCopyPropagation::eraseIfRedundant(MachineInstr &Copy,
MCRegister Dst, MCRegister Src) {
- if (isNeverRedundant(Src) || isNeverRedundant(Dst))
+ if (isNeverRedundant(Copy) || isNeverRedundant(Src) || isNeverRedundant(Dst))
return false;
// Search for an existing copy.
@@ -659,7 +665,7 @@ bool MachineCopyPropagation::isBackwardPropagatableCopy(
if (!Dst || !Src)
return false;
- if (isNeverRedundant(Dst) || isNeverRedundant(Src))
+ if (isNeverRedundant(Copy) || isNeverRedundant(Dst) || isNeverRedundant(Src))
return false;
return CopyOperands.Source->isRenamable() && CopyOperands.Source->isKill();
@@ -946,7 +952,7 @@ void MachineCopyPropagation::forwardCopyPropagateBlock(MachineBasicBlock &MBB) {
if (!TRI->regsOverlap(Dst, Src)) {
// FIXME: Document why this does not consider `RegSrc`, similar to how
// `backwardCopyPropagateBlock` does.
- if (!isNeverRedundant(Dst))
+ if (!isNeverRedundant(MI) && !isNeverRedundant(Dst))
MaybeDeadCopies.insert(&MI);
}
}
@@ -991,7 +997,7 @@ void MachineCopyPropagation::forwardCopyPropagateBlock(MachineBasicBlock &MBB) {
std::optional<DestSourcePair> CopyOperands =
isCopyInstr(*MaybeDead, *TII, UseCopyInstr);
MCRegister Reg = CopyOperands->Destination->getReg().asMCReg();
- assert(!isNeverRedundant(Reg));
+ assert(!isNeverRedundant(*MaybeDead) && !isNeverRedundant(Reg));
if (!RegMask->clobbersPhysReg(Reg)) {
++DI;
@@ -1059,7 +1065,7 @@ void MachineCopyPropagation::forwardCopyPropagateBlock(MachineBasicBlock &MBB) {
*isCopyInstr(*MaybeDead, *TII, UseCopyInstr);
auto [Dst, Src] = getDstSrcMCRegs(CopyOperands);
- assert(!isNeverRedundant(Dst));
+ assert(!isNeverRedundant(*MaybeDead) && !isNeverRedundant(Dst));
// Update matching debug values, if any.
const auto &DbgUsers = CopyDbgUsers[MaybeDead];
diff --git a/llvm/test/CodeGen/X86/machine-copy-prop.mir b/llvm/test/CodeGen/X86/machine-copy-prop.mir
index f4fed03a75e67..e04d638ef092a 100644
--- a/llvm/test/CodeGen/X86/machine-copy-prop.mir
+++ b/llvm/test/CodeGen/X86/machine-copy-prop.mir
@@ -16,6 +16,8 @@
define void @nocopyprop3() { ret void }
define void @nocopyprop4() { ret void }
define void @nocopyprop5() { ret void }
+ define void @nocopyprop_frame_setup() { ret void }
+ define void @nocopyprop_frame_destroy() { ret void }
...
---
# The second copy is redundant and will be removed, check that we also remove
@@ -253,3 +255,23 @@ body: |
$rip = COPY $rax
$rip = COPY $rax
...
+---
+# Conservatively assume prologue/epilogue instructions may have
+# additional effects.
+# CHECK-LABEL: name: nocopyprop_frame_setup
+# CHECK: bb.0:
+# CHECK-NEXT: $rbx = frame-setup COPY $rax
+name: nocopyprop_frame_setup
+body: |
+ bb.0:
+ $rbx = frame-setup COPY $rax
+...
+---
+# CHECK-LABEL: name: nocopyprop_frame_destroy
+# CHECK: bb.0:
+# CHECK-NEXT: $rbx = frame-destroy COPY $rax
+name: nocopyprop_frame_destroy
+body: |
+ bb.0:
+ $rbx = frame-destroy COPY $rax
+...
More information about the llvm-commits
mailing list