From b186974e78d2829fac89838941fb5d3f0685521f Mon Sep 17 00:00:00 2001 From: Mikhail Gudim 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 IncomingCSRLocations; + /// Set of locations where the callee saved registers are at basic block + /// exit. + SmallVector 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 R, std::optional O) - : Reg(R), Offset(O) {} - std::optional Reg; - std::optional Offset; - }; - /// Contains cfa offset and register values valid at entry and exit of basic /// blocks. std::vector MBBVector; - /// Map the callee save registers to the locations where they are saved. - SmallDenseMap 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 &OutgoingCSRLocations = + MBBInfo.OutgoingCSRLocations; + OutgoingCSRLocations = MBBInfo.IncomingCSRLocations; + MachineFunction *MF = MBBInfo.MBB->getParent(); const std::vector &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 CSRReg; - std::optional 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