[DIArgList] Re-unique after changing operands to fix non-determinism

[tejohnson]

Summary

We have a large compile showing occasional non-deterministic behavior
that is due to DIArgList not being properly uniqued in some cases. I
tracked this down to handleChangedOperands, for which there is a custom
implementation for DIArgList, that does not take care of re-uniquing
after updating the DIArgList Args, unlike the default version of
handleChangedOperands for MDNode.

Since the Args in the DIArgList form the key for the store, this seems
to be occasionally breaking the lookup in that DenseSet. Specifically,
when invoking DIArgList::get() from replaceVariableLocationOp, very
occasionally it returns a new DIArgList object, when one already exists
having the same exact Args pointers. This in turn causes a subsequent
call to Instruction::isIdenticalToWhenDefined on those two otherwise
identical DIArgList objects during a later pass to return false, leading
to different IR in those rare cases.

I modified DIArgList::handleChangedOperands to perform similar
re-uniquing as the MDNode version used by other metadata node types.
This also necessitated a change to the context destructor, since in some
cases we end up with DIArgList as distinct nodes: DIArgList is the only
metadata node type to have a custom dropAllReferences, so we need to
invoke that version on DIArgList in the DistinctMDNodes store to clean
it up properly.

[dblaikie]

[just a general: Thanks a bunch for looking into this, I understand tracking down nondeterminism is decidedly not easy]

[MaskRay]

[Thanks! Worth cherry picking this into release/13.x so that distcc/icecc/etc users can benefit ]

llvm/lib/IR/DebugInfoMetadata.cpp

1661

only used once, so perhaps just if (uniquify() != this)

[StephenTozer]

Good catch - thanks for finding and fixing this, LGTM after sorting out the inline comment.

[alexfh]

llvm/lib/IR/DebugInfoMetadata.cpp

1646

nit: Maybe wrap the body in braces, since it's multiline due to the comment?

[tejohnson]

Address comments

llvm/include/llvm/IR/Metadata.h

 /// support permanently.
 ///
 /// If an unresolved node is part of a cycle, \a resolveCycles() needs
 /// to be called on some member of the cycle once all temporary nodes have been
 /// replaced.
 class MDNode : public Metadata {
   friend class ReplaceableMetadataImpl;
   friend class LLVMContextImpl;
+  friend class DIArgList;
 
   unsigned NumOperands;
   unsigned NumUnresolved;
 
   ContextAndReplaceableUses Context;
 
 protected:
   MDNode(LLVMContext &Context, unsigned ID, StorageType Storage,

llvm/lib/IR/DebugInfoMetadata.cpp

DIArgList *DIArgList::getImpl(LLVMContext &Context,
   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();
+  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) {
       if (NewVM)
         VM = NewVM;
       else
         VM = ValueAsMetadata::get(UndefValue::get(VM->getValue()->getType()));
     }
   }
+  if (Uniq) {
+    if (uniquify() != this)
+      storeDistinctInContext();
+  }
   track();
 }
 void DIArgList::track() {
   for (ValueAsMetadata *&VAM : Args)
     if (VAM)
       MetadataTracking::track(&VAM, *VAM, *this);
 }
 void DIArgList::untrack() {

llvm/lib/IR/LLVMContextImpl.cpp

#ifndef NDEBUG
   // Check for metadata references from leaked Values.
   for (auto &Pair : ValueMetadata)
     Pair.first->dump();
   assert(ValueMetadata.empty() && "Values with metadata have been leaked");
 #endif
 
   // 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();
 #include "llvm/IR/Metadata.def"
 
   // Also drop references that come from the Value bridges.
   for (auto &Pair : ValuesAsMetadata)
     Pair.second->dropUsers();