Skip to content

Commit 0fd5dc9

Browse files
SLTozerSLTozer
authored
Revert "[DebugInfo] Make DIArgList inherit from Metadata and always unique" (#72682)
Reverts #72147 Reverted due to buildbot failure: https://lab.llvm.org/buildbot/#/builders/5/builds/38410
1 parent 6b56dd6 commit 0fd5dc9

13 files changed

+123
-119
lines changed

‎llvm/include/llvm/AsmParser/LLParser.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,6 @@ namespace llvm {
548548
bool parseMetadataAsValue(Value *&V, PerFunctionState &PFS);
549549
bool parseValueAsMetadata(Metadata *&MD, const Twine &TypeMsg,
550550
PerFunctionState *PFS);
551-
bool parseDIArgList(Metadata *&MD, PerFunctionState *PFS);
552551
bool parseMetadata(Metadata *&MD, PerFunctionState *PFS);
553552
bool parseMDTuple(MDNode *&MD, bool IsDistinct = false);
554553
bool parseMDNode(MDNode *&N);
@@ -570,6 +569,8 @@ namespace llvm {
570569
#define HANDLE_SPECIALIZED_MDNODE_LEAF(CLASS) \
571570
bool parse##CLASS(MDNode *&Result, bool IsDistinct);
572571
#include "llvm/IR/Metadata.def"
572+
bool parseDIArgList(MDNode *&Result, bool IsDistinct,
573+
PerFunctionState *PFS);
573574

574575
// Function Parsing.
575576
struct ArgInfo {

‎llvm/include/llvm/IR/DebugInfoMetadata.h

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3753,38 +3753,49 @@ class DIMacroFile : public DIMacroNode {
37533753

37543754
/// List of ValueAsMetadata, to be used as an argument to a dbg.value
37553755
/// intrinsic.
3756-
class DIArgList : public Metadata, ReplaceableMetadataImpl {
3757-
friend class ReplaceableMetadataImpl;
3756+
class DIArgList : public MDNode {
37583757
friend class LLVMContextImpl;
3758+
friend class MDNode;
37593759
using iterator = SmallVectorImpl<ValueAsMetadata *>::iterator;
37603760

37613761
SmallVector<ValueAsMetadata *, 4> Args;
37623762

3763-
DIArgList(LLVMContext &Context, ArrayRef<ValueAsMetadata *> Args)
3764-
: Metadata(DIArgListKind, Uniqued), ReplaceableMetadataImpl(Context),
3763+
DIArgList(LLVMContext &C, StorageType Storage,
3764+
ArrayRef<ValueAsMetadata *> Args)
3765+
: MDNode(C, DIArgListKind, Storage, std::nullopt),
37653766
Args(Args.begin(), Args.end()) {
37663767
track();
37673768
}
37683769
~DIArgList() { untrack(); }
37693770

3771+
static DIArgList *getImpl(LLVMContext &Context,
3772+
ArrayRef<ValueAsMetadata *> Args,
3773+
StorageType Storage, bool ShouldCreate = true);
3774+
3775+
TempDIArgList cloneImpl() const {
3776+
return getTemporary(getContext(), getArgs());
3777+
}
3778+
37703779
void track();
37713780
void untrack();
3772-
void dropAllReferences(bool Untrack);
3781+
void dropAllReferences();
37733782

37743783
public:
3775-
static DIArgList *get(LLVMContext &Context, ArrayRef<ValueAsMetadata *> Args);
3784+
DEFINE_MDNODE_GET(DIArgList, (ArrayRef<ValueAsMetadata *> Args), (Args))
3785+
3786+
TempDIArgList clone() const { return cloneImpl(); }
37763787

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

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

3782-
static bool classof(const Metadata *MD) {
3783-
return MD->getMetadataID() == DIArgListKind;
3793+
ReplaceableMetadataImpl *getReplaceableUses() {
3794+
return Context.getReplaceableUses();
37843795
}
37853796

3786-
SmallVector<DPValue *> getAllDPValueUsers() {
3787-
return ReplaceableMetadataImpl::getAllDPValueUsers();
3797+
static bool classof(const Metadata *MD) {
3798+
return MD->getMetadataID() == DIArgListKind;
37883799
}
37893800

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

‎llvm/include/llvm/IR/Metadata.def

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ HANDLE_METADATA_BRANCH(ValueAsMetadata)
7777
HANDLE_METADATA_LEAF(ConstantAsMetadata)
7878
HANDLE_METADATA_LEAF(LocalAsMetadata)
7979
HANDLE_METADATA_LEAF(DistinctMDOperandPlaceholder)
80-
HANDLE_METADATA_LEAF(DIArgList)
8180
HANDLE_MDNODE_BRANCH(MDNode)
8281
HANDLE_MDNODE_LEAF_UNIQUABLE(MDTuple)
8382
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DILocation)
@@ -116,6 +115,7 @@ HANDLE_SPECIALIZED_MDNODE_BRANCH(DIMacroNode)
116115
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIMacro)
117116
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIMacroFile)
118117
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DICommonBlock)
118+
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIArgList)
119119
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIStringType)
120120
HANDLE_SPECIALIZED_MDNODE_LEAF_UNIQUABLE(DIGenericSubrange)
121121

‎llvm/include/llvm/IR/Metadata.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1037,6 +1037,7 @@ struct TempMDNodeDeleter {
10371037
class MDNode : public Metadata {
10381038
friend class ReplaceableMetadataImpl;
10391039
friend class LLVMContextImpl;
1040+
friend class DIArgList;
10401041

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

1222-
bool isReplaceable() const { return isTemporary(); }
1223+
bool isReplaceable() const {
1224+
return isTemporary() || getMetadataID() == DIArgListKind;
1225+
}
12231226

12241227
/// RAUW a temporary.
12251228
///

‎llvm/lib/AsmParser/LLParser.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5488,9 +5488,13 @@ bool LLParser::parseDIExpression(MDNode *&Result, bool IsDistinct) {
54885488
return false;
54895489
}
54905490

5491+
bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct) {
5492+
return parseDIArgList(Result, IsDistinct, nullptr);
5493+
}
54915494
/// ParseDIArgList:
54925495
/// ::= !DIArgList(i32 7, i64 %0)
5493-
bool LLParser::parseDIArgList(Metadata *&MD, PerFunctionState *PFS) {
5496+
bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct,
5497+
PerFunctionState *PFS) {
54945498
assert(PFS && "Expected valid function state");
54955499
assert(Lex.getKind() == lltok::MetadataVar && "Expected metadata type name");
54965500
Lex.Lex();
@@ -5510,7 +5514,7 @@ bool LLParser::parseDIArgList(Metadata *&MD, PerFunctionState *PFS) {
55105514
if (parseToken(lltok::rparen, "expected ')' here"))
55115515
return true;
55125516

5513-
MD = DIArgList::get(Context, Args);
5517+
Result = GET_OR_DISTINCT(DIArgList, (Context, Args));
55145518
return false;
55155519
}
55165520

@@ -5624,17 +5628,13 @@ bool LLParser::parseValueAsMetadata(Metadata *&MD, const Twine &TypeMsg,
56245628
/// ::= !DILocation(...)
56255629
bool LLParser::parseMetadata(Metadata *&MD, PerFunctionState *PFS) {
56265630
if (Lex.getKind() == lltok::MetadataVar) {
5631+
MDNode *N;
56275632
// DIArgLists are a special case, as they are a list of ValueAsMetadata and
56285633
// so parsing this requires a Function State.
56295634
if (Lex.getStrVal() == "DIArgList") {
5630-
Metadata *AL;
5631-
if (parseDIArgList(AL, PFS))
5635+
if (parseDIArgList(N, false, PFS))
56325636
return true;
5633-
MD = AL;
5634-
return false;
5635-
}
5636-
MDNode *N;
5637-
if (parseSpecializedMDNode(N)) {
5637+
} else if (parseSpecializedMDNode(N)) {
56385638
return true;
56395639
}
56405640
MD = N;

‎llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,8 @@ class ModuleBitcodeWriter : public ModuleBitcodeWriterBase {
336336
unsigned Abbrev);
337337
void writeDIMacroFile(const DIMacroFile *N, SmallVectorImpl<uint64_t> &Record,
338338
unsigned Abbrev);
339-
void writeDIArgList(const DIArgList *N, SmallVectorImpl<uint64_t> &Record);
339+
void writeDIArgList(const DIArgList *N, SmallVectorImpl<uint64_t> &Record,
340+
unsigned Abbrev);
340341
void writeDIModule(const DIModule *N, SmallVectorImpl<uint64_t> &Record,
341342
unsigned Abbrev);
342343
void writeDIAssignID(const DIAssignID *N, SmallVectorImpl<uint64_t> &Record,
@@ -1974,12 +1975,13 @@ void ModuleBitcodeWriter::writeDIMacroFile(const DIMacroFile *N,
19741975
}
19751976

19761977
void ModuleBitcodeWriter::writeDIArgList(const DIArgList *N,
1977-
SmallVectorImpl<uint64_t> &Record) {
1978+
SmallVectorImpl<uint64_t> &Record,
1979+
unsigned Abbrev) {
19781980
Record.reserve(N->getArgs().size());
19791981
for (ValueAsMetadata *MD : N->getArgs())
19801982
Record.push_back(VE.getMetadataID(MD));
19811983

1982-
Stream.EmitRecord(bitc::METADATA_ARG_LIST, Record);
1984+
Stream.EmitRecord(bitc::METADATA_ARG_LIST, Record, Abbrev);
19831985
Record.clear();
19841986
}
19851987

@@ -2262,10 +2264,6 @@ void ModuleBitcodeWriter::writeMetadataRecords(
22622264
#include "llvm/IR/Metadata.def"
22632265
}
22642266
}
2265-
if (auto *AL = dyn_cast<DIArgList>(MD)) {
2266-
writeDIArgList(AL, Record);
2267-
continue;
2268-
}
22692267
writeValueAsMetadata(cast<ValueAsMetadata>(MD), Record);
22702268
}
22712269
}

‎llvm/lib/IR/AsmWriter.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1266,8 +1266,9 @@ void SlotTracker::CreateFunctionSlot(const Value *V) {
12661266
void SlotTracker::CreateMetadataSlot(const MDNode *N) {
12671267
assert(N && "Can't insert a null Value into SlotTracker!");
12681268

1269-
// Don't make slots for DIExpressions. We just print them inline everywhere.
1270-
if (isa<DIExpression>(N))
1269+
// Don't make slots for DIExpressions or DIArgLists. We just print them inline
1270+
// everywhere.
1271+
if (isa<DIExpression>(N) || isa<DIArgList>(N))
12711272
return;
12721273

12731274
unsigned DestSlot = mdnNext;
@@ -3516,6 +3517,8 @@ void AssemblyWriter::printNamedMDNode(const NamedMDNode *NMD) {
35163517
// Write DIExpressions inline.
35173518
// FIXME: Ban DIExpressions in NamedMDNodes, they will serve no purpose.
35183519
MDNode *Op = NMD->getOperand(i);
3520+
assert(!isa<DIArgList>(Op) &&
3521+
"DIArgLists should not appear in NamedMDNodes");
35193522
if (auto *Expr = dyn_cast<DIExpression>(Op)) {
35203523
writeDIExpression(Out, Expr, AsmWriterContext::getEmpty());
35213524
continue;
@@ -4916,7 +4919,7 @@ static void printMetadataImplRec(raw_ostream &ROS, const Metadata &MD,
49164919
WriteAsOperandInternal(OS, &MD, WriterCtx, /* FromValue */ true);
49174920

49184921
auto *N = dyn_cast<MDNode>(&MD);
4919-
if (!N || isa<DIExpression>(MD))
4922+
if (!N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
49204923
return;
49214924

49224925
OS << " = ";
@@ -4984,7 +4987,7 @@ static void printMetadataImpl(raw_ostream &ROS, const Metadata &MD,
49844987
WriteAsOperandInternal(OS, &MD, *WriterCtx, /* FromValue */ true);
49854988

49864989
auto *N = dyn_cast<MDNode>(&MD);
4987-
if (OnlyAsOperand || !N || isa<DIExpression>(MD))
4990+
if (OnlyAsOperand || !N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
49884991
return;
49894992

49904993
OS << " = ";

‎llvm/lib/IR/DebugInfoMetadata.cpp

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2115,24 +2115,24 @@ DIMacroFile *DIMacroFile::getImpl(LLVMContext &Context, unsigned MIType,
21152115
DEFINE_GETIMPL_STORE(DIMacroFile, (MIType, Line), Ops);
21162116
}
21172117

2118-
DIArgList *DIArgList::get(LLVMContext &Context,
2119-
ArrayRef<ValueAsMetadata *> Args) {
2120-
auto ExistingIt = Context.pImpl->DIArgLists.find_as(DIArgListKeyInfo(Args));
2121-
if (ExistingIt != Context.pImpl->DIArgLists.end())
2122-
return *ExistingIt;
2123-
DIArgList *NewArgList = new DIArgList(Context, Args);
2124-
Context.pImpl->DIArgLists.insert(NewArgList);
2125-
return NewArgList;
2118+
DIArgList *DIArgList::getImpl(LLVMContext &Context,
2119+
ArrayRef<ValueAsMetadata *> Args,
2120+
StorageType Storage, bool ShouldCreate) {
2121+
DEFINE_GETIMPL_LOOKUP(DIArgList, (Args));
2122+
DEFINE_GETIMPL_STORE_NO_OPS(DIArgList, (Args));
21262123
}
21272124

21282125
void DIArgList::handleChangedOperand(void *Ref, Metadata *New) {
21292126
ValueAsMetadata **OldVMPtr = static_cast<ValueAsMetadata **>(Ref);
21302127
assert((!New || isa<ValueAsMetadata>(New)) &&
21312128
"DIArgList must be passed a ValueAsMetadata");
21322129
untrack();
2133-
// We need to update the set storage once the Args are updated since they
2134-
// form the key to the DIArgLists store.
2135-
getContext().pImpl->DIArgLists.erase(this);
2130+
bool Uniq = isUniqued();
2131+
if (Uniq) {
2132+
// We need to update the uniqueness once the Args are updated since they
2133+
// form the key to the DIArgLists store.
2134+
eraseFromStore();
2135+
}
21362136
ValueAsMetadata *NewVM = cast_or_null<ValueAsMetadata>(New);
21372137
for (ValueAsMetadata *&VM : Args) {
21382138
if (&VM == OldVMPtr) {
@@ -2142,19 +2142,28 @@ void DIArgList::handleChangedOperand(void *Ref, Metadata *New) {
21422142
VM = ValueAsMetadata::get(PoisonValue::get(VM->getValue()->getType()));
21432143
}
21442144
}
2145-
// We've changed the contents of this DIArgList, and the set storage may
2146-
// already contain a DIArgList with our new set of args; if it does, then we
2147-
// must RAUW this with the existing DIArgList, otherwise we simply insert this
2148-
// back into the set storage.
2149-
DIArgList *ExistingArgList = getUniqued(getContext().pImpl->DIArgLists, this);
2150-
if (ExistingArgList) {
2151-
replaceAllUsesWith(ExistingArgList);
2152-
// Clear this here so we don't try to untrack in the destructor.
2153-
Args.clear();
2154-
delete this;
2155-
return;
2145+
if (Uniq) {
2146+
// In the RemoveDIs project (eliminating debug-info-intrinsics), DIArgLists
2147+
// can be referred to by DebugValueUser objects, which necessitates them
2148+
// being unique and replaceable metadata. This causes a slight
2149+
// performance regression that's to be avoided during the early stages of
2150+
// the RemoveDIs prototype, see D154080.
2151+
#ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS
2152+
MDNode *UniqueArgList = uniquify();
2153+
if (UniqueArgList != this) {
2154+
replaceAllUsesWith(UniqueArgList);
2155+
// Clear this here so we don't try to untrack in the destructor.
2156+
Args.clear();
2157+
delete this;
2158+
return;
2159+
}
2160+
#else
2161+
// Otherwise, don't fully unique, become distinct instead. See D108968,
2162+
// there's a latent bug that presents here as nondeterminism otherwise.
2163+
if (uniquify() != this)
2164+
storeDistinctInContext();
2165+
#endif
21562166
}
2157-
getContext().pImpl->DIArgLists.insert(this);
21582167
track();
21592168
}
21602169
void DIArgList::track() {
@@ -2167,9 +2176,8 @@ void DIArgList::untrack() {
21672176
if (VAM)
21682177
MetadataTracking::untrack(&VAM, *VAM);
21692178
}
2170-
void DIArgList::dropAllReferences(bool Untrack) {
2171-
if (Untrack)
2172-
untrack();
2179+
void DIArgList::dropAllReferences() {
2180+
untrack();
21732181
Args.clear();
2174-
ReplaceableMetadataImpl::resolveAllUses(/* ResolveUsers */ false);
2182+
MDNode::dropAllReferences();
21752183
}

‎llvm/lib/IR/LLVMContextImpl.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,15 @@ LLVMContextImpl::~LLVMContextImpl() {
6868

6969
// Drop references for MDNodes. Do this before Values get deleted to avoid
7070
// unnecessary RAUW when nodes are still unresolved.
71-
for (auto *I : DistinctMDNodes)
71+
for (auto *I : DistinctMDNodes) {
72+
// We may have DIArgList that were uniqued, and as it has a custom
73+
// implementation of dropAllReferences, it needs to be explicitly invoked.
74+
if (auto *AL = dyn_cast<DIArgList>(I)) {
75+
AL->dropAllReferences();
76+
continue;
77+
}
7278
I->dropAllReferences();
79+
}
7380
#define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS) \
7481
for (auto *I : CLASS##s) \
7582
I->dropAllReferences();
@@ -80,10 +87,6 @@ LLVMContextImpl::~LLVMContextImpl() {
8087
Pair.second->dropUsers();
8188
for (auto &Pair : MetadataAsValues)
8289
Pair.second->dropUse();
83-
// Do not untrack ValueAsMetadata references for DIArgLists, as they have
84-
// already been more efficiently untracked above.
85-
for (DIArgList *AL : DIArgLists)
86-
AL->dropAllReferences(/* Untrack */ false);
8790

8891
// Destroy MDNodes.
8992
for (MDNode *I : DistinctMDNodes)

0 commit comments

Comments
 (0)