[llvm] 58c108c - [NFC][AsmPrinter] Refactor DbgVariable as a std::variant
Scott Linder via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 11 10:33:02 PDT 2023
Author: Scott Linder
Date: 2023-09-11T17:31:59Z
New Revision: 58c108cde70bd6f2c39f9d57200c959090fd861a
URL: https://github.com/llvm/llvm-project/commit/58c108cde70bd6f2c39f9d57200c959090fd861a
DIFF: https://github.com/llvm/llvm-project/commit/58c108cde70bd6f2c39f9d57200c959090fd861a.diff
LOG: [NFC][AsmPrinter] Refactor DbgVariable as a std::variant
Only a subset of the fields of DbgVariable are meaningful at any time,
and some fields are re-used for multiple purposes (for example
FrameIndexExprs is used with a throw-away frame-index of 0 to hold a
single DIExpression without needing to add another member). The exact
invariants must be reverse-engineered by inspecting the actual use of
the class, its imprecise/outdated doc-comment, and some asserts.
Refactor DbgVariable into a sum type by inheriting from std::variant.
This makes the active fields for any given state explicit and removes
the need to re-use fields in disparate contexts. As a bonus, it seems to
reduce the size on my x86_64 linux box from 144 bytes to 96 bytes.
There is some potential cost to `std::get` as it must check the active
alternative even when context or an assert obviates it. To try to help
ensure the compiler can optimize out the checks the patch also adds a
helper `get` method which uses the noexcept `std::get_if`.
Some of the extra cost would also be avoided more cleanly with a
refactor that exposes the alternative types in the public interface,
which will come in another patch.
Differential Revision: https://reviews.llvm.org/D158675
Added:
Modified:
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index 938538ef7980462..fffbb28256341b9 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -882,12 +882,11 @@ DIE *DwarfCompileUnit::constructVariableDIEImpl(const DbgVariable &DV,
return VariableDie;
}
- if (const std::optional<DbgVariableEntryValue> &EntryValueVar =
- DV.getEntryValue()) {
+ if (const std::set<EntryValueInfo> *EntryValues = DV.getEntryValues()) {
DIELoc *Loc = new (DIEValueAllocator) DIELoc;
DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
// Emit each expression as: EntryValue(Register) <other ops> <Fragment>.
- for (auto [Register, Expr] : EntryValueVar->getEntryValuesInfo()) {
+ for (auto [Register, Expr] : *EntryValues) {
DwarfExpr.addFragmentOffset(&Expr);
DIExpressionCursor Cursor(Expr.getElements());
DwarfExpr.beginEntryValueExpression(Cursor);
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index ff59b7829435139..5f48b51046290f9 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -257,21 +257,37 @@ static DbgValueLoc getDebugLocValue(const MachineInstr *MI) {
return DbgValueLoc(Expr, DbgValueLocEntries, IsVariadic);
}
-void DbgVariable::initializeDbgValue(const MachineInstr *DbgValue) {
- assert(FrameIndexExprs.empty() && "Already initialized?");
- assert(!ValueLoc.get() && "Already initialized?");
+/// Initialize from the MMI table.
+void DbgVariable::initializeMMI(const DIExpression *E, int FI) {
+ assert(holds<std::monostate>() && "Already initialized");
+ assert((!E || E->isValid()) && "Expected valid expression");
+ assert(FI != std::numeric_limits<int>::max() && "Expected valid index");
+ emplace<MMILoc>().FrameIndexExprs.push_back({FI, E});
+}
+
+void DbgVariable::initializeDbgValue(DbgValueLoc Value) {
+ assert(holds<std::monostate>() && "Already initialized");
+ assert(!Value.getExpression()->isFragment() && "Fragments not supported");
+ emplace<SingleLoc>(Value);
+}
+void DbgVariable::initializeDbgValue(const MachineInstr *DbgValue) {
+ assert(holds<std::monostate>() && "Already initialized");
assert(getVariable() == DbgValue->getDebugVariable() && "Wrong variable");
assert(getInlinedAt() == DbgValue->getDebugLoc()->getInlinedAt() &&
"Wrong inlined-at");
+ emplace<SingleLoc>(getDebugLocValue(DbgValue));
+}
- ValueLoc = std::make_unique<DbgValueLoc>(getDebugLocValue(DbgValue));
- if (auto *E = DbgValue->getDebugExpression())
- if (E->getNumElements())
- FrameIndexExprs.push_back({0, E});
+void DbgVariable::initializeEntryValue(MCRegister Reg,
+ const DIExpression &Expr) {
+ assert(holds<std::monostate>() && "Already initialized?");
+ emplace<EntryValueLoc>(Reg, Expr);
}
-ArrayRef<DbgVariable::FrameIndexExpr> DbgVariable::getFrameIndexExprs() const {
+ArrayRef<FrameIndexExpr> DbgVariable::getFrameIndexExprs() const {
+ auto &FrameIndexExprs = get<MMILoc>().FrameIndexExprs;
+
if (FrameIndexExprs.size() == 1)
return FrameIndexExprs;
@@ -290,13 +306,11 @@ ArrayRef<DbgVariable::FrameIndexExpr> DbgVariable::getFrameIndexExprs() const {
}
void DbgVariable::addMMIEntry(const DbgVariable &V) {
- assert(DebugLocListIndex == ~0U && !ValueLoc.get() && "not an MMI entry");
- assert(V.DebugLocListIndex == ~0U && !V.ValueLoc.get() && "not an MMI entry");
assert(V.getVariable() == getVariable() && "conflicting variable");
assert(V.getInlinedAt() == getInlinedAt() && "conflicting inlined-at location");
- assert(!FrameIndexExprs.empty() && "Expected an MMI entry");
- assert(!V.FrameIndexExprs.empty() && "Expected an MMI entry");
+ auto &FrameIndexExprs = get<MMILoc>().FrameIndexExprs;
+ auto &VFrameIndexExprs = V.get<MMILoc>().FrameIndexExprs;
// FIXME: This logic should not be necessary anymore, as we now have proper
// deduplication. However, without it, we currently run into the assertion
@@ -308,7 +322,7 @@ void DbgVariable::addMMIEntry(const DbgVariable &V) {
return;
}
- for (const auto &FIE : V.FrameIndexExprs)
+ for (const auto &FIE : VFrameIndexExprs)
// Ignore duplicate entries.
if (llvm::none_of(FrameIndexExprs, [&](const FrameIndexExpr &Other) {
return FIE.FI == Other.FI && FIE.Expr == Other.Expr;
@@ -1573,7 +1587,7 @@ void DwarfDebug::collectVariableInfoFromMFTable(
if (DbgVar->hasFrameIndexExprs())
DbgVar->addMMIEntry(*RegVar);
else
- DbgVar->getEntryValue()->addExpr(VI.getEntryValueRegister(), *VI.Expr);
+ DbgVar->addEntryValueExpr(VI.getEntryValueRegister(), *VI.Expr);
} else if (InfoHolder.addScopeVariable(Scope, RegVar.get())) {
MFVars.insert({Var, RegVar.get()});
ConcreteEntities.push_back(std::move(RegVar));
diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
index dfbf0d1b7d8987e..7bede4e0df9e05d 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h
@@ -41,6 +41,7 @@
#include <limits>
#include <memory>
#include <utility>
+#include <variant>
#include <vector>
namespace llvm {
@@ -100,36 +101,59 @@ class DbgEntity {
}
};
-/// Helper class to model a DbgVariable whose location is derived from an
-/// EntryValue.
-/// TODO: split the current implementation of `DbgVariable` into a class per
-/// variant of location that it can represent, and make `DbgVariableEntryValue`
-/// a subclass.
-class DbgVariableEntryValue {
- struct EntryValueInfo {
- MCRegister Reg;
- const DIExpression &Expr;
-
- /// Operator enabling sorting based on fragment offset.
- bool operator<(const EntryValueInfo &Other) const {
- return getFragmentOffsetInBits() < Other.getFragmentOffsetInBits();
- }
+/// Proxy for one MMI entry.
+struct FrameIndexExpr {
+ int FI;
+ const DIExpression *Expr;
+};
- private:
- uint64_t getFragmentOffsetInBits() const {
- std::optional<DIExpression::FragmentInfo> Fragment =
- Expr.getFragmentInfo();
- return Fragment ? Fragment->OffsetInBits : 0;
- }
- };
+/// Represents an entry-value location, or a fragment of one.
+struct EntryValueInfo {
+ MCRegister Reg;
+ const DIExpression &Expr;
- std::set<EntryValueInfo> EntryValues;
+ /// Operator enabling sorting based on fragment offset.
+ bool operator<(const EntryValueInfo &Other) const {
+ return getFragmentOffsetInBits() < Other.getFragmentOffsetInBits();
+ }
-public:
- DbgVariableEntryValue(MCRegister Reg, const DIExpression &Expr) {
+private:
+ uint64_t getFragmentOffsetInBits() const {
+ std::optional<DIExpression::FragmentInfo> Fragment = Expr.getFragmentInfo();
+ return Fragment ? Fragment->OffsetInBits : 0;
+ }
+};
+
+// Namespace for private alternatives of a DbgVariable.
+namespace {
+/// Single value location description.
+struct SingleLoc {
+ std::unique_ptr<DbgValueLoc> ValueLoc;
+ const DIExpression *Expr;
+ SingleLoc(DbgValueLoc ValueLoc)
+ : ValueLoc(std::make_unique<DbgValueLoc>(ValueLoc)),
+ Expr(ValueLoc.getExpression()) {
+ if (!Expr->getNumElements())
+ Expr = nullptr;
+ }
+};
+/// Multi-value location description.
+struct MultiLoc {
+ /// Index of the entry list in DebugLocs.
+ unsigned DebugLocListIndex = ~0u;
+ /// DW_OP_LLVM_tag_offset value from DebugLocs.
+ std::optional<uint8_t> DebugLocListTagOffset;
+};
+/// Single location defined by (potentially multiple) MMI entries.
+struct MMILoc {
+ mutable SmallVector<FrameIndexExpr, 1> FrameIndexExprs;
+};
+/// Single location defined by (potentially multiple) EntryValueInfo.
+struct EntryValueLoc {
+ std::set<EntryValueInfo> EntryValues;
+ EntryValueLoc(MCRegister Reg, const DIExpression &Expr) {
addExpr(Reg, Expr);
};
-
// Add the pair Reg, Expr to the list of entry values describing the variable.
// If multiple expressions are added, it is the callers responsibility to
// ensure they are all non-overlapping fragments.
@@ -140,46 +164,48 @@ class DbgVariableEntryValue {
EntryValues.insert({Reg, **NonVariadicExpr});
}
-
- /// Returns the set of EntryValueInfo.
- const std::set<EntryValueInfo> &getEntryValuesInfo() const {
- return EntryValues;
- }
};
+} // namespace
//===----------------------------------------------------------------------===//
/// This class is used to track local variable information.
///
+/// Variables that have been optimized out hold the \c monostate alternative.
+/// This is not distinguished from the case of a constructed \c DbgVariable
+/// which has not be initialized yet.
+///
/// Variables can be created from allocas, in which case they're generated from
-/// the MMI table. Such variables can have multiple expressions and frame
-/// indices.
+/// the MMI table. Such variables hold the \c MMILoc alternative which can have
+/// multiple expressions and frame indices.
///
/// Variables can be created from the entry value of registers, in which case
-/// they're generated from the MMI table. Such variables can have either a
-/// single expression or multiple *fragment* expressions.
+/// they're generated from the MMI table. Such variables hold the \c
+/// EntryValueLoc alternative which can either have a single expression or
+/// multiple *fragment* expressions.
///
-/// Variables can be created from \c DBG_VALUE instructions. Those whose
-/// location changes over time use \a DebugLocListIndex, while those with a
-/// single location use \a ValueLoc and (optionally) a single entry of \a Expr.
-///
-/// Variables that have been optimized out use none of these fields.
-class DbgVariable : public DbgEntity {
- /// Index of the entry list in DebugLocs.
- unsigned DebugLocListIndex = ~0u;
- /// DW_OP_LLVM_tag_offset value from DebugLocs.
- std::optional<uint8_t> DebugLocListTagOffset;
-
- /// Single value location description.
- std::unique_ptr<DbgValueLoc> ValueLoc = nullptr;
-
- struct FrameIndexExpr {
- int FI;
- const DIExpression *Expr;
- };
- mutable SmallVector<FrameIndexExpr, 1>
- FrameIndexExprs; /// Frame index + expression.
-
- std::optional<DbgVariableEntryValue> EntryValue;
+/// Variables can be created from \c DBG_VALUE instructions. Those whose
+/// location changes over time hold a \c MultiLoc alternative which uses \c
+/// DebugLocListIndex and (optionally) \c DebugLocListTagOffset, while those
+/// with a single location hold a \c SingleLoc alternative which use \c ValueLoc
+/// and (optionally) a single \c Expr.
+class DbgVariable : public DbgEntity,
+ private std::variant<std::monostate, SingleLoc, MultiLoc,
+ MMILoc, EntryValueLoc> {
+
+ /// Member shorthand for std::holds_alternative
+ template <typename T> bool holds() const {
+ return std::holds_alternative<T>(*this);
+ }
+ /// Asserting, noexcept member alternative to std::get
+ template <typename T> auto &get() noexcept {
+ assert(holds<T>());
+ return *std::get_if<T>(this);
+ }
+ /// Asserting, noexcept member alternative to std::get
+ template <typename T> const auto &get() const noexcept {
+ assert(holds<T>());
+ return *std::get_if<T>(this);
+ }
public:
/// Construct a DbgVariable.
@@ -189,41 +215,27 @@ class DbgVariable : public DbgEntity {
DbgVariable(const DILocalVariable *V, const DILocation *IA)
: DbgEntity(V, IA, DbgVariableKind) {}
- bool isInitialized() const {
- return !FrameIndexExprs.empty() || ValueLoc || EntryValue;
- }
-
/// Initialize from the MMI table.
- void initializeMMI(const DIExpression *E, int FI) {
- assert(!isInitialized() && "Already initialized?");
-
- assert((!E || E->isValid()) && "Expected valid expression");
- assert(FI != std::numeric_limits<int>::max() && "Expected valid index");
-
- FrameIndexExprs.push_back({FI, E});
- }
+ void initializeMMI(const DIExpression *E, int FI);
// Initialize variable's location.
- void initializeDbgValue(DbgValueLoc Value) {
- assert(!isInitialized() && "Already initialized?");
- assert(!Value.getExpression()->isFragment() && "Fragments not supported.");
+ void initializeDbgValue(DbgValueLoc Value);
- ValueLoc = std::make_unique<DbgValueLoc>(Value);
- if (auto *E = ValueLoc->getExpression())
- if (E->getNumElements())
- FrameIndexExprs.push_back({0, E});
- }
+ void initializeEntryValue(MCRegister Reg, const DIExpression &Expr);
- void initializeEntryValue(MCRegister Reg, const DIExpression &Expr) {
- assert(!isInitialized() && "Already initialized?");
- EntryValue = DbgVariableEntryValue(Reg, Expr);
+ const std::set<EntryValueInfo> *getEntryValues() const {
+ if (const auto *EV = std::get_if<EntryValueLoc>(this))
+ return &EV->EntryValues;
+ return nullptr;
}
-
- const std::optional<DbgVariableEntryValue> &getEntryValue() const {
- return EntryValue;
+ std::set<EntryValueInfo> *getEntryValues() {
+ if (auto *EV = std::get_if<EntryValueLoc>(this))
+ return &EV->EntryValues;
+ return nullptr;
+ }
+ void addEntryValueExpr(MCRegister Reg, const DIExpression &Expr) {
+ get<EntryValueLoc>().addExpr(Reg, Expr);
}
-
- std::optional<DbgVariableEntryValue> &getEntryValue() { return EntryValue; }
/// Initialize from a DBG_VALUE instruction.
void initializeDbgValue(const MachineInstr *DbgValue);
@@ -234,21 +246,38 @@ class DbgVariable : public DbgEntity {
}
const DIExpression *getSingleExpression() const {
- assert(ValueLoc.get() && FrameIndexExprs.size() <= 1);
- return FrameIndexExprs.size() ? FrameIndexExprs[0].Expr : nullptr;
+ return get<SingleLoc>().Expr;
}
- void setDebugLocListIndex(unsigned O) { DebugLocListIndex = O; }
- unsigned getDebugLocListIndex() const { return DebugLocListIndex; }
- void setDebugLocListTagOffset(uint8_t O) { DebugLocListTagOffset = O; }
+ void setDebugLocListIndex(uint8_t O) {
+ if (auto *M = std::get_if<MultiLoc>(this))
+ M->DebugLocListIndex = O;
+ else
+ emplace<MultiLoc>().DebugLocListIndex = O;
+ }
+ unsigned getDebugLocListIndex() const {
+ if (auto *M = std::get_if<MultiLoc>(this))
+ return M->DebugLocListIndex;
+ return ~0U;
+ }
+ void setDebugLocListTagOffset(uint8_t O) {
+ if (auto *M = std::get_if<MultiLoc>(this))
+ M->DebugLocListTagOffset = O;
+ else
+ emplace<MultiLoc>().DebugLocListTagOffset = O;
+ }
std::optional<uint8_t> getDebugLocListTagOffset() const {
- return DebugLocListTagOffset;
+ return get<MultiLoc>().DebugLocListTagOffset;
}
StringRef getName() const { return getVariable()->getName(); }
- const DbgValueLoc *getValueLoc() const { return ValueLoc.get(); }
+ const DbgValueLoc *getValueLoc() const {
+ if (auto *M = std::get_if<SingleLoc>(this))
+ return M->ValueLoc.get();
+ return nullptr;
+ }
/// Get the FI entries, sorted by fragment offset.
ArrayRef<FrameIndexExpr> getFrameIndexExprs() const;
- bool hasFrameIndexExprs() const { return !FrameIndexExprs.empty(); }
+ bool hasFrameIndexExprs() const { return holds<MMILoc>(); }
void addMMIEntry(const DbgVariable &V);
// Translate tag to proper Dwarf tag.
@@ -277,14 +306,7 @@ class DbgVariable : public DbgEntity {
return false;
}
- bool hasComplexAddress() const {
- assert(ValueLoc.get() && "Expected DBG_VALUE, not MMI variable");
- assert((FrameIndexExprs.empty() ||
- (FrameIndexExprs.size() == 1 &&
- FrameIndexExprs[0].Expr->getNumElements())) &&
- "Invalid Expr for DBG_VALUE");
- return !FrameIndexExprs.empty();
- }
+ bool hasComplexAddress() const { return get<SingleLoc>().Expr; }
const DIType *getType() const;
More information about the llvm-commits
mailing list