[llvm] [CFIInserter] Improve `CSRSavedLocation` struct. (PR #168869)
Mikhail Gudim via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 10 11:24:46 PST 2025
https://github.com/mgudim updated https://github.com/llvm/llvm-project/pull/168869
>From 720eb42f9cf5df64a144ef663a352cf483fc27a4 Mon Sep 17 00:00:00 2001
From: Mikhail Gudim <mgudim at ventanamicro.com>
Date: Thu, 20 Nov 2025 04:39:52 -0800
Subject: [PATCH 1/4] [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 | 91 +++++++++++++++++++++------
1 file changed, 72 insertions(+), 19 deletions(-)
diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp
index 38858270c9197..d9e9d0e2a8ef3 100644
--- a/llvm/lib/CodeGen/CFIInstrInserter.cpp
+++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp
@@ -88,19 +88,57 @@ 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<unsigned> R, std::optional<int> O)
- : Reg(R), Offset(O) {
- assert((Reg.has_value() ^ Offset.has_value()) &&
- "Register and offset can not both be valid");
+ 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;
}
- std::optional<unsigned> Reg;
- std::optional<int> Offset;
- bool operator==(const CSRSavedLocation &RHS) const {
- return Reg == RHS.Reg && Offset == RHS.Offset;
+ 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);
}
@@ -277,10 +315,20 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
case MCCFIInstruction::OpValOffset:
break;
}
- if (CSRReg || CSROffset) {
- CSRSavedLocation Loc(CSRReg, CSROffset);
- auto [It, Inserted] = CSRLocMap.insert({CFI.getRegister(), Loc});
- if (!Inserted && It->second != Loc) {
+ 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(), CSRLoc});
+ } else if (It->second != CSRLoc) {
reportFatalInternalError(
"Different saved locations for the same CSR");
}
@@ -403,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 {
- assert((RO.Reg && !RO.Offset) &&
- "Reg and Offset cannot both be valid/invalid");
+ MCCFIInstruction::createOffset(nullptr, Reg, RO.getOffset()));
+ break;
+ }
+ case CSRSavedLocation::REGISTER: {
CFIIndex = MF.addFrameInst(
- MCCFIInstruction::createRegister(nullptr, Reg, *RO.Reg));
+ 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 9d6fbb4364501cbc97396f0ed09211c10ad10321 Mon Sep 17 00:00:00 2001
From: Mikhail Gudim <mgudim at ventanamicro.com>
Date: Thu, 20 Nov 2025 09:01:39 -0800
Subject: [PATCH 2/4] 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 0db6b47b15524415f531c0b88fd00aadbef32aa1 Mon Sep 17 00:00:00 2001
From: Mikhail Gudim <mgudim at ventanamicro.com>
Date: Fri, 21 Nov 2025 01:52:56 -0800
Subject: [PATCH 3/4] 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;
}
>From 76b10c1e9c57fa700944b16b758fdc6f11f2fca5 Mon Sep 17 00:00:00 2001
From: Mikhail Gudim <mgudim at ventanamicro.com>
Date: Wed, 10 Dec 2025 08:48:18 -0800
Subject: [PATCH 4/4] addressed review comments.
---
llvm/lib/CodeGen/CFIInstrInserter.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp
index 930d590a1e5b9..0047cdae1167a 100644
--- a/llvm/lib/CodeGen/CFIInstrInserter.cpp
+++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp
@@ -319,10 +319,13 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
case MCCFIInstruction::OpValOffset:
break;
}
+ assert((bool)CSRReg + (bool)CSROffset <= 1 &&
+ "A register can only be at an offset from CFA or in another "
+ "register, but not both!");
CSRSavedLocation CSRLoc;
if (CSRReg)
CSRLoc = CSRSavedLocation::createRegister(*CSRReg);
- if (CSROffset)
+ else if (CSROffset)
CSRLoc = CSRSavedLocation::createCFAOffset(*CSROffset);
if (CSRLoc.isValid()) {
auto It = CSRLocMap.find(CFI.getRegister());
More information about the llvm-commits
mailing list