Skip to content

[DebugInfo] Make DIArgList inherit from Metadata and always unique #72147

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 17, 2023
Merged

[DebugInfo] Make DIArgList inherit from Metadata and always unique #72147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions llvm/include/llvm/AsmParser/LLParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,7 @@ namespace llvm {
bool parseMetadataAsValue(Value *&V, PerFunctionState &PFS);
bool parseValueAsMetadata(Metadata *&MD, const Twine &TypeMsg,
PerFunctionState *PFS);
bool parseDIArgList(Metadata *&MD, PerFunctionState *PFS);
bool parseMetadata(Metadata *&MD, PerFunctionState *PFS);
bool parseMDTuple(MDNode *&MD, bool IsDistinct = false);
bool parseMDNode(MDNode *&N);
Expand All @@ -569,8 +570,6 @@ namespace llvm {
#define HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS) \
bool parse##CLASS(MDNode *&Result, bool IsDistinct);
#include "llvm/IR/Metadata.def"
bool parseDIArgList(MDNode *&Result, bool IsDistinct,
PerFunctionState *PFS);

// Function Parsing.
struct ArgInfo {
Expand Down
31 changes: 10 additions & 21 deletions llvm/include/llvm/IR/DebugInfoMetadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3753,51 +3753,40 @@ class DIMacroFile : public DIMacroNode {

/// List of ValueAsMetadata, to be used as an argument to a dbg.value
/// intrinsic.
class DIArgList : public MDNode {
class DIArgList : public Metadata, ReplaceableMetadataImpl {
friend class ReplaceableMetadataImpl;
friend class LLVMContextImpl;
friend class MDNode;
using iterator = SmallVectorImpl<ValueAsMetadata *>::iterator;

SmallVector<ValueAsMetadata *, 4> Args;

DIArgList(LLVMContext &C, StorageType Storage,
ArrayRef<ValueAsMetadata *> Args)
: MDNode(C, DIArgListKind, Storage, std::nullopt),
DIArgList(LLVMContext &Context, ArrayRef<ValueAsMetadata *> Args)
: Metadata(DIArgListKind, Uniqued), ReplaceableMetadataImpl(Context),
Args(Args.begin(), Args.end()) {
track();
}
~DIArgList() { untrack(); }

static DIArgList *getImpl(LLVMContext &Context,
ArrayRef<ValueAsMetadata *> Args,
StorageType Storage, bool ShouldCreate = true);

TempDIArgList cloneImpl() const {
return getTemporary(getContext(), getArgs());
}

void track();
void untrack();
void dropAllReferences();
void dropAllReferences(bool Untrack);

public:
DEFINE_MDNODE_GET(DIArgList, (ArrayRef<ValueAsMetadata *> Args), (Args))

TempDIArgList clone() const { return cloneImpl(); }
static DIArgList *get(LLVMContext &Context, ArrayRef<ValueAsMetadata *> Args);

ArrayRef<ValueAsMetadata *> getArgs() const { return Args; }

iterator args_begin() { return Args.begin(); }
iterator args_end() { return Args.end(); }

ReplaceableMetadataImpl *getReplaceableUses() {
return Context.getReplaceableUses();
}

static bool classof(const Metadata *MD) {
return MD->getMetadataID() == DIArgListKind;
}

SmallVector<DPValue *> getAllDPValueUsers() {
return ReplaceableMetadataImpl::getAllDPValueUsers();
}

void handleChangedOperand(void *Ref, Metadata *New);
};

Expand Down
2 changes: 1 addition & 1 deletion llvm/include/llvm/IR/Metadata.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ HANDLE_METADATA_BRANCH(ValueAsMetadata)
HANDLE_METADATA_LEAF(ConstantAsMetadata)
HANDLE_METADATA_LEAF(LocalAsMetadata)
HANDLE_METADATA_LEAF(DistinctMDOperandPlaceholder)
HANDLE_METADATA_LEAF(DIArgList)
HANDLE_MDNODE_BRANCH(MDNode)
HANDLE_MDNODE_LEAF_UNIQUABLE(MDTuple)
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILocation)
Expand Down Expand Up @@ -115,7 +116,6 @@ HANDLE_SPECIALIZED_MDNODE_BRANCH(DIMacroNode)
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIMacro)
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIMacroFile)
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DICommonBlock)
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIArgList)
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIStringType)
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIGenericSubrange)

Expand Down
5 changes: 1 addition & 4 deletions llvm/include/llvm/IR/Metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,7 +1037,6 @@ struct TempMDNodeDeleter {
class MDNode : public Metadata {
friend class ReplaceableMetadataImpl;
friend class LLVMContextImpl;
friend class DIArgList;

/// The header that is coallocated with an MDNode along with its "small"
/// operands. It is located immediately before the main body of the node.
Expand Down Expand Up @@ -1220,9 +1219,7 @@ class MDNode : public Metadata {
bool isDistinct() const { return Storage == Distinct; }
bool isTemporary() const { return Storage == Temporary; }

bool isReplaceable() const {
return isTemporary() || getMetadataID() == DIArgListKind;
}
bool isReplaceable() const { return isTemporary(); }

/// RAUW a temporary.
///
Expand Down
18 changes: 9 additions & 9 deletions llvm/lib/AsmParser/LLParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5486,13 +5486,9 @@ bool LLParser::parseDIExpression(MDNode *&Result, bool IsDistinct) {
return false;
}

bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct) {
return parseDIArgList(Result, IsDistinct, nullptr);
}
/// ParseDIArgList:
/// ::= !DIArgList(i32 7, i64 %0)
bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct,
PerFunctionState *PFS) {
bool LLParser::parseDIArgList(Metadata *&MD, PerFunctionState *PFS) {
assert(PFS && "Expected valid function state");
assert(Lex.getKind() == lltok::MetadataVar && "Expected metadata type name");
Lex.Lex();
Expand All @@ -5512,7 +5508,7 @@ bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct,
if (parseToken(lltok::rparen, "expected ')' here"))
return true;

Result = GET_OR_DISTINCT(DIArgList, (Context, Args));
MD = DIArgList::get(Context, Args);
return false;
}

Expand Down Expand Up @@ -5626,13 +5622,17 @@ bool LLParser::parseValueAsMetadata(Metadata *&MD, const Twine &TypeMsg,
/// ::= !DILocation(...)
bool LLParser::parseMetadata(Metadata *&MD, PerFunctionState *PFS) {
if (Lex.getKind() == lltok::MetadataVar) {
MDNode *N;
// DIArgLists are a special case, as they are a list of ValueAsMetadata and
// so parsing this requires a Function State.
if (Lex.getStrVal() == "DIArgList") {
if (parseDIArgList(N, false, PFS))
Metadata *AL;
if (parseDIArgList(AL, PFS))
return true;
} else if (parseSpecializedMDNode(N)) {
MD = AL;
return false;
}
MDNode *N;
if (parseSpecializedMDNode(N)) {
return true;
}
MD = N;
Expand Down
12 changes: 7 additions & 5 deletions llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,7 @@ class ModuleBitcodeWriter : public ModuleBitcodeWriterBase {
unsigned Abbrev);
void writeDIMacroFile(const DIMacroFile *N, SmallVectorImpl<uint64_t> &Record,
unsigned Abbrev);
void writeDIArgList(const DIArgList *N, SmallVectorImpl<uint64_t> &Record,
unsigned Abbrev);
void writeDIArgList(const DIArgList *N, SmallVectorImpl<uint64_t> &Record);
void writeDIModule(const DIModule *N, SmallVectorImpl<uint64_t> &Record,
unsigned Abbrev);
void writeDIAssignID(const DIAssignID *N, SmallVectorImpl<uint64_t> &Record,
Expand Down Expand Up @@ -1975,13 +1974,12 @@ void ModuleBitcodeWriter::writeDIMacroFile(const DIMacroFile *N,
}

void ModuleBitcodeWriter::writeDIArgList(const DIArgList *N,
SmallVectorImpl<uint64_t> &Record,
unsigned Abbrev) {
SmallVectorImpl<uint64_t> &Record) {
Record.reserve(N->getArgs().size());
for (ValueAsMetadata *MD : N->getArgs())
Record.push_back(VE.getMetadataID(MD));

Stream.EmitRecord(bitc::METADATA_ARG_LIST, Record, Abbrev);
Stream.EmitRecord(bitc::METADATA_ARG_LIST, Record);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding, what are the implications of not having an abbrev here -- will we end up with a potentially suboptimal encoding or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DIArgLists have no abbreviations, so this is a no-op I believe.

Record.clear();
}

Expand Down Expand Up @@ -2264,6 +2262,10 @@ void ModuleBitcodeWriter::writeMetadataRecords(
#include "llvm/IR/Metadata.def"
}
}
if (auto *AL = dyn_cast<DIArgList>(MD)) {
writeDIArgList(AL, Record);
continue;
}
writeValueAsMetadata(cast<ValueAsMetadata>(MD), Record);
}
}
Expand Down
11 changes: 4 additions & 7 deletions llvm/lib/IR/AsmWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,9 +1265,8 @@ void SlotTracker::CreateFunctionSlot(const Value *V) {
void SlotTracker::CreateMetadataSlot(const MDNode *N) {
assert(N && "Can't insert a null Value into SlotTracker!");

// Don't make slots for DIExpressions or DIArgLists. We just print them inline
// everywhere.
if (isa<DIExpression>(N) || isa<DIArgList>(N))
// Don't make slots for DIExpressions. We just print them inline everywhere.
if (isa<DIExpression>(N))
Comment on lines +1268 to +1269
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm my understanding: the mechanism of print DIArgLists isn't changing at all, but as they're no longer MDNodes, they don't get considered for a metadata-node-number slot so don't need to appear here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this function only gets called for MDNodes, so DIArgList doesn't need to care anymore.

return;

unsigned DestSlot = mdnNext;
Expand Down Expand Up @@ -3516,8 +3515,6 @@ void AssemblyWriter::printNamedMDNode(const NamedMDNode *NMD) {
// Write DIExpressions inline.
// FIXME: Ban DIExpressions in NamedMDNodes, they will serve no purpose.
MDNode *Op = NMD->getOperand(i);
assert(!isa<DIArgList>(Op) &&
"DIArgLists should not appear in NamedMDNodes");
if (auto *Expr = dyn_cast<DIExpression>(Op)) {
writeDIExpression(Out, Expr, AsmWriterContext::getEmpty());
continue;
Expand Down Expand Up @@ -4918,7 +4915,7 @@ static void printMetadataImplRec(raw_ostream &ROS, const Metadata &MD,
WriteAsOperandInternal(OS, &MD, WriterCtx, /* FromValue */ true);

auto *N = dyn_cast<MDNode>(&MD);
if (!N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
if (!N || isa<DIExpression>(MD))
return;

OS << " = ";
Expand Down Expand Up @@ -4986,7 +4983,7 @@ static void printMetadataImpl(raw_ostream &ROS, const Metadata &MD,
WriteAsOperandInternal(OS, &MD, *WriterCtx, /* FromValue */ true);

auto *N = dyn_cast<MDNode>(&MD);
if (OnlyAsOperand || !N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
if (OnlyAsOperand || !N || isa<DIExpression>(MD))
return;

OS << " = ";
Expand Down
62 changes: 27 additions & 35 deletions llvm/lib/IR/DebugInfoMetadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2115,24 +2115,24 @@ DIMacroFile *DIMacroFile::getImpl(LLVMContext &Context, unsigned MIType,
DEFINE_GETIMPL_STORE(DIMacroFile, (MIType, Line), Ops);
}

DIArgList *DIArgList::getImpl(LLVMContext &Context,
ArrayRef<ValueAsMetadata *> Args,
StorageType Storage, bool ShouldCreate) {
DEFINE_GETIMPL_LOOKUP(DIArgList, (Args));
DEFINE_GETIMPL_STORE_NO_OPS(DIArgList, (Args));
DIArgList *DIArgList::get(LLVMContext &Context,
ArrayRef<ValueAsMetadata *> Args) {
auto ExistingIt = Context.pImpl->DIArgLists.find_as(DIArgListKeyInfo(Args));
if (ExistingIt != Context.pImpl->DIArgLists.end())
return *ExistingIt;
DIArgList *NewArgList = new DIArgList(Context, Args);
Context.pImpl->DIArgLists.insert(NewArgList);
return NewArgList;
}

void DIArgList::handleChangedOperand(void *Ref, Metadata *New) {
ValueAsMetadata **OldVMPtr = static_cast<ValueAsMetadata **>(Ref);
assert((!New || isa<ValueAsMetadata>(New)) &&
"DIArgList must be passed a ValueAsMetadata");
untrack();
bool Uniq = isUniqued();
if (Uniq) {
// We need to update the uniqueness once the Args are updated since they
// form the key to the DIArgLists store.
eraseFromStore();
}
// We need to update the set storage once the Args are updated since they
// form the key to the DIArgLists store.
getContext().pImpl->DIArgLists.erase(this);
ValueAsMetadata *NewVM = cast_or_null<ValueAsMetadata>(New);
for (ValueAsMetadata *&VM : Args) {
if (&VM == OldVMPtr) {
Expand All @@ -2142,28 +2142,19 @@ void DIArgList::handleChangedOperand(void *Ref, Metadata *New) {
VM = ValueAsMetadata::get(PoisonValue::get(VM->getValue()->getType()));
}
}
if (Uniq) {
// In the RemoveDIs project (eliminating debug-info-intrinsics), DIArgLists
// can be referred to by DebugValueUser objects, which necessitates them
// being unique and replaceable metadata. This causes a slight
// performance regression that's to be avoided during the early stages of
// the RemoveDIs prototype, see D154080.
#ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS
MDNode *UniqueArgList = uniquify();
if (UniqueArgList != this) {
replaceAllUsesWith(UniqueArgList);
// Clear this here so we don't try to untrack in the destructor.
Args.clear();
delete this;
return;
}
#else
// Otherwise, don't fully unique, become distinct instead. See D108968,
// there's a latent bug that presents here as nondeterminism otherwise.
if (uniquify() != this)
storeDistinctInContext();
#endif
// We've changed the contents of this DIArgList, and the set storage may
// already contain a DIArgList with our new set of args; if it does, then we
// must RAUW this with the existing DIArgList, otherwise we simply insert this
// back into the set storage.
DIArgList *ExistingArgList = getUniqued(getContext().pImpl->DIArgLists, this);
if (ExistingArgList) {
replaceAllUsesWith(ExistingArgList);
// Clear this here so we don't try to untrack in the destructor.
Args.clear();
delete this;
return;
}
getContext().pImpl->DIArgLists.insert(this);
track();
}
void DIArgList::track() {
Expand All @@ -2176,8 +2167,9 @@ void DIArgList::untrack() {
if (VAM)
MetadataTracking::untrack(&VAM, *VAM);
}
void DIArgList::dropAllReferences() {
untrack();
void DIArgList::dropAllReferences(bool Untrack) {
if (Untrack)
untrack();
Args.clear();
MDNode::dropAllReferences();
ReplaceableMetadataImpl::resolveAllUses(/* ResolveUsers */ false);
}
13 changes: 5 additions & 8 deletions llvm/lib/IR/LLVMContextImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,8 @@ LLVMContextImpl::~LLVMContextImpl() {

// Drop references for MDNodes. Do this before Values get deleted to avoid
// unnecessary RAUW when nodes are still unresolved.
for (auto *I : DistinctMDNodes) {
// We may have DIArgList that were uniqued, and as it has a custom
// implementation of dropAllReferences, it needs to be explicitly invoked.
if (auto *AL = dyn_cast<DIArgList>(I)) {
AL->dropAllReferences();
continue;
}
for (auto *I : DistinctMDNodes)
I->dropAllReferences();
}
#define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS) \
for (auto *I : CLASS##s) \
I->dropAllReferences();
Expand All @@ -87,6 +80,10 @@ LLVMContextImpl::~LLVMContextImpl() {
Pair.second->dropUsers();
for (auto &Pair : MetadataAsValues)
Pair.second->dropUse();
// Do not untrack ValueAsMetadata references for DIArgLists, as they have
// already been more efficiently untracked above.
for (DIArgList *AL : DIArgLists)
AL->dropAllReferences(/* Untrack */ false);

// Destroy MDNodes.
for (MDNode *I : DistinctMDNodes)
Expand Down
Loading