[llvm] [CodeGen] Distinguish zero-sized call frames from no call frame in MachineBB (PR #106964)
Fabian Ritter via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 5 03:42:52 PDT 2024
https://github.com/ritter-x2a updated https://github.com/llvm/llvm-project/pull/106964
>From 79c40aa7a7d05546410870ac49f5d81a11f0bff2 Mon Sep 17 00:00:00 2001
From: Fabian Ritter <fabian.ritter at amd.com>
Date: Mon, 2 Sep 2024 05:33:07 -0400
Subject: [PATCH 1/3] [CodeGen] Distinguish zero-sized call frames from no call
frame in MachineBB
Call frames can be zero-sized. Distinguishing between instructions in a
zero-sized call frame and instructions outside of call frames via the
call frame size info stored in the MachineBBs is helpful to avoid nested
CALLSEQs (which are illegal and cause machine verifier errors).
This patch uses std::optional<unsigned> instead of unsigned to represent
call frame sizes. Locations outside of call frames are marked with
std::nullopt whereas locations in zero-sized call-frames are represented
with a std::optional that contains 0.
---
llvm/include/llvm/CodeGen/MachineBasicBlock.h | 19 +++++++++++-----
llvm/include/llvm/CodeGen/TargetInstrInfo.h | 5 +++--
llvm/lib/CodeGen/MIRParser/MIParser.cpp | 6 ++---
llvm/lib/CodeGen/MachineBasicBlock.cpp | 4 ++--
llvm/lib/CodeGen/MachineVerifier.cpp | 4 ++--
llvm/lib/CodeGen/PrologEpilogInserter.cpp | 20 ++++++++---------
llvm/lib/CodeGen/TargetInstrInfo.cpp | 6 ++---
llvm/lib/Target/ARM/ARMISelLowering.cpp | 4 ++--
llvm/lib/Target/AVR/AVRISelLowering.cpp | 2 +-
llvm/lib/Target/M68k/M68kISelLowering.cpp | 2 +-
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | 2 +-
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | 3 ++-
llvm/lib/Target/X86/X86ISelLowering.cpp | 2 +-
llvm/test/CodeGen/MIR/ARM/call-frame-size.mir | 22 +++++++++++++++++++
14 files changed, 67 insertions(+), 34 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
index 6efb17c55493a9..cbc2c66d8bb5d8 100644
--- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h
@@ -146,12 +146,13 @@ class MachineBasicBlock
int Number;
/// The call frame size on entry to this basic block due to call frame setup
- /// instructions in a predecessor. This is usually zero, unless basic blocks
- /// are split in the middle of a call sequence.
+ /// instructions in a predecessor. Usually does not contain a value, unless
+ /// basic blocks are split in the middle of a call sequence. Zero-sized call
+ /// frames are possible.
///
/// This information is only maintained until PrologEpilogInserter eliminates
/// call frame pseudos.
- unsigned CallFrameSize = 0;
+ std::optional<unsigned> CallFrameSize;
MachineFunction *xParent;
Instructions Insts;
@@ -1210,9 +1211,17 @@ class MachineBasicBlock
void setNumber(int N) { Number = N; }
/// Return the call frame size on entry to this basic block.
- unsigned getCallFrameSize() const { return CallFrameSize; }
+ std::optional<unsigned> getCallFrameSize() const { return CallFrameSize; }
+
+ /// Return the call frame size on entry to this basic block if it is part of a
+ /// call sequence and 0 otherwise.
+ unsigned getCallFrameSizeOrZero() const { return CallFrameSize.value_or(0); }
+
/// Set the call frame size on entry to this basic block.
- void setCallFrameSize(unsigned N) { CallFrameSize = N; }
+ void setCallFrameSize(std::optional<unsigned> N) { CallFrameSize = N; }
+
+ /// Reset the stored call frame size.
+ void clearCallFrameSize() { CallFrameSize.reset(); }
/// Return the MCSymbol for this basic block.
MCSymbol *getSymbol() const;
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 49ce13dd8cbe39..0ae3a0ca53757a 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -2265,8 +2265,9 @@ class TargetInstrInfo : public MCInstrInfo {
return false;
}
- // Get the call frame size just before MI.
- unsigned getCallFrameSizeAt(MachineInstr &MI) const;
+ /// Get the call frame size just before MI. Contains no value if MI is not in
+ /// a call sequence. Zero-sized call frames are possible.
+ std::optional<unsigned> getCallFrameSizeAt(MachineInstr &MI) const;
/// Fills in the necessary MachineOperands to refer to a frame index.
/// The best way to understand this is to print `asm(""::"m"(x));` after
diff --git a/llvm/lib/CodeGen/MIRParser/MIParser.cpp b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
index 47b220172602d4..4295956b2a5cc9 100644
--- a/llvm/lib/CodeGen/MIRParser/MIParser.cpp
+++ b/llvm/lib/CodeGen/MIRParser/MIParser.cpp
@@ -502,7 +502,7 @@ class MIParser {
bool parseAddrspace(unsigned &Addrspace);
bool parseSectionID(std::optional<MBBSectionID> &SID);
bool parseBBID(std::optional<UniqueBBID> &BBID);
- bool parseCallFrameSize(unsigned &CallFrameSize);
+ bool parseCallFrameSize(std::optional<unsigned> &CallFrameSize);
bool parseOperandsOffset(MachineOperand &Op);
bool parseIRValue(const Value *&V);
bool parseMemoryOperandFlag(MachineMemOperand::Flags &Flags);
@@ -685,7 +685,7 @@ bool MIParser::parseBBID(std::optional<UniqueBBID> &BBID) {
}
// Parse basic block call frame size.
-bool MIParser::parseCallFrameSize(unsigned &CallFrameSize) {
+bool MIParser::parseCallFrameSize(std::optional<unsigned> &CallFrameSize) {
assert(Token.is(MIToken::kw_call_frame_size));
lex();
unsigned Value = 0;
@@ -713,7 +713,7 @@ bool MIParser::parseBasicBlockDefinition(
std::optional<MBBSectionID> SectionID;
uint64_t Alignment = 0;
std::optional<UniqueBBID> BBID;
- unsigned CallFrameSize = 0;
+ std::optional<unsigned> CallFrameSize;
BasicBlock *BB = nullptr;
if (consumeIfPresent(MIToken::lparen)) {
do {
diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp
index 5d06af3ebf3360..a948efb41d7152 100644
--- a/llvm/lib/CodeGen/MachineBasicBlock.cpp
+++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp
@@ -578,9 +578,9 @@ void MachineBasicBlock::printName(raw_ostream &os, unsigned printNameFlags,
os << " " << getBBID()->CloneID;
hasAttributes = true;
}
- if (CallFrameSize != 0) {
+ if (CallFrameSize.has_value()) {
os << (hasAttributes ? ", " : " (");
- os << "call-frame-size " << CallFrameSize;
+ os << "call-frame-size " << CallFrameSize.value();
hasAttributes = true;
}
}
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 759201ed9dadc7..62ece9ef54b58b 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -3837,11 +3837,11 @@ void MachineVerifier::verifyStackFrame() {
BBState.ExitIsSetup = BBState.EntryIsSetup;
}
- if ((int)MBB->getCallFrameSize() != -BBState.EntryValue) {
+ if ((int)MBB->getCallFrameSizeOrZero() != -BBState.EntryValue) {
report("Call frame size on entry does not match value computed from "
"predecessor",
MBB);
- errs() << "Call frame size on entry " << MBB->getCallFrameSize()
+ errs() << "Call frame size on entry " << MBB->getCallFrameSizeOrZero()
<< " does not match value computed from predecessor "
<< -BBState.EntryValue << '\n';
}
diff --git a/llvm/lib/CodeGen/PrologEpilogInserter.cpp b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
index ee03eaa8ae527c..b2f355c0aa8ccf 100644
--- a/llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ b/llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -392,9 +392,9 @@ void PEI::calculateCallFrameInfo(MachineFunction &MF) {
TFI->eliminateCallFramePseudoInstr(MF, *I->getParent(), I);
// We can't track the call frame size after call frame pseudos have been
- // eliminated. Set it to zero everywhere to keep MachineVerifier happy.
+ // eliminated. Clear it everywhere to keep MachineVerifier happy.
for (MachineBasicBlock &MBB : MF)
- MBB.setCallFrameSize(0);
+ MBB.clearCallFrameSize();
}
}
@@ -1350,11 +1350,11 @@ void PEI::replaceFrameIndicesBackward(MachineFunction &MF) {
// Get the SP adjustment for the end of MBB from the start of any of its
// successors. They should all be the same.
assert(all_of(MBB.successors(), [&MBB](const MachineBasicBlock *Succ) {
- return Succ->getCallFrameSize() ==
- (*MBB.succ_begin())->getCallFrameSize();
+ return Succ->getCallFrameSizeOrZero() ==
+ (*MBB.succ_begin())->getCallFrameSizeOrZero();
}));
const MachineBasicBlock &FirstSucc = **MBB.succ_begin();
- SPAdj = TFI.alignSPAdjust(FirstSucc.getCallFrameSize());
+ SPAdj = TFI.alignSPAdjust(FirstSucc.getCallFrameSizeOrZero());
if (TFI.getStackGrowthDirection() == TargetFrameLowering::StackGrowsUp)
SPAdj = -SPAdj;
}
@@ -1362,8 +1362,8 @@ void PEI::replaceFrameIndicesBackward(MachineFunction &MF) {
replaceFrameIndicesBackward(&MBB, MF, SPAdj);
// We can't track the call frame size after call frame pseudos have been
- // eliminated. Set it to zero everywhere to keep MachineVerifier happy.
- MBB.setCallFrameSize(0);
+ // eliminated. Clear it everywhere to keep MachineVerifier happy.
+ MBB.clearCallFrameSize();
}
}
@@ -1373,15 +1373,15 @@ void PEI::replaceFrameIndices(MachineFunction &MF) {
const TargetFrameLowering &TFI = *MF.getSubtarget().getFrameLowering();
for (auto &MBB : MF) {
- int SPAdj = TFI.alignSPAdjust(MBB.getCallFrameSize());
+ int SPAdj = TFI.alignSPAdjust(MBB.getCallFrameSizeOrZero());
if (TFI.getStackGrowthDirection() == TargetFrameLowering::StackGrowsUp)
SPAdj = -SPAdj;
replaceFrameIndices(&MBB, MF, SPAdj);
// We can't track the call frame size after call frame pseudos have been
- // eliminated. Set it to zero everywhere to keep MachineVerifier happy.
- MBB.setCallFrameSize(0);
+ // eliminated. Clear it everywhere to keep MachineVerifier happy.
+ MBB.clearCallFrameSize();
}
}
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 38bd0b0ba4114c..f623ba0352de8b 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -1624,15 +1624,15 @@ TargetInstrInfo::describeLoadedValue(const MachineInstr &MI,
return std::nullopt;
}
-// Get the call frame size just before MI.
-unsigned TargetInstrInfo::getCallFrameSizeAt(MachineInstr &MI) const {
+std::optional<unsigned>
+TargetInstrInfo::getCallFrameSizeAt(MachineInstr &MI) const {
// Search backwards from MI for the most recent call frame instruction.
MachineBasicBlock *MBB = MI.getParent();
for (auto &AdjI : reverse(make_range(MBB->instr_begin(), MI.getIterator()))) {
if (AdjI.getOpcode() == getCallFrameSetupOpcode())
return getFrameTotalSize(AdjI);
if (AdjI.getOpcode() == getCallFrameDestroyOpcode())
- return 0;
+ return std::nullopt;
}
// If none was found, use the call frame size from the start of the basic
diff --git a/llvm/lib/Target/ARM/ARMISelLowering.cpp b/llvm/lib/Target/ARM/ARMISelLowering.cpp
index 9096617a948557..e2c57d8b9ac524 100644
--- a/llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ b/llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -11594,7 +11594,7 @@ ARMTargetLowering::EmitStructByval(MachineInstr &MI,
MF->insert(It, exitMBB);
// Set the call frame size on entry to the new basic blocks.
- unsigned CallFrameSize = TII->getCallFrameSizeAt(MI);
+ std::optional<unsigned> CallFrameSize = TII->getCallFrameSizeAt(MI);
loopMBB->setCallFrameSize(CallFrameSize);
exitMBB->setCallFrameSize(CallFrameSize);
@@ -12195,7 +12195,7 @@ ARMTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
F->insert(It, sinkMBB);
// Set the call frame size on entry to the new basic blocks.
- unsigned CallFrameSize = TII->getCallFrameSizeAt(MI);
+ std::optional<unsigned> CallFrameSize = TII->getCallFrameSizeAt(MI);
copy0MBB->setCallFrameSize(CallFrameSize);
sinkMBB->setCallFrameSize(CallFrameSize);
diff --git a/llvm/lib/Target/AVR/AVRISelLowering.cpp b/llvm/lib/Target/AVR/AVRISelLowering.cpp
index 0046e757f4efa2..cf6f026869da80 100644
--- a/llvm/lib/Target/AVR/AVRISelLowering.cpp
+++ b/llvm/lib/Target/AVR/AVRISelLowering.cpp
@@ -2424,7 +2424,7 @@ AVRTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
MF->insert(I, falseMBB);
// Set the call frame size on entry to the new basic blocks.
- unsigned CallFrameSize = TII.getCallFrameSizeAt(MI);
+ std::optional<unsigned> CallFrameSize = TII.getCallFrameSizeAt(MI);
trueMBB->setCallFrameSize(CallFrameSize);
falseMBB->setCallFrameSize(CallFrameSize);
diff --git a/llvm/lib/Target/M68k/M68kISelLowering.cpp b/llvm/lib/Target/M68k/M68kISelLowering.cpp
index 316a6eebc2db0f..3b0a0c83d94d16 100644
--- a/llvm/lib/Target/M68k/M68kISelLowering.cpp
+++ b/llvm/lib/Target/M68k/M68kISelLowering.cpp
@@ -3199,7 +3199,7 @@ M68kTargetLowering::EmitLoweredSelect(MachineInstr &MI,
F->insert(It, SinkMBB);
// Set the call frame size on entry to the new basic blocks.
- unsigned CallFrameSize = TII->getCallFrameSizeAt(MI);
+ std::optional<unsigned> CallFrameSize = TII->getCallFrameSizeAt(MI);
Copy0MBB->setCallFrameSize(CallFrameSize);
SinkMBB->setCallFrameSize(CallFrameSize);
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index 8a0858e2462520..f56443770095c7 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -13031,7 +13031,7 @@ PPCTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
// Set the call frame size on entry to the new basic blocks.
// See https://reviews.llvm.org/D156113.
- unsigned CallFrameSize = TII->getCallFrameSizeAt(MI);
+ std::optional<unsigned> CallFrameSize = TII->getCallFrameSizeAt(MI);
copy0MBB->setCallFrameSize(CallFrameSize);
sinkMBB->setCallFrameSize(CallFrameSize);
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index d02078372b24a2..7fcc624e834b9e 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -18444,7 +18444,8 @@ static MachineBasicBlock *emitSelectPseudo(MachineInstr &MI,
F->insert(I, TailMBB);
// Set the call frame size on entry to the new basic blocks.
- unsigned CallFrameSize = TII.getCallFrameSizeAt(*LastSelectPseudo);
+ std::optional<unsigned> CallFrameSize =
+ TII.getCallFrameSizeAt(*LastSelectPseudo);
IfFalseMBB->setCallFrameSize(CallFrameSize);
TailMBB->setCallFrameSize(CallFrameSize);
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index f011249d295040..bbee0af109c74b 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -35264,7 +35264,7 @@ X86TargetLowering::EmitLoweredSelect(MachineInstr &MI,
F->insert(It, SinkMBB);
// Set the call frame size on entry to the new basic blocks.
- unsigned CallFrameSize = TII->getCallFrameSizeAt(MI);
+ std::optional<unsigned> CallFrameSize = TII->getCallFrameSizeAt(MI);
FalseMBB->setCallFrameSize(CallFrameSize);
SinkMBB->setCallFrameSize(CallFrameSize);
diff --git a/llvm/test/CodeGen/MIR/ARM/call-frame-size.mir b/llvm/test/CodeGen/MIR/ARM/call-frame-size.mir
index d3e87120693cf3..58e7f50e583455 100644
--- a/llvm/test/CodeGen/MIR/ARM/call-frame-size.mir
+++ b/llvm/test/CodeGen/MIR/ARM/call-frame-size.mir
@@ -23,3 +23,25 @@ body: |
ADJCALLSTACKUP 100, 0, 14, $noreg, implicit-def $sp, implicit $sp
bb.2:
...
+
+---
+name: callframesizezero
+body: |
+ ; CHECK-LABEL: name: callframesizezero
+ ; CHECK: bb.0:
+ ; CHECK-NEXT: successors: %bb.1(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: ADJCALLSTACKDOWN 0, 0, 14 /* CC::al */, $noreg, implicit-def $sp, implicit $sp
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.1 (call-frame-size 0):
+ ; CHECK-NEXT: successors: %bb.2(0x80000000)
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: ADJCALLSTACKUP 0, 0, 14 /* CC::al */, $noreg, implicit-def $sp, implicit $sp
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: bb.2:
+ bb.0:
+ ADJCALLSTACKDOWN 0, 0, 14, $noreg, implicit-def $sp, implicit $sp
+ bb.1 (call-frame-size 0):
+ ADJCALLSTACKUP 0, 0, 14, $noreg, implicit-def $sp, implicit $sp
+ bb.2:
+...
>From 60543c0b46fd6d6db87d3252cb5a10757965d863 Mon Sep 17 00:00:00 2001
From: Fabian Ritter <fabian.ritter at amd.com>
Date: Thu, 5 Sep 2024 04:34:26 -0400
Subject: [PATCH 2/3] Use std::optional in the MachineVerifier to distinguish
zero-sized from no call frames.
---
llvm/lib/CodeGen/MachineVerifier.cpp | 95 ++++++++++++----------------
1 file changed, 41 insertions(+), 54 deletions(-)
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 62ece9ef54b58b..f0567cc500f1de 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -3786,22 +3786,18 @@ void MachineVerifier::verifyLiveInterval(const LiveInterval &LI) {
namespace {
- // FrameSetup and FrameDestroy can have zero adjustment, so using a single
- // integer, we can't tell whether it is a FrameSetup or FrameDestroy if the
- // value is zero.
- // We use a bool plus an integer to capture the stack state.
- struct StackStateOfBB {
- StackStateOfBB() = default;
- StackStateOfBB(int EntryVal, int ExitVal, bool EntrySetup, bool ExitSetup) :
- EntryValue(EntryVal), ExitValue(ExitVal), EntryIsSetup(EntrySetup),
- ExitIsSetup(ExitSetup) {}
-
- // Can be negative, which means we are setting up a frame.
- int EntryValue = 0;
- int ExitValue = 0;
- bool EntryIsSetup = false;
- bool ExitIsSetup = false;
- };
+/// Store for each MachineBasicBlock the call frame size at its entry and its
+/// exit. No value means that no call frame is open, zero means that a
+/// zero-sized call frame is open.
+struct StackStateOfBB {
+ StackStateOfBB() = default;
+ StackStateOfBB(std::optional<unsigned> EntryVal,
+ std::optional<unsigned> ExitVal)
+ : Entry(EntryVal), Exit(ExitVal) {}
+
+ std::optional<unsigned> Entry;
+ std::optional<unsigned> Exit;
+};
} // end anonymous namespace
@@ -3809,19 +3805,20 @@ namespace {
/// by a FrameDestroy <n>, stack adjustments are identical on all
/// CFG edges to a merge point, and frame is destroyed at end of a return block.
void MachineVerifier::verifyStackFrame() {
- unsigned FrameSetupOpcode = TII->getCallFrameSetupOpcode();
+ unsigned FrameSetupOpcode = TII->getCallFrameSetupOpcode();
unsigned FrameDestroyOpcode = TII->getCallFrameDestroyOpcode();
if (FrameSetupOpcode == ~0u && FrameDestroyOpcode == ~0u)
return;
SmallVector<StackStateOfBB, 8> SPState;
SPState.resize(MF->getNumBlockIDs());
- df_iterator_default_set<const MachineBasicBlock*> Reachable;
+ df_iterator_default_set<const MachineBasicBlock *> Reachable;
// Visit the MBBs in DFS order.
for (df_ext_iterator<const MachineFunction *,
df_iterator_default_set<const MachineBasicBlock *>>
- DFI = df_ext_begin(MF, Reachable), DFE = df_ext_end(MF, Reachable);
+ DFI = df_ext_begin(MF, Reachable),
+ DFE = df_ext_end(MF, Reachable);
DFI != DFE; ++DFI) {
const MachineBasicBlock *MBB = *DFI;
@@ -3831,49 +3828,45 @@ void MachineVerifier::verifyStackFrame() {
const MachineBasicBlock *StackPred = DFI.getPath(DFI.getPathLength() - 2);
assert(Reachable.count(StackPred) &&
"DFS stack predecessor is already visited.\n");
- BBState.EntryValue = SPState[StackPred->getNumber()].ExitValue;
- BBState.EntryIsSetup = SPState[StackPred->getNumber()].ExitIsSetup;
- BBState.ExitValue = BBState.EntryValue;
- BBState.ExitIsSetup = BBState.EntryIsSetup;
+ BBState.Entry = SPState[StackPred->getNumber()].Exit;
+ BBState.Exit = BBState.Entry;
}
- if ((int)MBB->getCallFrameSizeOrZero() != -BBState.EntryValue) {
+ if (MBB->getCallFrameSize() != BBState.Entry) {
report("Call frame size on entry does not match value computed from "
"predecessor",
MBB);
- errs() << "Call frame size on entry " << MBB->getCallFrameSizeOrZero()
+ errs() << "Call frame size on entry " << MBB->getCallFrameSize()
<< " does not match value computed from predecessor "
- << -BBState.EntryValue << '\n';
+ << BBState.Entry << '\n';
}
// Update stack state by checking contents of MBB.
for (const auto &I : *MBB) {
if (I.getOpcode() == FrameSetupOpcode) {
- if (BBState.ExitIsSetup)
+ if (BBState.Exit.has_value())
report("FrameSetup is after another FrameSetup", &I);
if (!MRI->isSSA() && !MF->getFrameInfo().adjustsStack())
report("AdjustsStack not set in presence of a frame pseudo "
- "instruction.", &I);
- BBState.ExitValue -= TII->getFrameTotalSize(I);
- BBState.ExitIsSetup = true;
+ "instruction.",
+ &I);
+ BBState.Exit = TII->getFrameTotalSize(I);
}
if (I.getOpcode() == FrameDestroyOpcode) {
- int Size = TII->getFrameTotalSize(I);
- if (!BBState.ExitIsSetup)
+ int64_t Size = TII->getFrameTotalSize(I);
+ if (!BBState.Exit.has_value())
report("FrameDestroy is not after a FrameSetup", &I);
- int AbsSPAdj = BBState.ExitValue < 0 ? -BBState.ExitValue :
- BBState.ExitValue;
- if (BBState.ExitIsSetup && AbsSPAdj != Size) {
+ else if ((int64_t)BBState.Exit.value() != Size) {
report("FrameDestroy <n> is after FrameSetup <m>", &I);
errs() << "FrameDestroy <" << Size << "> is after FrameSetup <"
- << AbsSPAdj << ">.\n";
+ << BBState.Exit.value() << ">.\n";
}
if (!MRI->isSSA() && !MF->getFrameInfo().adjustsStack())
report("AdjustsStack not set in presence of a frame pseudo "
- "instruction.", &I);
- BBState.ExitValue += Size;
- BBState.ExitIsSetup = false;
+ "instruction.",
+ &I);
+ BBState.Exit.reset();
}
}
SPState[MBB->getNumber()] = BBState;
@@ -3882,14 +3875,12 @@ void MachineVerifier::verifyStackFrame() {
// state.
for (const MachineBasicBlock *Pred : MBB->predecessors()) {
if (Reachable.count(Pred) &&
- (SPState[Pred->getNumber()].ExitValue != BBState.EntryValue ||
- SPState[Pred->getNumber()].ExitIsSetup != BBState.EntryIsSetup)) {
+ SPState[Pred->getNumber()].Exit != BBState.Entry) {
report("The exit stack state of a predecessor is inconsistent.", MBB);
errs() << "Predecessor " << printMBBReference(*Pred)
- << " has exit state (" << SPState[Pred->getNumber()].ExitValue
- << ", " << SPState[Pred->getNumber()].ExitIsSetup << "), while "
- << printMBBReference(*MBB) << " has entry state ("
- << BBState.EntryValue << ", " << BBState.EntryIsSetup << ").\n";
+ << " has exit state " << SPState[Pred->getNumber()].Exit
+ << ", while " << printMBBReference(*MBB) << " has entry state "
+ << BBState.Entry << ".\n";
}
}
@@ -3897,23 +3888,19 @@ void MachineVerifier::verifyStackFrame() {
// state.
for (const MachineBasicBlock *Succ : MBB->successors()) {
if (Reachable.count(Succ) &&
- (SPState[Succ->getNumber()].EntryValue != BBState.ExitValue ||
- SPState[Succ->getNumber()].EntryIsSetup != BBState.ExitIsSetup)) {
+ SPState[Succ->getNumber()].Entry != BBState.Exit) {
report("The entry stack state of a successor is inconsistent.", MBB);
errs() << "Successor " << printMBBReference(*Succ)
- << " has entry state (" << SPState[Succ->getNumber()].EntryValue
- << ", " << SPState[Succ->getNumber()].EntryIsSetup << "), while "
- << printMBBReference(*MBB) << " has exit state ("
- << BBState.ExitValue << ", " << BBState.ExitIsSetup << ").\n";
+ << " has entry state " << SPState[Succ->getNumber()].Entry
+ << ", while " << printMBBReference(*MBB) << " has exit state "
+ << BBState.Exit << ".\n";
}
}
// Make sure a basic block with return ends with zero stack adjustment.
if (!MBB->empty() && MBB->back().isReturn()) {
- if (BBState.ExitIsSetup)
+ if (BBState.Exit.has_value())
report("A return block ends with a FrameSetup.", MBB);
- if (BBState.ExitValue)
- report("A return block ends with a nonzero stack adjustment.", MBB);
}
}
}
>From 9aead99a484eeecaa02baf6accc820847d3714aa Mon Sep 17 00:00:00 2001
From: Fabian Ritter <fabian.ritter at amd.com>
Date: Thu, 5 Sep 2024 04:36:25 -0400
Subject: [PATCH 3/3] Catch some cases where the call frame size stored in
MachineBasicBlocks was not set or updated.
---
llvm/include/llvm/CodeGen/TargetInstrInfo.h | 6 +++++
llvm/lib/CodeGen/TargetInstrInfo.cpp | 12 ++++++---
.../Target/AMDGPU/AMDGPURegisterBankInfo.cpp | 9 +++++++
llvm/lib/Target/X86/X86FrameLowering.cpp | 26 +++++++++++++++++++
llvm/lib/Target/X86/X86ISelLowering.cpp | 10 +++++++
5 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 0ae3a0ca53757a..1960b0750707db 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -2269,6 +2269,12 @@ class TargetInstrInfo : public MCInstrInfo {
/// a call sequence. Zero-sized call frames are possible.
std::optional<unsigned> getCallFrameSizeAt(MachineInstr &MI) const;
+ /// Get the call frame size just before MII. Contains no value if MII is not
+ /// in a call sequence. Zero-sized call frames are possible.
+ std::optional<unsigned>
+ getCallFrameSizeAt(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator MII) const;
+
/// Fills in the necessary MachineOperands to refer to a frame index.
/// The best way to understand this is to print `asm(""::"m"(x));` after
/// finalize-isel. Example:
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index f623ba0352de8b..9318e684fffd96 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -13,6 +13,7 @@
#include "llvm/CodeGen/TargetInstrInfo.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/CodeGen/MachineBasicBlock.h"
#include "llvm/CodeGen/MachineCombinerPattern.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
#include "llvm/CodeGen/MachineInstrBuilder.h"
@@ -1626,9 +1627,14 @@ TargetInstrInfo::describeLoadedValue(const MachineInstr &MI,
std::optional<unsigned>
TargetInstrInfo::getCallFrameSizeAt(MachineInstr &MI) const {
+ return this->getCallFrameSizeAt(*MI.getParent(), MI.getIterator());
+}
+
+std::optional<unsigned>
+TargetInstrInfo::getCallFrameSizeAt(MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator MII) const {
// Search backwards from MI for the most recent call frame instruction.
- MachineBasicBlock *MBB = MI.getParent();
- for (auto &AdjI : reverse(make_range(MBB->instr_begin(), MI.getIterator()))) {
+ for (auto &AdjI : reverse(make_range(MBB.begin(), MII))) {
if (AdjI.getOpcode() == getCallFrameSetupOpcode())
return getFrameTotalSize(AdjI);
if (AdjI.getOpcode() == getCallFrameDestroyOpcode())
@@ -1637,7 +1643,7 @@ TargetInstrInfo::getCallFrameSizeAt(MachineInstr &MI) const {
// If none was found, use the call frame size from the start of the basic
// block.
- return MBB->getCallFrameSize();
+ return MBB.getCallFrameSize();
}
/// Both DefMI and UseMI must be valid. By default, call directly to the
diff --git a/llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp b/llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
index 4737a322c255f4..4ba44587906995 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
@@ -816,6 +816,15 @@ bool AMDGPURegisterBankInfo::executeInWaterfallLoop(
MachineBasicBlock *BodyBB = MF->CreateMachineBasicBlock();
MachineBasicBlock *RemainderBB = MF->CreateMachineBasicBlock();
MachineBasicBlock *RestoreExecBB = MF->CreateMachineBasicBlock();
+
+ // Set call-frame sizes in each new BB, in case we are in a call sequence.
+ std::optional<unsigned> CallFrameSizeBefore =
+ TII->getCallFrameSizeAt(MBB, Range.begin());
+ LoopBB->setCallFrameSize(CallFrameSizeBefore);
+ BodyBB->setCallFrameSize(CallFrameSizeBefore);
+ RemainderBB->setCallFrameSize(CallFrameSizeBefore);
+ RestoreExecBB->setCallFrameSize(CallFrameSizeBefore);
+
MachineFunction::iterator MBBI(MBB);
++MBBI;
MF->insert(MBBI, LoopBB);
diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index e7c725ded4bdef..4fa413e36d8760 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -790,6 +790,12 @@ void X86FrameLowering::emitStackProbeInlineGenericLoop(
MachineBasicBlock *testMBB = MF.CreateMachineBasicBlock(LLVM_BB);
MachineBasicBlock *tailMBB = MF.CreateMachineBasicBlock(LLVM_BB);
+ // Set call-frame sizes in each new BB, in case we are in a call sequence.
+ std::optional<unsigned> CallFrameSizeBefore =
+ TII.getCallFrameSizeAt(MBB, MBBI);
+ testMBB->setCallFrameSize(CallFrameSizeBefore);
+ tailMBB->setCallFrameSize(CallFrameSizeBefore);
+
MachineFunction::iterator MBBIter = ++MBB.getIterator();
MF.insert(MBBIter, testMBB);
MF.insert(MBBIter, tailMBB);
@@ -932,6 +938,13 @@ void X86FrameLowering::emitStackProbeInlineWindowsCoreCLR64(
MachineBasicBlock *LoopMBB = MF.CreateMachineBasicBlock(LLVM_BB);
MachineBasicBlock *ContinueMBB = MF.CreateMachineBasicBlock(LLVM_BB);
+ // Set call-frame sizes in each new BB, in case we are in a call sequence.
+ std::optional<unsigned> CallFrameSizeBefore =
+ TII.getCallFrameSizeAt(MBB, MBBI);
+ RoundMBB->setCallFrameSize(CallFrameSizeBefore);
+ LoopMBB->setCallFrameSize(CallFrameSizeBefore);
+ ContinueMBB->setCallFrameSize(CallFrameSizeBefore);
+
MachineFunction::iterator MBBIter = std::next(MBB.getIterator());
MF.insert(MBBIter, RoundMBB);
MF.insert(MBBIter, LoopMBB);
@@ -1286,6 +1299,19 @@ void X86FrameLowering::BuildStackAlignAND(MachineBasicBlock &MBB,
: Is64Bit ? X86::R11D
: X86::EAX;
+ std::optional<unsigned> CallFrameSizeBefore =
+ TII.getCallFrameSizeAt(MBB, MBBI);
+ // entryMBB takes the place of MBB, so give it its stored call frame size.
+ entryMBB->setCallFrameSize(MBB.getCallFrameSize());
+ // MBB contains only a tail of its original instructions, so update its
+ // stored call frame size.
+ MBB.setCallFrameSize(CallFrameSizeBefore);
+ // For the other basic blocks, take the state at the insertion point as
+ // well.
+ headMBB->setCallFrameSize(CallFrameSizeBefore);
+ bodyMBB->setCallFrameSize(CallFrameSizeBefore);
+ footMBB->setCallFrameSize(CallFrameSizeBefore);
+
// Setup entry block
{
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index bbee0af109c74b..e5ba54c176f07b 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -35337,6 +35337,16 @@ X86TargetLowering::EmitLoweredProbedAlloca(MachineInstr &MI,
MachineBasicBlock *tailMBB = MF->CreateMachineBasicBlock(LLVM_BB);
MachineBasicBlock *blockMBB = MF->CreateMachineBasicBlock(LLVM_BB);
+ // Set call-frame sizes in each new BB, in case we are in a call sequence.
+ // MBB is split at std::next(MachineBasicBlock::iterator(MI) and these blocks
+ // are inserted in between, so they should all have the call frame size at
+ // the split point.
+ std::optional<unsigned> CallFrameSizeAtSplit =
+ TII->getCallFrameSizeAt(*MBB, std::next(MachineBasicBlock::iterator(MI)));
+ testMBB->setCallFrameSize(CallFrameSizeAtSplit);
+ blockMBB->setCallFrameSize(CallFrameSizeAtSplit);
+ tailMBB->setCallFrameSize(CallFrameSizeAtSplit);
+
MachineFunction::iterator MBBIter = ++MBB->getIterator();
MF->insert(MBBIter, testMBB);
MF->insert(MBBIter, blockMBB);
More information about the llvm-commits
mailing list