Revert "[DebugInfo] Make DIArgList inherit from Metadata and always unique"

llvm/include/llvm/AsmParser/LLParser.h

@@ -548,7 +548,6 @@ 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);
@@ -570,6 +569,8 @@ 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 {

llvm/include/llvm/IR/DebugInfoMetadata.h

@@ -3753,38 +3753,49 @@ class DIMacroFile : public DIMacroNode {
 
 /// List of ValueAsMetadata, to be used as an argument to a dbg.value
 /// intrinsic.
-class DIArgList : public Metadata, ReplaceableMetadataImpl {
-  friend class ReplaceableMetadataImpl;
+class DIArgList : public MDNode {
   friend class LLVMContextImpl;
+  friend class MDNode;
   using iterator = SmallVectorImpl<ValueAsMetadata *>::iterator;
 
   SmallVector<ValueAsMetadata *, 4> Args;
 
-  DIArgList(LLVMContext &Context, ArrayRef<ValueAsMetadata *> Args)
-      : Metadata(DIArgListKind, Uniqued), ReplaceableMetadataImpl(Context),
+  DIArgList(LLVMContext &C, StorageType Storage,
+            ArrayRef<ValueAsMetadata *> Args)
+      : MDNode(C, DIArgListKind, Storage, std::nullopt),
         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(bool Untrack);
+  void dropAllReferences();
 
 public:
-  static DIArgList *get(LLVMContext &Context, ArrayRef<ValueAsMetadata *> Args);
+  DEFINE_MDNODE_GET(DIArgList, (ArrayRef<ValueAsMetadata *> Args), (Args))
+
+  TempDIArgList clone() const { return cloneImpl(); }
 
   ArrayRef<ValueAsMetadata *> getArgs() const { return Args; }
 
   iterator args_begin() { return Args.begin(); }
   iterator args_end() { return Args.end(); }
 
-  static bool classof(const Metadata *MD) {
-    return MD->getMetadataID() == DIArgListKind;
+  ReplaceableMetadataImpl *getReplaceableUses() {
+    return Context.getReplaceableUses();
   }
 
-  SmallVector<DPValue *> getAllDPValueUsers() {
-    return ReplaceableMetadataImpl::getAllDPValueUsers();
+  static bool classof(const Metadata *MD) {
+    return MD->getMetadataID() == DIArgListKind;
   }
 
   void handleChangedOperand(void *Ref, Metadata *New);

llvm/include/llvm/IR/Metadata.def

@@ -77,7 +77,6 @@ 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)
@@ -116,6 +115,7 @@ 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)
 

llvm/include/llvm/IR/Metadata.h

@@ -1037,6 +1037,7 @@ 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.
@@ -1219,7 +1220,9 @@ class MDNode : public Metadata {
   bool isDistinct() const { return Storage == Distinct; }
   bool isTemporary() const { return Storage == Temporary; }
 
-  bool isReplaceable() const { return isTemporary(); }
+  bool isReplaceable() const {
+    return isTemporary() || getMetadataID() == DIArgListKind;
+  }
 
   /// RAUW a temporary.
   ///

llvm/lib/AsmParser/LLParser.cpp

@@ -5488,9 +5488,13 @@ 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(Metadata *&MD, PerFunctionState *PFS) {
+bool LLParser::parseDIArgList(MDNode *&Result, bool IsDistinct,
+                              PerFunctionState *PFS) {
   assert(PFS && "Expected valid function state");
   assert(Lex.getKind() == lltok::MetadataVar && "Expected metadata type name");
   Lex.Lex();
@@ -5510,7 +5514,7 @@ bool LLParser::parseDIArgList(Metadata *&MD, PerFunctionState *PFS) {
   if (parseToken(lltok::rparen, "expected ')' here"))
     return true;
 
-  MD = DIArgList::get(Context, Args);
+  Result = GET_OR_DISTINCT(DIArgList, (Context, Args));
   return false;
 }
 
@@ -5624,17 +5628,13 @@ 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") {
-      Metadata *AL;
-      if (parseDIArgList(AL, PFS))
+      if (parseDIArgList(N, false, PFS))
         return true;
-      MD = AL;
-      return false;
-    }
-    MDNode *N;
-    if (parseSpecializedMDNode(N)) {
+    } else if (parseSpecializedMDNode(N)) {
       return true;
     }
     MD = N;

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

@@ -336,7 +336,8 @@ 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);
+  void writeDIArgList(const DIArgList *N, SmallVectorImpl<uint64_t> &Record,
+                      unsigned Abbrev);
   void writeDIModule(const DIModule *N, SmallVectorImpl<uint64_t> &Record,
                      unsigned Abbrev);
   void writeDIAssignID(const DIAssignID *N, SmallVectorImpl<uint64_t> &Record,
@@ -1974,12 +1975,13 @@ void ModuleBitcodeWriter::writeDIMacroFile(const DIMacroFile *N,
 }
 
 void ModuleBitcodeWriter::writeDIArgList(const DIArgList *N,
-                                         SmallVectorImpl<uint64_t> &Record) {
+                                         SmallVectorImpl<uint64_t> &Record,
+                                         unsigned Abbrev) {
   Record.reserve(N->getArgs().size());
   for (ValueAsMetadata *MD : N->getArgs())
     Record.push_back(VE.getMetadataID(MD));
 
-  Stream.EmitRecord(bitc::METADATA_ARG_LIST, Record);
+  Stream.EmitRecord(bitc::METADATA_ARG_LIST, Record, Abbrev);
   Record.clear();
 }
 
@@ -2262,10 +2264,6 @@ void ModuleBitcodeWriter::writeMetadataRecords(
 #include "llvm/IR/Metadata.def"
       }
     }
-    if (auto *AL = dyn_cast<DIArgList>(MD)) {
-      writeDIArgList(AL, Record);
-      continue;
-    }
     writeValueAsMetadata(cast<ValueAsMetadata>(MD), Record);
   }
 }

llvm/lib/IR/AsmWriter.cpp

@@ -1266,8 +1266,9 @@ 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. We just print them inline everywhere.
-  if (isa<DIExpression>(N))
+  // Don't make slots for DIExpressions or DIArgLists. We just print them inline
+  // everywhere.
+  if (isa<DIExpression>(N) || isa<DIArgList>(N))
     return;
 
   unsigned DestSlot = mdnNext;
@@ -3516,6 +3517,8 @@ 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;
@@ -4916,7 +4919,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))
+  if (!N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
     return;
 
   OS << " = ";
@@ -4984,7 +4987,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))
+  if (OnlyAsOperand || !N || isa<DIExpression>(MD) || isa<DIArgList>(MD))
     return;
 
   OS << " = ";

llvm/lib/IR/DebugInfoMetadata.cpp

@@ -2115,24 +2115,24 @@ DIMacroFile *DIMacroFile::getImpl(LLVMContext &Context, unsigned MIType,
   DEFINE_GETIMPL_STORE(DIMacroFile, (MIType, Line), Ops);
 }
 
-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;
+DIArgList *DIArgList::getImpl(LLVMContext &Context,
+                              ArrayRef<ValueAsMetadata *> Args,
+                              StorageType Storage, bool ShouldCreate) {
+  DEFINE_GETIMPL_LOOKUP(DIArgList, (Args));
+  DEFINE_GETIMPL_STORE_NO_OPS(DIArgList, (Args));
 }
 
 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();
-  // 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);
+  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();
+  }
   ValueAsMetadata *NewVM = cast_or_null<ValueAsMetadata>(New);
   for (ValueAsMetadata *&VM : Args) {
     if (&VM == OldVMPtr) {
@@ -2142,19 +2142,28 @@ void DIArgList::handleChangedOperand(void *Ref, Metadata *New) {
         VM = ValueAsMetadata::get(PoisonValue::get(VM->getValue()->getType()));
     }
   }
-  // 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;
+  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
   }
-  getContext().pImpl->DIArgLists.insert(this);
   track();
 }
 void DIArgList::track() {
@@ -2167,9 +2176,8 @@ void DIArgList::untrack() {
     if (VAM)
       MetadataTracking::untrack(&VAM, *VAM);
 }
-void DIArgList::dropAllReferences(bool Untrack) {
-  if (Untrack)
-    untrack();
+void DIArgList::dropAllReferences() {
+  untrack();
   Args.clear();
-  ReplaceableMetadataImpl::resolveAllUses(/* ResolveUsers */ false);
+  MDNode::dropAllReferences();
 }

llvm/lib/IR/LLVMContextImpl.cpp

@@ -68,8 +68,15 @@ 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)
+  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;
+    }
     I->dropAllReferences();
+  }
 #define HANDLE_MDNODE_LEAF_UNIQUABLE(CLASS)                                    \
   for (auto *I : CLASS##s)                                                     \
     I->dropAllReferences();
@@ -80,10 +87,6 @@ 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)