[llvm] [llvm][X86] Fix merging of large sp updates (PR #125007)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 31 07:23:10 PST 2025
https://github.com/macurtis-amd updated https://github.com/llvm/llvm-project/pull/125007
>From 83bb5691c550c4c46a10e373c42153ce0a19fc44 Mon Sep 17 00:00:00 2001
From: Matthew Curtis <macurtis at amd.com>
Date: Wed, 29 Jan 2025 14:31:21 -0600
Subject: [PATCH] [llvm][X86] Fix merging of large sp updates
---
llvm/lib/Target/X86/X86ExpandPseudo.cpp | 4 +-
llvm/lib/Target/X86/X86FrameLowering.cpp | 95 ++++++++++++-------
llvm/lib/Target/X86/X86FrameLowering.h | 35 +++++--
.../test/CodeGen/X86/merge-huge-sp-updates.ll | 31 ++++++
4 files changed, 122 insertions(+), 43 deletions(-)
create mode 100644 llvm/test/CodeGen/X86/merge-huge-sp-updates.ll
diff --git a/llvm/lib/Target/X86/X86ExpandPseudo.cpp b/llvm/lib/Target/X86/X86ExpandPseudo.cpp
index 78db8413e62c9b..c202f7fa93db61 100644
--- a/llvm/lib/Target/X86/X86ExpandPseudo.cpp
+++ b/llvm/lib/Target/X86/X86ExpandPseudo.cpp
@@ -284,7 +284,7 @@ bool X86ExpandPseudo::expandMI(MachineBasicBlock &MBB,
// Adjust stack pointer.
int StackAdj = StackAdjust.getImm();
int MaxTCDelta = X86FI->getTCReturnAddrDelta();
- int Offset = 0;
+ int64_t Offset = 0;
assert(MaxTCDelta <= 0 && "MaxTCDelta should never be positive");
// Incoporate the retaddr area.
@@ -297,7 +297,7 @@ bool X86ExpandPseudo::expandMI(MachineBasicBlock &MBB,
if (Offset) {
// Check for possible merge with preceding ADD instruction.
- Offset += X86FL->mergeSPUpdates(MBB, MBBI, true);
+ Offset = X86FL->mergeSPAdd(MBB, MBBI, Offset, true);
X86FL->emitSPUpdate(MBB, MBBI, DL, Offset, /*InEpilogue=*/true);
}
diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index a15db039a5ed49..05118ea27a3e71 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -223,6 +223,8 @@ flagsNeedToBePreservedBeforeTheTerminators(const MachineBasicBlock &MBB) {
return false;
}
+constexpr int64_t MaxSPChunk = (1LL << 31) - 1;
+
/// emitSPUpdate - Emit a series of instructions to increment / decrement the
/// stack pointer by a constant value.
void X86FrameLowering::emitSPUpdate(MachineBasicBlock &MBB,
@@ -242,7 +244,7 @@ void X86FrameLowering::emitSPUpdate(MachineBasicBlock &MBB,
return;
}
- uint64_t Chunk = (1LL << 31) - 1;
+ uint64_t Chunk = MaxSPChunk;
MachineFunction &MF = *MBB.getParent();
const X86Subtarget &STI = MF.getSubtarget<X86Subtarget>();
@@ -391,12 +393,14 @@ MachineInstrBuilder X86FrameLowering::BuildStackAdjustment(
return MI;
}
-int X86FrameLowering::mergeSPUpdates(MachineBasicBlock &MBB,
- MachineBasicBlock::iterator &MBBI,
- bool doMergeWithPrevious) const {
+template <typename T>
+int64_t X86FrameLowering::mergeSPUpdates(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator &MBBI,
+ T CalcNewOffset,
+ bool doMergeWithPrevious) const {
if ((doMergeWithPrevious && MBBI == MBB.begin()) ||
(!doMergeWithPrevious && MBBI == MBB.end()))
- return 0;
+ return CalcNewOffset(0);
MachineBasicBlock::iterator PI = doMergeWithPrevious ? std::prev(MBBI) : MBBI;
@@ -415,27 +419,37 @@ int X86FrameLowering::mergeSPUpdates(MachineBasicBlock &MBB,
if (doMergeWithPrevious && PI != MBB.begin() && PI->isCFIInstruction())
PI = std::prev(PI);
- unsigned Opc = PI->getOpcode();
- int Offset = 0;
-
- if ((Opc == X86::ADD64ri32 || Opc == X86::ADD32ri) &&
- PI->getOperand(0).getReg() == StackPtr) {
- assert(PI->getOperand(1).getReg() == StackPtr);
- Offset = PI->getOperand(2).getImm();
- } else if ((Opc == X86::LEA32r || Opc == X86::LEA64_32r) &&
- PI->getOperand(0).getReg() == StackPtr &&
- PI->getOperand(1).getReg() == StackPtr &&
- PI->getOperand(2).getImm() == 1 &&
- PI->getOperand(3).getReg() == X86::NoRegister &&
- PI->getOperand(5).getReg() == X86::NoRegister) {
- // For LEAs we have: def = lea SP, FI, noreg, Offset, noreg.
- Offset = PI->getOperand(4).getImm();
- } else if ((Opc == X86::SUB64ri32 || Opc == X86::SUB32ri) &&
- PI->getOperand(0).getReg() == StackPtr) {
- assert(PI->getOperand(1).getReg() == StackPtr);
- Offset = -PI->getOperand(2).getImm();
- } else
- return 0;
+ int64_t Offset = 0;
+ for (;;) {
+ unsigned Opc = PI->getOpcode();
+
+ if ((Opc == X86::ADD64ri32 || Opc == X86::ADD32ri) &&
+ PI->getOperand(0).getReg() == StackPtr) {
+ assert(PI->getOperand(1).getReg() == StackPtr);
+ Offset = PI->getOperand(2).getImm();
+ } else if ((Opc == X86::LEA32r || Opc == X86::LEA64_32r) &&
+ PI->getOperand(0).getReg() == StackPtr &&
+ PI->getOperand(1).getReg() == StackPtr &&
+ PI->getOperand(2).getImm() == 1 &&
+ PI->getOperand(3).getReg() == X86::NoRegister &&
+ PI->getOperand(5).getReg() == X86::NoRegister) {
+ // For LEAs we have: def = lea SP, FI, noreg, Offset, noreg.
+ Offset = PI->getOperand(4).getImm();
+ } else if ((Opc == X86::SUB64ri32 || Opc == X86::SUB32ri) &&
+ PI->getOperand(0).getReg() == StackPtr) {
+ assert(PI->getOperand(1).getReg() == StackPtr);
+ Offset = -PI->getOperand(2).getImm();
+ } else
+ return CalcNewOffset(0);
+
+ if (std::abs((int64_t)CalcNewOffset(Offset)) < MaxSPChunk)
+ break;
+
+ if (doMergeWithPrevious ? (PI == MBB.begin()) : (PI == MBB.end()))
+ return CalcNewOffset(0);
+
+ PI = doMergeWithPrevious ? std::prev(PI) : std::next(PI);
+ }
PI = MBB.erase(PI);
if (PI != MBB.end() && PI->isCFIInstruction()) {
@@ -448,7 +462,16 @@ int X86FrameLowering::mergeSPUpdates(MachineBasicBlock &MBB,
if (!doMergeWithPrevious)
MBBI = skipDebugInstructionsForward(PI, MBB.end());
- return Offset;
+ return CalcNewOffset(Offset);
+}
+
+int64_t X86FrameLowering::mergeSPAdd(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator &MBBI,
+ int64_t AddOffset,
+ bool doMergeWithPrevious) const {
+ return mergeSPUpdates(
+ MBB, MBBI, [AddOffset](int64_t Offset) { return AddOffset + Offset; },
+ doMergeWithPrevious);
}
void X86FrameLowering::BuildCFI(MachineBasicBlock &MBB,
@@ -1975,8 +1998,10 @@ void X86FrameLowering::emitPrologue(MachineFunction &MF,
// If there is an SUB32ri of ESP immediately before this instruction, merge
// the two. This can be the case when tail call elimination is enabled and
- // the callee has more arguments then the caller.
- NumBytes -= mergeSPUpdates(MBB, MBBI, true);
+ // the callee has more arguments than the caller.
+ NumBytes = mergeSPUpdates(
+ MBB, MBBI, [NumBytes](int64_t Offset) { return NumBytes - Offset; },
+ true);
// Adjust stack pointer: ESP -= numbytes.
@@ -2457,7 +2482,7 @@ void X86FrameLowering::emitEpilogue(MachineFunction &MF,
if (HasFP) {
if (X86FI->hasSwiftAsyncContext()) {
// Discard the context.
- int Offset = 16 + mergeSPUpdates(MBB, MBBI, true);
+ int64_t Offset = mergeSPAdd(MBB, MBBI, 16, true);
emitSPUpdate(MBB, MBBI, DL, Offset, /*InEpilogue*/ true);
}
// Pop EBP.
@@ -2531,7 +2556,7 @@ void X86FrameLowering::emitEpilogue(MachineFunction &MF,
// If there is an ADD32ri or SUB32ri of ESP immediately before this
// instruction, merge the two instructions.
if (NumBytes || MFI.hasVarSizedObjects())
- NumBytes += mergeSPUpdates(MBB, MBBI, true);
+ NumBytes = mergeSPAdd(MBB, MBBI, NumBytes, true);
// If dynamic alloca is used, then reset esp to point to the last callee-saved
// slot before popping them off! Same applies for the case, when stack was
@@ -2612,11 +2637,11 @@ void X86FrameLowering::emitEpilogue(MachineFunction &MF,
if (Terminator == MBB.end() || !isTailCallOpcode(Terminator->getOpcode())) {
// Add the return addr area delta back since we are not tail calling.
- int Offset = -1 * X86FI->getTCReturnAddrDelta();
+ int64_t Offset = -1 * X86FI->getTCReturnAddrDelta();
assert(Offset >= 0 && "TCDelta should never be positive");
if (Offset) {
// Check for possible merge with preceding ADD instruction.
- Offset += mergeSPUpdates(MBB, Terminator, true);
+ Offset = mergeSPAdd(MBB, Terminator, Offset, true);
emitSPUpdate(MBB, Terminator, DL, Offset, /*InEpilogue=*/true);
}
}
@@ -3819,8 +3844,8 @@ MachineBasicBlock::iterator X86FrameLowering::eliminateCallFramePseudoInstr(
// Merge with any previous or following adjustment instruction. Note: the
// instructions merged with here do not have CFI, so their stack
// adjustments do not feed into CfaAdjustment.
- StackAdjustment += mergeSPUpdates(MBB, InsertPos, true);
- StackAdjustment += mergeSPUpdates(MBB, InsertPos, false);
+ StackAdjustment = mergeSPAdd(MBB, InsertPos, StackAdjustment, true);
+ StackAdjustment = mergeSPAdd(MBB, InsertPos, StackAdjustment, false);
if (StackAdjustment) {
if (!(F.hasMinSize() &&
diff --git a/llvm/lib/Target/X86/X86FrameLowering.h b/llvm/lib/Target/X86/X86FrameLowering.h
index 02fe8ee02a7e45..193f9deb32465d 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.h
+++ b/llvm/lib/Target/X86/X86FrameLowering.h
@@ -134,12 +134,35 @@ class X86FrameLowering : public TargetFrameLowering {
processFunctionBeforeFrameIndicesReplaced(MachineFunction &MF,
RegScavenger *RS) const override;
- /// Check the instruction before/after the passed instruction. If
- /// it is an ADD/SUB/LEA instruction it is deleted argument and the
- /// stack adjustment is returned as a positive value for ADD/LEA and
- /// a negative for SUB.
- int mergeSPUpdates(MachineBasicBlock &MBB, MachineBasicBlock::iterator &MBBI,
- bool doMergeWithPrevious) const;
+private:
+ /// Basic Pseudocode:
+ /// if (instruction before/after the passed instruction is ADD/SUB/LEA)
+ /// Offset = instruction stack adjustment
+ /// ... positive value for ADD/LEA and negative for SUB
+ /// return CalcNewOffset(Offset)
+ /// else
+ /// return CalcNewOffset(0)
+ ///
+ /// It's possible that the selected instruction is not immediately
+ /// before/after MBBI for large adjustments that have been split into multiple
+ /// instructions.
+ ///
+ /// CalcNewOffset should have the signature:
+ /// int64_t CalcNewOffset(int64_t Offset)
+ template <typename T>
+ int64_t mergeSPUpdates(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator &MBBI, T CalcNewOffset,
+ bool doMergeWithPrevious) const;
+
+public:
+ /// Equivalent to:
+ /// mergeSPUpdates(MBB, MBBI,
+ /// [AddOffset](int64_t Offset) {
+ /// return AddOffset + Offset;
+ /// },
+ /// doMergeWithPrevious);
+ int64_t mergeSPAdd(MachineBasicBlock &MBB, MachineBasicBlock::iterator &MBBI,
+ int64_t AddOffset, bool doMergeWithPrevious) const;
/// Emit a series of instructions to increment / decrement the stack
/// pointer by a constant value.
diff --git a/llvm/test/CodeGen/X86/merge-huge-sp-updates.ll b/llvm/test/CodeGen/X86/merge-huge-sp-updates.ll
new file mode 100644
index 00000000000000..c63935b63df9eb
--- /dev/null
+++ b/llvm/test/CodeGen/X86/merge-huge-sp-updates.ll
@@ -0,0 +1,31 @@
+; RUN: llc < %s -O3 -mtriple=x86_64-linux-unknown -verify-machineinstrs -o %t.s
+; RUN: FileCheck --input-file=%t.s %s
+
+; Double-check that we are able to assemble the generated '.s'. A symptom of the
+; problem that led to this test is an assembler failure when using
+; '-save-temps'. For example:
+;
+; > ...s:683:7: error: invalid operand for instruction
+; > addq $2147483679, %rsp # imm = 0x8000001F
+;
+; RUN: llvm-mc -triple x86_64-unknown-unknown %t.s
+
+; Check that the stack update after calling bar gets merged into the second add
+; and not the first which is already at the chunk size limit (0x7FFFFFFF).
+
+define void @foo(ptr %rhs) {
+; CHECK-LABEL: foo
+entry:
+ %lhs = alloca [5 x [5 x [3 x [162 x [161 x [161 x double]]]]]], align 16
+ store ptr %lhs, ptr %rhs, align 8
+ %0 = call i32 @baz()
+ call void @bar(i64 0, i64 0, i64 0, i64 0, i64 0, ptr null, ptr %rhs, ptr null, ptr %rhs)
+; CHECK: call{{.*}}bar
+; CHECK: addq{{.*}}$2147483647, %rsp
+; CHECK: addq{{.*}}$372037585, %rsp
+ ret void
+}
+
+declare void @bar(i64, i64, i64, i64, i64, ptr, ptr, ptr, ptr)
+
+declare i32 @baz()
More information about the llvm-commits
mailing list