-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[DwarfDebug] Track abstract entities in DwarfUnit separately #152680
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,12 @@ | |
#include "llvm/ADT/SmallVector.h" | ||
#include "llvm/ADT/StringRef.h" | ||
#include "llvm/CodeGen/DIE.h" | ||
#include "llvm/IR/DebugInfoMetadata.h" | ||
#include "llvm/Support/Allocator.h" | ||
#include <functional> | ||
#include <map> | ||
#include <memory> | ||
#include <optional> | ||
#include <utility> | ||
|
||
namespace llvm { | ||
|
@@ -26,9 +29,6 @@ class AsmPrinter; | |
class DbgEntity; | ||
class DbgVariable; | ||
class DbgLabel; | ||
class DINode; | ||
class DILocalScope; | ||
class DISubprogram; | ||
class DwarfCompileUnit; | ||
class DwarfUnit; | ||
class LexicalScope; | ||
|
@@ -53,6 +53,137 @@ struct RangeSpanList { | |
SmallVector<RangeSpan, 2> Ranges; | ||
}; | ||
|
||
/// Tracks abstract and concrete DIEs for debug info entities of a certain type. | ||
template <typename DINodeT, typename DbgEntityT> class DINodeInfoHolder { | ||
public: | ||
using AbstractMapT = DenseMap<const DINodeT *, DIE *>; | ||
using ConcreteMapT = | ||
DenseMap<const DINodeT *, SmallDenseMap<const DbgEntityT *, DIE *, 2>>; | ||
|
||
private: | ||
AbstractMapT AbstractMap; | ||
ConcreteMapT ConcreteMap; | ||
|
||
public: | ||
void insertAbstractDIE(const DINodeT *N, DIE *D) { | ||
auto [_, Inserted] = AbstractMap.try_emplace(N, D); | ||
assert(Inserted && "Duplicate abstract DIE for debug info node"); | ||
} | ||
|
||
void insertConcreteDIE(const DINodeT *N, const DbgEntityT *E, DIE *D) { | ||
auto [_, Inserted] = ConcreteMap[N].try_emplace(E, D); | ||
assert(Inserted && "Duplicate concrete DIE for debug info node"); | ||
} | ||
|
||
void insertDIE(const DINodeT *N, const DbgEntityT *E, DIE *D, bool Abstract) { | ||
if (Abstract) | ||
insertAbstractDIE(N, D); | ||
else | ||
insertConcreteDIE(N, E, D); | ||
} | ||
|
||
DIE *getAbstractDIE(const DINodeT *N) const { return AbstractMap.lookup(N); } | ||
|
||
std::optional< | ||
std::reference_wrapper<const typename ConcreteMapT::mapped_type>> | ||
getConcreteDIEs(const DINodeT *N) const { | ||
if (auto I = ConcreteMap.find(N); I != ConcreteMap.end()) | ||
return std::make_optional(std::ref(I->second)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. optional references are a bit awkward - could we use a raw DIE*? (or does it need to be a DIE**, I guess? That'd be OK, if a bit awkward in its own right) |
||
return std::nullopt; | ||
} | ||
|
||
DIE *getConcreteDIE(const DINodeT *N, const DbgEntityT *E) const { | ||
if (auto I = getConcreteDIEs(N)) | ||
return I->get().lookup(E); | ||
return nullptr; | ||
} | ||
|
||
DIE *getAnyConcreteDIE(const DINodeT *N) const { | ||
if (auto I = getConcreteDIEs(N)) | ||
return I->get().empty() ? nullptr : I->get().begin()->second; | ||
return nullptr; | ||
} | ||
|
||
/// Returns abstract DIE for the entity. | ||
/// If no abstract DIE was created, returns any concrete DIE for the entity. | ||
DIE *getDIE(const DINodeT *N) const { | ||
if (DIE *D = getAbstractDIE(N)) | ||
return D; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra blank line could be removed to make this consistent with similar code above? |
||
return getAnyConcreteDIE(N); | ||
} | ||
|
||
AbstractMapT &getAbstractDIEs() { return AbstractMap; } | ||
}; | ||
|
||
/// Tracks DIEs for debug info entites. | ||
/// These DIEs can be shared across CUs, that is why we keep the map here | ||
/// instead of in DwarfCompileUnit. | ||
class DwarfInfoHolder { | ||
/// DIEs of local DbgVariables. | ||
DINodeInfoHolder<DILocalVariable, DbgVariable> LVHolder; | ||
/// DIEs of labels. | ||
DINodeInfoHolder<DILabel, DbgLabel> LabelHolder; | ||
DenseMap<const DINode *, std::unique_ptr<DbgEntity>> AbstractEntities; | ||
// List of abstract local scopes (either DISubprogram or DILexicalBlock). | ||
DenseMap<const DILocalScope *, DIE *> AbstractLocalScopeDIEs; | ||
/// Keeps track of abstract subprograms to populate them only once. | ||
// FIXME: merge creation and population of abstract scopes. | ||
SmallPtrSet<const DISubprogram *, 8> FinalizedAbstractSubprograms; | ||
|
||
/// Other DINodes with the corresponding DIEs. | ||
DenseMap<const DINode *, DIE *> MDNodeToDieMap; | ||
Comment on lines
+122
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this creates a union of the way we were tracking the node<>die map at the file level and the unit level - and then stores that union at both the file and the unit level? That seems like a bit of added complexity (but I guess it's the title of the patch). I guess I didn't quite follow from the patch description why that's the goal. Could you try to summarize/rephrase this goal/motivation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, we have fields, where the DINodes shared across CUs are stored:
And we have the corresponding fields in DwarfUnit/DwarfCompileUnit:
The first goal of DwarfInfoHolder is to deduplicate these field definitions by putting them inside one "container", and reuse that container when needed. DwarfUnit::InfoHolder will store CU-local (or type-unit-local) DIEs, DwarfFile::InfoHolder will store shareable DIEs.
So, an instance of DwarfInfoHolder holds either shareable or CU-local DIEs. It's not a union of them. Second goal is to split the general DIE map into separate maps for DILocalVariables, DILabels, DILocalScopes (in the following two PRs), and a map for the rest of DINodes (mostly types), since, as I assume, we can get DILocalVariables/DILabels/DILocalScopes used in multiple llvm::Functions during LTO with DISubprogram uniquing, so DIEs for these nodes created in different Functions should be somehow distinguished. It's done with additional DbgEntityT pointer. |
||
|
||
public: | ||
void insertDIE(const DINode *N, DIE *Die) { | ||
assert((!isa<DILabel>(N) && !isa<DILocalVariable>(N)) && | ||
"Use getLabels().insertDIE() for labels or getLVs().insertDIE() for " | ||
"local variables"); | ||
auto [_, Inserted] = MDNodeToDieMap.try_emplace(N, Die); | ||
assert((Inserted || isa<DISubprogram>(N) || isa<DIType>(N)) && | ||
"DIE for this DINode has already been added"); | ||
} | ||
|
||
void insertDIE(DIE *D) { MDNodeToDieMap.try_emplace(nullptr, D); } | ||
|
||
DIE *getDIE(const DINode *N) const { | ||
DIE *D = MDNodeToDieMap.lookup(N); | ||
assert((!D || (!isa<DILabel>(N) && !isa<DILocalVariable>(N))) && | ||
"Use getLabels().getDIE() for labels or getLVs().getDIE() for " | ||
"local variables"); | ||
return D; | ||
} | ||
|
||
auto &getLVs() { return LVHolder; } | ||
auto &getLVs() const { return LVHolder; } | ||
|
||
auto &getLabels() { return LabelHolder; } | ||
auto &getLabels() const { return LabelHolder; } | ||
Comment on lines
+157
to
+161
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably worth spelling out the return types |
||
|
||
/// For a global variable, returns DIE of the variable. | ||
/// | ||
/// For a local variable, returns abstract DIE of the variable. | ||
/// If no abstract DIE was created, returns any concrete DIE of the variable. | ||
DIE *getVariableDIE(const DIVariable *V) const { | ||
if (auto *LV = dyn_cast<DILocalVariable>(V)) | ||
if (DIE *D = getLVs().getDIE(LV)) | ||
return D; | ||
return getDIE(V); | ||
} | ||
|
||
DenseMap<const DILocalScope *, DIE *> &getAbstractScopeDIEs() { | ||
return AbstractLocalScopeDIEs; | ||
} | ||
|
||
DenseMap<const DINode *, std::unique_ptr<DbgEntity>> &getAbstractEntities() { | ||
return AbstractEntities; | ||
} | ||
|
||
auto &getFinalizedAbstractSubprograms() { | ||
return FinalizedAbstractSubprograms; | ||
} | ||
}; | ||
|
||
class DwarfFile { | ||
// Target of Dwarf emission, used for sizing of abbreviations. | ||
AsmPrinter *Asm; | ||
|
@@ -93,17 +224,7 @@ class DwarfFile { | |
using LabelList = SmallVector<DbgLabel *, 4>; | ||
DenseMap<LexicalScope *, LabelList> ScopeLabels; | ||
|
||
// Collection of abstract subprogram DIEs. | ||
DenseMap<const DILocalScope *, DIE *> AbstractLocalScopeDIEs; | ||
DenseMap<const DINode *, std::unique_ptr<DbgEntity>> AbstractEntities; | ||
/// Keeps track of abstract subprograms to populate them only once. | ||
// FIXME: merge creation and population of abstract scopes. | ||
SmallPtrSet<const DISubprogram *, 8> FinalizedAbstractSubprograms; | ||
|
||
/// Maps MDNodes for type system with the corresponding DIEs. These DIEs can | ||
/// be shared across CUs, that is why we keep the map here instead | ||
/// of in DwarfCompileUnit. | ||
DenseMap<const MDNode *, DIE *> DITypeNodeToDieMap; | ||
DwarfInfoHolder InfoHolder; | ||
|
||
public: | ||
DwarfFile(AsmPrinter *AP, StringRef Pref, BumpPtrAllocator &DA); | ||
|
@@ -171,25 +292,7 @@ class DwarfFile { | |
return ScopeLabels; | ||
} | ||
|
||
DenseMap<const DILocalScope *, DIE *> &getAbstractScopeDIEs() { | ||
return AbstractLocalScopeDIEs; | ||
} | ||
|
||
DenseMap<const DINode *, std::unique_ptr<DbgEntity>> &getAbstractEntities() { | ||
return AbstractEntities; | ||
} | ||
|
||
auto &getFinalizedAbstractSubprograms() { | ||
return FinalizedAbstractSubprograms; | ||
} | ||
|
||
void insertDIE(const MDNode *TypeMD, DIE *Die) { | ||
DITypeNodeToDieMap.insert(std::make_pair(TypeMD, Die)); | ||
} | ||
|
||
DIE *getDIE(const MDNode *TypeMD) { | ||
return DITypeNodeToDieMap.lookup(TypeMD); | ||
} | ||
DwarfInfoHolder &getDIEs() { return InfoHolder; } | ||
}; | ||
|
||
} // end namespace llvm | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be DINodeMap?