-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[CodeGen] Allow multiple location for the same CSR. #168531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-backend-risc-v Author: Mikhail Gudim (mgudim) ChangesCurrently there is an implicit assumption in Full diff: https://github.com/llvm/llvm-project/pull/168531.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp
index 14098bc821617..667928304df50 100644
--- a/llvm/lib/CodeGen/CFIInstrInserter.cpp
+++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp
@@ -66,72 +66,103 @@ class CFIInstrInserter : public MachineFunctionPass {
}
private:
- struct MBBCFAInfo {
- MachineBasicBlock *MBB;
- /// Value of cfa offset valid at basic block entry.
- int64_t IncomingCFAOffset = -1;
- /// Value of cfa offset valid at basic block exit.
- int64_t OutgoingCFAOffset = -1;
- /// Value of cfa register valid at basic block entry.
- 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;
- /// 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.
- void calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo);
- /// Update in/out cfa offset and register values for successors of the basic
- /// block.
- void updateSuccCFAInfo(MBBCFAInfo &MBBInfo);
-
- /// Check if incoming CFA information of a basic block matches outgoing CFA
- /// information of the previous block. If it doesn't, insert CFI instruction
- /// at the beginning of the block that corrects the CFA calculation rule for
- /// 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.
- int64_t getCorrectCFAOffset(MachineBasicBlock *MBB) {
- return MBBVector[MBB->getNumber()].IncomingCFAOffset;
- }
-
- void reportCFAError(const MBBCFAInfo &Pred, const MBBCFAInfo &Succ);
- void reportCSRError(const MBBCFAInfo &Pred, const MBBCFAInfo &Succ);
- /// Go through each MBB in a function and check that outgoing offset and
- /// register of its predecessors match incoming offset and register of that
- /// MBB, as well as that incoming offset and register of its successors match
- /// outgoing offset and register of the MBB.
- unsigned verify(MachineFunction &MF);
+ /// 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.
+ int64_t IncomingCFAOffset = -1;
+ /// Value of cfa offset valid at basic block exit.
+ int64_t OutgoingCFAOffset = -1;
+ /// Value of cfa register valid at basic block entry.
+ unsigned IncomingCFARegister = 0;
+ /// Value of cfa register valid at basic block exit.
+ unsigned OutgoingCFARegister = 0;
+ /// 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;
+ };
+
+ /// Contains cfa offset and register values valid at entry and exit of basic
+ /// blocks.
+ std::vector<MBBCFAInfo> MBBVector;
+
+ /// 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.
+ void calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo);
+ /// Update in/out cfa offset and register values for successors of the basic
+ /// block.
+ void updateSuccCFAInfo(MBBCFAInfo &MBBInfo);
+
+ /// Check if incoming CFA information of a basic block matches outgoing CFA
+ /// information of the previous block. If it doesn't, insert CFI instruction
+ /// at the beginning of the block that corrects the CFA calculation rule for
+ /// 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.
+ int64_t getCorrectCFAOffset(MachineBasicBlock *MBB) {
+ return MBBVector[MBB->getNumber()].IncomingCFAOffset;
+ }
+
+ void reportCFAError(const MBBCFAInfo &Pred, const MBBCFAInfo &Succ);
+ void reportCSRError(const MBBCFAInfo &Pred, const MBBCFAInfo &Succ);
+ /// Go through each MBB in a function and check that outgoing offset and
+ /// register of its predecessors match incoming offset and register of that
+ /// MBB, as well as that incoming offset and register of its successors match
+ /// outgoing offset and register of the MBB.
+ unsigned verify(MachineFunction &MF);
};
} // namespace
@@ -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) {
- CFIIndex = MF.addFrameInst(
- MCCFIInstruction::createOffset(nullptr, Reg, *RO.Offset));
- } else 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::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
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) LLVMLLVM.CodeGen/X86/cfi-inserter-callee-save-register-2.mirLLVM.CodeGen/X86/cfi-inserter-callee-save-register.mirLLVM.CodeGen/X86/cfi-inserter-verify-inconsistent-loc.mirLLVM.CodeGen/X86/segmented-stacks-dynamic.llLLVM.CodeGen/X86/segmented-stacks.llLLVM.DebugInfo/MIR/X86/no-cfi-loc.mirIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
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.
9d45640 to
b186974
Compare
Currently there is an implicit assumption in
CFIInstrInserterthat a CSR can be saved only in one location. However, scenarios when this assumption breaks are possible.