From 4f627a6d7c42ffc0bb7df4cf0a1cd917b0282352 Mon Sep 17 00:00:00 2001 From: Mikhail Gudim Date: Thu, 20 Nov 2025 04:39:52 -0800 Subject: [PATCH 1/3] [CFIInserter] Improve `CSRSavedLocation` struct. (1) Define `CSRSavedLocation::Kind` and use it in the code. This makes the code more readable and allows to extend it to new kinds. For example, soon I want to add "scalable offset from a given register" kind. (2) Store the contents in a union. This should reduce memory usage. --- llvm/lib/CodeGen/CFIInstrInserter.cpp | 92 ++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 15 deletions(-) diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp index 1a0e222da98cd..d9e9d0e2a8ef3 100644 --- a/llvm/lib/CodeGen/CFIInstrInserter.cpp +++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp @@ -88,11 +88,60 @@ class CFIInstrInserter : public MachineFunctionPass { #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; + class CSRSavedLocation { + public: + enum Kind { INVALID, REGISTER, CFA_OFFSET }; + Kind K; + + private: + union { + // Dwarf register number + unsigned Reg; + // CFA offset + int64_t Offset; + } U; + + public: + CSRSavedLocation() { K = Kind::INVALID; } + + bool isValid() const { return K != Kind::INVALID; } + + unsigned getRegister() const { + assert(K == REGISTER); + return U.Reg; + } + + void setRegister(unsigned _Reg) { + assert(K == REGISTER); + U.Reg = _Reg; + } + + int64_t getOffset() const { + assert(K == REGISTER); + return U.Offset; + } + + void setOffset(int64_t _Offset) { + assert(K == REGISTER); + U.Offset = _Offset; + } + + bool operator==(const CSRSavedLocation &RHS) const { + if (K != RHS.K) + return false; + switch (K) { + case Kind::INVALID: + return !RHS.isValid(); + case Kind::REGISTER: + return getRegister() == RHS.getRegister(); + case Kind::CFA_OFFSET: + return getOffset() == RHS.getOffset(); + } + llvm_unreachable("Unknown CSRSavedLocation Kind!"); + } + bool operator!=(const CSRSavedLocation &RHS) const { + return !(*this == RHS); + } }; /// Contains cfa offset and register values valid at entry and exit of basic @@ -266,12 +315,20 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) { case MCCFIInstruction::OpValOffset: break; } - if (CSRReg || CSROffset) { + CSRSavedLocation CSRLoc; + if (CSRReg) { + CSRLoc.K = CSRSavedLocation::Kind::REGISTER; + CSRLoc.setRegister(*CSRReg); + } + if (CSROffset) { + CSRLoc.K = CSRSavedLocation::Kind::CFA_OFFSET; + CSRLoc.setOffset(*CSROffset); + } + if (CSRLoc.isValid()) { 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) { + CSRLocMap.insert({CFI.getRegister(), CSRLoc}); + } else if (It->second != CSRLoc) { reportFatalInternalError( "Different saved locations for the same CSR"); } @@ -394,14 +451,19 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) { assert(it != CSRLocMap.end() && "Reg should have an entry in CSRLocMap"); unsigned CFIIndex; CSRSavedLocation RO = it->second; - if (!RO.Reg && RO.Offset) { + switch (RO.K) { + case CSRSavedLocation::CFA_OFFSET: { CFIIndex = MF.addFrameInst( - MCCFIInstruction::createOffset(nullptr, Reg, *RO.Offset)); - } else if (RO.Reg && !RO.Offset) { + MCCFIInstruction::createOffset(nullptr, Reg, RO.getOffset())); + break; + } + case CSRSavedLocation::REGISTER: { CFIIndex = MF.addFrameInst( - MCCFIInstruction::createRegister(nullptr, Reg, *RO.Reg)); - } else { - llvm_unreachable("RO.Reg and RO.Offset cannot both be valid/invalid"); + MCCFIInstruction::createRegister(nullptr, Reg, RO.getRegister())); + break; + } + default: + llvm_unreachable("INVALID CSRSavedLocation!"); } BuildMI(*MBBInfo.MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION)) .addCFIIndex(CFIIndex); From fc3c68fabda67bf2ed41c8aee33398a741333c4f Mon Sep 17 00:00:00 2001 From: Mikhail Gudim Date: Thu, 20 Nov 2025 09:01:39 -0800 Subject: [PATCH 2/3] addressed review comments. --- llvm/lib/CodeGen/CFIInstrInserter.cpp | 60 +++++++++++++-------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp index d9e9d0e2a8ef3..f03e46775e6a3 100644 --- a/llvm/lib/CodeGen/CFIInstrInserter.cpp +++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp @@ -90,8 +90,8 @@ class CFIInstrInserter : public MachineFunctionPass { /// contains the location where CSR register is saved. class CSRSavedLocation { public: - enum Kind { INVALID, REGISTER, CFA_OFFSET }; - Kind K; + enum Kind { Invalid, Register, CFAOffset }; + Kind K = Invalid; private: union { @@ -102,39 +102,43 @@ class CFIInstrInserter : public MachineFunctionPass { } U; public: - CSRSavedLocation() { K = Kind::INVALID; } + CSRSavedLocation() { K = Kind::Invalid; } - bool isValid() const { return K != Kind::INVALID; } + static CSRSavedLocation createCFAOffset(int64_t Offset) { + CSRSavedLocation Loc; + Loc.K = Kind::CFAOffset; + Loc.U.Offset = Offset; + return Loc; + } - unsigned getRegister() const { - assert(K == REGISTER); - return U.Reg; + static CSRSavedLocation createRegister(unsigned Reg) { + CSRSavedLocation Loc; + Loc.K = Kind::Register; + Loc.U.Reg = Reg; + return Loc; } - void setRegister(unsigned _Reg) { - assert(K == REGISTER); - U.Reg = _Reg; + bool isValid() const { return K != Kind::Invalid; } + + unsigned getRegister() const { + assert(K == Register); + return U.Reg; } int64_t getOffset() const { - assert(K == REGISTER); + assert(K == Register); return U.Offset; } - void setOffset(int64_t _Offset) { - assert(K == REGISTER); - U.Offset = _Offset; - } - bool operator==(const CSRSavedLocation &RHS) const { if (K != RHS.K) return false; switch (K) { - case Kind::INVALID: + case Kind::Invalid: return !RHS.isValid(); - case Kind::REGISTER: + case Kind::Register: return getRegister() == RHS.getRegister(); - case Kind::CFA_OFFSET: + case Kind::CFAOffset: return getOffset() == RHS.getOffset(); } llvm_unreachable("Unknown CSRSavedLocation Kind!"); @@ -316,14 +320,10 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) { break; } CSRSavedLocation CSRLoc; - if (CSRReg) { - CSRLoc.K = CSRSavedLocation::Kind::REGISTER; - CSRLoc.setRegister(*CSRReg); - } - if (CSROffset) { - CSRLoc.K = CSRSavedLocation::Kind::CFA_OFFSET; - CSRLoc.setOffset(*CSROffset); - } + if (CSRReg) + CSRLoc = CSRSavedLocation::createRegister(*CSRReg); + if (CSROffset) + CSRLoc = CSRSavedLocation::createCFAOffset(*CSROffset); if (CSRLoc.isValid()) { auto It = CSRLocMap.find(CFI.getRegister()); if (It == CSRLocMap.end()) { @@ -452,18 +452,18 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) { unsigned CFIIndex; CSRSavedLocation RO = it->second; switch (RO.K) { - case CSRSavedLocation::CFA_OFFSET: { + case CSRSavedLocation::CFAOffset: { CFIIndex = MF.addFrameInst( MCCFIInstruction::createOffset(nullptr, Reg, RO.getOffset())); break; } - case CSRSavedLocation::REGISTER: { + case CSRSavedLocation::Register: { CFIIndex = MF.addFrameInst( MCCFIInstruction::createRegister(nullptr, Reg, RO.getRegister())); break; } default: - llvm_unreachable("INVALID CSRSavedLocation!"); + llvm_unreachable("Invalid CSRSavedLocation!"); } BuildMI(*MBBInfo.MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION)) .addCFIIndex(CFIIndex); From 1aaf673fd7a126dfcbb4777c212339722836d87f Mon Sep 17 00:00:00 2001 From: Mikhail Gudim Date: Fri, 21 Nov 2025 01:52:56 -0800 Subject: [PATCH 3/3] fixed a wrong assertion. --- llvm/lib/CodeGen/CFIInstrInserter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp index f03e46775e6a3..930d590a1e5b9 100644 --- a/llvm/lib/CodeGen/CFIInstrInserter.cpp +++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp @@ -121,12 +121,12 @@ class CFIInstrInserter : public MachineFunctionPass { bool isValid() const { return K != Kind::Invalid; } unsigned getRegister() const { - assert(K == Register); + assert(K == Kind::Register); return U.Reg; } int64_t getOffset() const { - assert(K == Register); + assert(K == Kind::CFAOffset); return U.Offset; }