[llvm] [CodeGen] Allow multiple location for the same CSR. (PR #168531)
Mikhail Gudim via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 18 07:25:59 PST 2025
https://github.com/mgudim updated https://github.com/llvm/llvm-project/pull/168531
>From b186974e78d2829fac89838941fb5d3f0685521f Mon Sep 17 00:00:00 2001
From: Mikhail Gudim <mgudim at ventanamicro.com>
Date: Tue, 18 Nov 2025 04:58:07 -0800
Subject: [PATCH] [CodeGen] Allow multiple location for the same CSR.
Currently there is an implicit assumption in `CFIInstrInserter` that a CSR can
be saved only in one location. However, scenarios when this assumption breaks are possible.
---
llvm/lib/CodeGen/CFIInstrInserter.cpp | 257 +++++++++++-------
.../CodeGen/RISCV/cfi-multiple-locations.mir | 2 -
2 files changed, 155 insertions(+), 104 deletions(-)
diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp
index 14098bc821617..20fe26afe7503 100644
--- a/llvm/lib/CodeGen/CFIInstrInserter.cpp
+++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp
@@ -65,7 +65,49 @@ class CFIInstrInserter : public MachineFunctionPass {
return insertedCFI;
}
- private:
+private:
+#define INVALID_REG UINT_MAX
+#define INVALID_OFFSET INT_MAX
+ /// contains the location where CSR register is saved.
+ struct CSRSavedLocation {
+ enum Kind { INVALID, REGISTER, CFA_OFFSET };
+ CSRSavedLocation() {
+ K = Kind::INVALID;
+ Reg = 0;
+ Offset = 0;
+ }
+ Kind K;
+ // Dwarf register number
+ unsigned Reg;
+ // CFA offset
+ int64_t Offset;
+ bool isValid() const { return K != Kind::INVALID; }
+ bool operator==(const CSRSavedLocation &RHS) const {
+ switch (K) {
+ case Kind::INVALID:
+ return !RHS.isValid();
+ case Kind::REGISTER:
+ return Reg == RHS.Reg;
+ case Kind::CFA_OFFSET:
+ return Offset == RHS.Offset;
+ }
+ llvm_unreachable("Unknown CSRSavedLocation Kind!");
+ }
+ void dump(raw_ostream &OS) const {
+ switch (K) {
+ case Kind::INVALID:
+ OS << "INVALID";
+ break;
+ case Kind::REGISTER:
+ OS << "In Dwarf register: " << Reg;
+ break;
+ case Kind::CFA_OFFSET:
+ OS << "At CFA offset: " << Offset;
+ break;
+ }
+ }
+ };
+
struct MBBCFAInfo {
MachineBasicBlock *MBB;
/// Value of cfa offset valid at basic block entry.
@@ -76,38 +118,27 @@ class CFIInstrInserter : public MachineFunctionPass {
unsigned IncomingCFARegister = 0;
/// Value of cfa register valid at basic block exit.
unsigned OutgoingCFARegister = 0;
- /// Set of callee saved registers saved at basic block entry.
- BitVector IncomingCSRSaved;
- /// Set of callee saved registers saved at basic block exit.
- BitVector OutgoingCSRSaved;
+ /// Set of locations where the callee saved registers are at basic block
+ /// entry.
+ SmallVector<CSRSavedLocation> IncomingCSRLocations;
+ /// Set of locations where the callee saved registers are at basic block
+ /// exit.
+ SmallVector<CSRSavedLocation> OutgoingCSRLocations;
/// If in/out cfa offset and register values for this block have already
/// been set or not.
bool Processed = false;
};
-#define INVALID_REG UINT_MAX
-#define INVALID_OFFSET INT_MAX
- /// contains the location where CSR register is saved.
- struct CSRSavedLocation {
- CSRSavedLocation(std::optional<unsigned> R, std::optional<int> O)
- : Reg(R), Offset(O) {}
- std::optional<unsigned> Reg;
- std::optional<int> Offset;
- };
-
/// Contains cfa offset and register values valid at entry and exit of basic
/// blocks.
std::vector<MBBCFAInfo> MBBVector;
- /// Map the callee save registers to the locations where they are saved.
- SmallDenseMap<unsigned, CSRSavedLocation, 16> CSRLocMap;
-
/// Calculate cfa offset and register values valid at entry and exit for all
/// basic blocks in a function.
void calculateCFAInfo(MachineFunction &MF);
/// Calculate cfa offset and register values valid at basic block exit by
- /// checking the block for CFI instructions. Block's incoming CFA info remains
- /// the same.
+ /// checking the block for CFI instructions. Block's incoming CFA info
+ /// remains the same.
void calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo);
/// Update in/out cfa offset and register values for successors of the basic
/// block.
@@ -119,8 +150,8 @@ class CFIInstrInserter : public MachineFunctionPass {
/// that block.
bool insertCFIInstrs(MachineFunction &MF);
/// Return the cfa offset value that should be set at the beginning of a MBB
- /// if needed. The negated value is needed when creating CFI instructions that
- /// set absolute offset.
+ /// if needed. The negated value is needed when creating CFI instructions
+ /// that set absolute offset.
int64_t getCorrectCFAOffset(MachineBasicBlock *MBB) {
return MBBVector[MBB->getNumber()].IncomingCFAOffset;
}
@@ -162,10 +193,18 @@ void CFIInstrInserter::calculateCFAInfo(MachineFunction &MF) {
MBBInfo.OutgoingCFAOffset = InitialOffset;
MBBInfo.IncomingCFARegister = DwarfInitialRegister;
MBBInfo.OutgoingCFARegister = DwarfInitialRegister;
- MBBInfo.IncomingCSRSaved.resize(NumRegs);
- MBBInfo.OutgoingCSRSaved.resize(NumRegs);
+ MBBInfo.IncomingCSRLocations.resize(NumRegs);
+ MBBInfo.OutgoingCSRLocations.resize(NumRegs);
+ }
+
+ // Record the initial location of all registers.
+ MBBCFAInfo &EntryMBBInfo = MBBVector[MF.front().getNumber()];
+ const MCPhysReg *CSRegs = MF.getRegInfo().getCalleeSavedRegs();
+ for (int i = 0; CSRegs[i]; ++i) {
+ unsigned Reg = TRI.getDwarfRegNum(CSRegs[i], true);
+ CSRSavedLocation &CSRLoc = EntryMBBInfo.IncomingCSRLocations[Reg];
+ CSRLoc.Reg = Reg;
}
- CSRLocMap.clear();
// Set in/out cfa info for all blocks in the function. This traversal is based
// on the assumption that the first block in the function is the entry block
@@ -176,14 +215,18 @@ void CFIInstrInserter::calculateCFAInfo(MachineFunction &MF) {
void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
// Outgoing cfa offset set by the block.
- int64_t SetOffset = MBBInfo.IncomingCFAOffset;
+ int64_t &OutgoingCFAOffset = MBBInfo.OutgoingCFAOffset;
+ OutgoingCFAOffset = MBBInfo.IncomingCFAOffset;
// Outgoing cfa register set by the block.
- unsigned SetRegister = MBBInfo.IncomingCFARegister;
+ unsigned &OutgoingCFARegister = MBBInfo.OutgoingCFARegister;
+ OutgoingCFARegister = MBBInfo.IncomingCFARegister;
+ // Outgoing locations for each callee-saved register set by the block.
+ SmallVector<CSRSavedLocation> &OutgoingCSRLocations =
+ MBBInfo.OutgoingCSRLocations;
+ OutgoingCSRLocations = MBBInfo.IncomingCSRLocations;
+
MachineFunction *MF = MBBInfo.MBB->getParent();
const std::vector<MCCFIInstruction> &Instrs = MF->getFrameInstructions();
- const TargetRegisterInfo &TRI = *MF->getSubtarget().getRegisterInfo();
- unsigned NumRegs = TRI.getNumSupportedRegs(*MF);
- BitVector CSRSaved(NumRegs), CSRRestored(NumRegs);
#ifndef NDEBUG
int RememberState = 0;
@@ -192,36 +235,51 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
// Determine cfa offset and register set by the block.
for (MachineInstr &MI : *MBBInfo.MBB) {
if (MI.isCFIInstruction()) {
- std::optional<unsigned> CSRReg;
- std::optional<int64_t> CSROffset;
unsigned CFIIndex = MI.getOperand(0).getCFIIndex();
const MCCFIInstruction &CFI = Instrs[CFIIndex];
switch (CFI.getOperation()) {
- case MCCFIInstruction::OpDefCfaRegister:
- SetRegister = CFI.getRegister();
+ case MCCFIInstruction::OpDefCfaRegister: {
+ OutgoingCFARegister = CFI.getRegister();
break;
- case MCCFIInstruction::OpDefCfaOffset:
- SetOffset = CFI.getOffset();
+ }
+ case MCCFIInstruction::OpDefCfaOffset: {
+ OutgoingCFAOffset = CFI.getOffset();
break;
- case MCCFIInstruction::OpAdjustCfaOffset:
- SetOffset += CFI.getOffset();
+ }
+ case MCCFIInstruction::OpAdjustCfaOffset: {
+ OutgoingCFAOffset += CFI.getOffset();
break;
- case MCCFIInstruction::OpDefCfa:
- SetRegister = CFI.getRegister();
- SetOffset = CFI.getOffset();
+ }
+ case MCCFIInstruction::OpDefCfa: {
+ OutgoingCFARegister = CFI.getRegister();
+ OutgoingCFAOffset = CFI.getOffset();
break;
- case MCCFIInstruction::OpOffset:
- CSROffset = CFI.getOffset();
+ }
+ case MCCFIInstruction::OpOffset: {
+ CSRSavedLocation &CSRLocation = OutgoingCSRLocations[CFI.getRegister()];
+ CSRLocation.K = CSRSavedLocation::Kind::CFA_OFFSET;
+ CSRLocation.Offset = CFI.getOffset();
break;
- case MCCFIInstruction::OpRegister:
- CSRReg = CFI.getRegister2();
+ }
+ case MCCFIInstruction::OpRegister: {
+ CSRSavedLocation &CSRLocation = OutgoingCSRLocations[CFI.getRegister()];
+ CSRLocation.K = CSRSavedLocation::Kind::REGISTER;
+ CSRLocation.Reg = CFI.getRegister2();
break;
- case MCCFIInstruction::OpRelOffset:
- CSROffset = CFI.getOffset() - SetOffset;
+ }
+ case MCCFIInstruction::OpRelOffset: {
+ CSRSavedLocation &CSRLocation = OutgoingCSRLocations[CFI.getRegister()];
+ CSRLocation.K = CSRSavedLocation::Kind::CFA_OFFSET;
+ CSRLocation.Offset = CFI.getOffset() - OutgoingCFAOffset;
break;
- case MCCFIInstruction::OpRestore:
- CSRRestored.set(CFI.getRegister());
+ }
+ case MCCFIInstruction::OpRestore: {
+ unsigned Reg = CFI.getRegister();
+ CSRSavedLocation &CSRLocation = OutgoingCSRLocations[Reg];
+ CSRLocation.K = CSRSavedLocation::Kind::REGISTER;
+ CSRLocation.Reg = Reg;
break;
+ }
case MCCFIInstruction::OpLLVMDefAspaceCfa:
// TODO: Add support for handling cfi_def_aspace_cfa.
#ifndef NDEBUG
@@ -266,16 +324,6 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
case MCCFIInstruction::OpValOffset:
break;
}
- if (CSRReg || CSROffset) {
- auto It = CSRLocMap.find(CFI.getRegister());
- if (It == CSRLocMap.end()) {
- CSRLocMap.insert(
- {CFI.getRegister(), CSRSavedLocation(CSRReg, CSROffset)});
- } else if (It->second.Reg != CSRReg || It->second.Offset != CSROffset) {
- llvm_unreachable("Different saved locations for the same CSR");
- }
- CSRSaved.set(CFI.getRegister());
- }
}
}
@@ -288,15 +336,6 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
#endif
MBBInfo.Processed = true;
-
- // Update outgoing CFA info.
- MBBInfo.OutgoingCFAOffset = SetOffset;
- MBBInfo.OutgoingCFARegister = SetRegister;
-
- // Update outgoing CSR info.
- BitVector::apply([](auto x, auto y, auto z) { return (x | y) & ~z; },
- MBBInfo.OutgoingCSRSaved, MBBInfo.IncomingCSRSaved, CSRSaved,
- CSRRestored);
}
void CFIInstrInserter::updateSuccCFAInfo(MBBCFAInfo &MBBInfo) {
@@ -312,7 +351,7 @@ void CFIInstrInserter::updateSuccCFAInfo(MBBCFAInfo &MBBInfo) {
if (!SuccInfo.Processed) {
SuccInfo.IncomingCFAOffset = CurrentInfo.OutgoingCFAOffset;
SuccInfo.IncomingCFARegister = CurrentInfo.OutgoingCFARegister;
- SuccInfo.IncomingCSRSaved = CurrentInfo.OutgoingCSRSaved;
+ SuccInfo.IncomingCSRLocations = CurrentInfo.OutgoingCSRLocations;
Stack.push_back(Succ);
}
}
@@ -324,7 +363,6 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) {
const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
bool InsertedCFIInstr = false;
- BitVector SetDifference;
for (MachineBasicBlock &MBB : MF) {
// Skip the first MBB in a function
if (MBB.getNumber() == MF.front().getNumber()) continue;
@@ -376,32 +414,34 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) {
continue;
}
- BitVector::apply([](auto x, auto y) { return x & ~y; }, SetDifference,
- PrevMBBInfo->OutgoingCSRSaved, MBBInfo.IncomingCSRSaved);
- for (int Reg : SetDifference.set_bits()) {
- unsigned CFIIndex =
- MF.addFrameInst(MCCFIInstruction::createRestore(nullptr, Reg));
- BuildMI(*MBBInfo.MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
- .addCFIIndex(CFIIndex);
- InsertedCFIInstr = true;
- }
-
- BitVector::apply([](auto x, auto y) { return x & ~y; }, SetDifference,
- MBBInfo.IncomingCSRSaved, PrevMBBInfo->OutgoingCSRSaved);
- for (int Reg : SetDifference.set_bits()) {
- auto it = CSRLocMap.find(Reg);
- assert(it != CSRLocMap.end() && "Reg should have an entry in CSRLocMap");
- unsigned CFIIndex;
- CSRSavedLocation RO = it->second;
- if (!RO.Reg && RO.Offset) {
+ for (unsigned i = 0; i < PrevMBBInfo->OutgoingCSRLocations.size(); ++i) {
+ const CSRSavedLocation &PrevOutgoingCSRLoc =
+ PrevMBBInfo->OutgoingCSRLocations[i];
+ const CSRSavedLocation &HasToBeCSRLoc = MBBInfo.IncomingCSRLocations[i];
+ // Ignore non-callee-saved registers, they remain uninitialized (invalid).
+ if (!HasToBeCSRLoc.isValid())
+ continue;
+ if (HasToBeCSRLoc == PrevOutgoingCSRLoc)
+ continue;
+
+ unsigned CFIIndex = (unsigned)(-1);
+ if (HasToBeCSRLoc.K == CSRSavedLocation::Kind::CFA_OFFSET &&
+ HasToBeCSRLoc.Offset != PrevOutgoingCSRLoc.Offset) {
CFIIndex = MF.addFrameInst(
- MCCFIInstruction::createOffset(nullptr, Reg, *RO.Offset));
- } else if (RO.Reg && !RO.Offset) {
- CFIIndex = MF.addFrameInst(
- MCCFIInstruction::createRegister(nullptr, Reg, *RO.Reg));
- } else {
- llvm_unreachable("RO.Reg and RO.Offset cannot both be valid/invalid");
- }
+ MCCFIInstruction::createOffset(nullptr, i, HasToBeCSRLoc.Offset));
+ } else if (HasToBeCSRLoc.K == CSRSavedLocation::Kind::REGISTER &&
+ (HasToBeCSRLoc.Reg != PrevOutgoingCSRLoc.Reg)) {
+ unsigned NewReg = HasToBeCSRLoc.Reg;
+ unsigned DwarfEHReg = i;
+ if (NewReg == DwarfEHReg) {
+ CFIIndex = MF.addFrameInst(
+ MCCFIInstruction::createRestore(nullptr, DwarfEHReg));
+ } else {
+ CFIIndex = MF.addFrameInst(
+ MCCFIInstruction::createRegister(nullptr, i, HasToBeCSRLoc.Reg));
+ }
+ } else
+ llvm_unreachable("Unexpected CSR location.");
BuildMI(*MBBInfo.MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(CFIIndex);
InsertedCFIInstr = true;
@@ -434,13 +474,23 @@ void CFIInstrInserter::reportCSRError(const MBBCFAInfo &Pred,
<< Pred.MBB->getParent()->getName() << " ***\n";
errs() << "Pred: " << Pred.MBB->getName() << " #" << Pred.MBB->getNumber()
<< " outgoing CSR Saved: ";
- for (int Reg : Pred.OutgoingCSRSaved.set_bits())
- errs() << Reg << " ";
+ for (const CSRSavedLocation &OutgoingCSRLocation :
+ Pred.OutgoingCSRLocations) {
+ if (OutgoingCSRLocation.isValid()) {
+ OutgoingCSRLocation.dump(errs());
+ errs() << " ";
+ }
+ }
errs() << "\n";
errs() << "Succ: " << Succ.MBB->getName() << " #" << Succ.MBB->getNumber()
<< " incoming CSR Saved: ";
- for (int Reg : Succ.IncomingCSRSaved.set_bits())
- errs() << Reg << " ";
+ for (const CSRSavedLocation &IncomingCSRLocation :
+ Succ.IncomingCSRLocations) {
+ if (IncomingCSRLocation.isValid()) {
+ IncomingCSRLocation.dump(errs());
+ errs() << " ";
+ }
+ }
errs() << "\n";
}
@@ -463,9 +513,12 @@ unsigned CFIInstrInserter::verify(MachineFunction &MF) {
}
// Check that IncomingCSRSaved of every successor matches the
// OutgoingCSRSaved of CurrMBB
- if (SuccMBBInfo.IncomingCSRSaved != CurrMBBInfo.OutgoingCSRSaved) {
- reportCSRError(CurrMBBInfo, SuccMBBInfo);
- ErrorNum++;
+ for (unsigned i = 0; i < CurrMBBInfo.OutgoingCSRLocations.size(); ++i) {
+ if (!(CurrMBBInfo.OutgoingCSRLocations[i] ==
+ SuccMBBInfo.IncomingCSRLocations[i])) {
+ reportCSRError(CurrMBBInfo, SuccMBBInfo);
+ ErrorNum++;
+ }
}
}
}
diff --git a/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir b/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir
index 7844589e3f93c..a8547efde90cd 100644
--- a/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir
+++ b/llvm/test/CodeGen/RISCV/cfi-multiple-locations.mir
@@ -1,10 +1,8 @@
# RUN: llc %s -mtriple=riscv64 \
# RUN: -run-pass=cfi-instr-inserter \
# RUN: -riscv-enable-cfi-instr-inserter=true
-# XFAIL: *
# Technically, it is possible that a callee-saved register is saved in multiple different locations.
-# CFIInstrInserter should handle this, but currently it does not.
---
name: multiple_locations
tracksRegLiveness: true
More information about the llvm-commits
mailing list