[NFC] Return unique dbg intrinsics from findDbgValues and findDbgUsers

[Orlando]

Summary

The out-param vector from findDbgValues and findDbgUsers should not include duplicates, which is possible if the debug intrinsic uses the value multiple times. This filter is already in place for multiple uses in a DIArgLists; extend it to cover dbg.assigns too because a Value may be used in both the address and value components.

Alternatively: maybe the out-parameter should be a SmallSet? Assuming the result is iterated over I imagine the performance cost would overall be very low for the general case. But I don't have any numbers to back that up - I'm not sure what the distribution of sizes of the set are for any codebases.

Without this patch we hit an assertion in a fuchsia build: https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8783373381569070049/overview because replaceVariableLocationOp is called twice for the same intrinsic as a result of the issue mentioned above. replaceVariableLocationOp asserts that an operand is actually changed by the call, and the second time its called this assertion fires as the first call replaced both uses of the value already. I'm also not 100% sure that we need that assertion in there - it seems fairly harmless to try to replace a value that isn't used by an intrinsic.

[jmorse]

LGTM with a request for de-duplication, do fold that in when committing.

I'd prefer to not delete assertions if we can possibly avoid it, on the other hand there might be better ways for us to approach every-API-user-being-correct that's less disruptive. We should switch to that approach if this continues to fire (it's clearly rare enough that it's not going to break everyone's debug experience).

llvm/lib/IR/DebugInfo.cpp

104–105

Now that the body of the loops is identical between DIArgLists and LocalAsMetadatas, could you refactor this into a lambda/delegate to reduce duplication.

[StephenTozer]

Hopefully the size of the set isn't too high with this change since ordinary dbg.values will appear in the set now too; it should be fine, because there have been previous pathological cases of duplicated DIArgList-using dbg.values, and this function didn't cause major performance issues in those cases IIRC.

llvm/lib/IR/DebugInfo.cpp

106

[Orlando]

+ Reduce duplication in the functions with a lambda and between the functions with a template.
+ Fix unsigned/signed comparison warning in unittest.

[jmorse]

LGTM. Even if it does involve templates :(

llvm/lib/IR/DebugInfo.cpp

TinyPtrVector<DbgDeclareInst *> llvm::FindDbgDeclareUses(Value *V) {
   for (User *U : MDV->users()) {
     if (auto *DDI = dyn_cast<DbgDeclareInst>(U))
       Declares.push_back(DDI);
   }
 
   return Declares;
 }
 
-void llvm::findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues, Value *V) {
+template <typename IntrinsicT>
+static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result, Value *V) {
   // This function is hot. Check whether the value has any metadata to avoid a
   // DenseMap lookup.
   if (!V->isUsedByMetadata())
     return;
+
+  LLVMContext &Ctx = V->getContext();
   // TODO: If this value appears multiple times in a DIArgList, we should still
   // only add the owning DbgValueInst once; use this set to track ArgListUsers.
   // This behaviour can be removed when we can automatically remove duplicates.
-  SmallPtrSet<DbgValueInst *, 4> EncounteredDbgValues;
-  if (auto *L = LocalAsMetadata::getIfExists(V)) {
-    if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L)) {
+  // V will also appear twice in a dbg.assign if its used in the both the value
+  // and address components.
+  SmallPtrSet<IntrinsicT *, 4> EncounteredIntrinsics;
+
+  /// Append IntrinsicT users of MetadataAsValue(MD).
+  auto AppendUsers = [&Ctx, &EncounteredIntrinsics, &Result](Metadata *MD) {
+    if (auto *MDV = MetadataAsValue::getIfExists(Ctx, MD)) {
       for (User *U : MDV->users())
-        if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(U))
-          DbgValues.push_back(DVI);
-    }
-    for (Metadata *AL : L->getAllArgListUsers()) {
-      if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), AL)) {
-        for (User *U : MDV->users())
-          if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(U))
-            if (EncounteredDbgValues.insert(DVI).second)
-              DbgValues.push_back(DVI);
-      }
-    }
-  }
+        if (IntrinsicT *DVI = dyn_cast<IntrinsicT>(U))
+          if (EncounteredIntrinsics.insert(DVI).second)
+            Result.push_back(DVI);
+  };
+
+  if (auto *L = LocalAsMetadata::getIfExists(V)) {
+    AppendUsers(L);
+    for (Metadata *AL : L->getAllArgListUsers())
+      AppendUsers(AL);
+
+void llvm::findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues, Value *V) {
+  findDbgIntrinsics<DbgValueInst>(DbgValues, V);
 }
 
 void llvm::findDbgUsers(SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers,
                         Value *V) {
-  // This function is hot. Check whether the value has any metadata to avoid a
-  // DenseMap lookup.
-  if (!V->isUsedByMetadata())
-    return;
-  // TODO: If this value appears multiple times in a DIArgList, we should still
-  // only add the owning DbgValueInst once; use this set to track ArgListUsers.
-  // This behaviour can be removed when we can automatically remove duplicates.
-  SmallPtrSet<DbgVariableIntrinsic *, 4> EncounteredDbgValues;
-  if (auto *L = LocalAsMetadata::getIfExists(V)) {
-    if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), L)) {
-      for (User *U : MDV->users())
-        if (DbgVariableIntrinsic *DII = dyn_cast<DbgVariableIntrinsic>(U))
-          DbgUsers.push_back(DII);
-    }
-    for (Metadata *AL : L->getAllArgListUsers()) {
-      if (auto *MDV = MetadataAsValue::getIfExists(V->getContext(), AL)) {
-        for (User *U : MDV->users())
-          if (DbgVariableIntrinsic *DII = dyn_cast<DbgVariableIntrinsic>(U))
-            if (EncounteredDbgValues.insert(DII).second)
-              DbgUsers.push_back(DII);
-      }
-    }
-  }
+  findDbgIntrinsics<DbgVariableIntrinsic>(DbgUsers, V);
 }
 
 DISubprogram *llvm::getDISubprogram(const MDNode *Scope) {
   if (auto *LocalScope = dyn_cast_or_null<DILocalScope>(Scope))
     return LocalScope->getSubprogram();
   return nullptr;
 }
 
 DebugLoc llvm::getDebugValueLoc(DbgVariableIntrinsic *DII) {

llvm/unittests/Transforms/Utils/LocalTest.cpp

TEST(Local, ChangeToUnreachable) {
 
   // There should be an uncreachable instruction.
   ASSERT_TRUE(isa<UnreachableInst>(&B));
 
   DebugLoc DLB = B.getDebugLoc();
   EXPECT_EQ(DLA, DLB);
 }
 
+TEST(Local, FindDbgUsers) {
+  LLVMContext Ctx;
+  std::unique_ptr<Module> M = parseIR(Ctx,
+                                      R"(
+  define dso_local void @fun(ptr %a) #0 !dbg !11 {
+  entry:
+    call void @llvm.dbg.assign(metadata ptr %a, metadata !16, metadata !DIExpression(), metadata !15, metadata ptr %a, metadata !DIExpression()), !dbg !19
+    ret void
+  }
+
+  declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!2, !3, !9}
+  !llvm.ident = !{!10}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 17.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test.cpp", directory: "/")
+  !2 = !{i32 7, !"Dwarf Version", i32 5}
+  !3 = !{i32 2, !"Debug Info Version", i32 3}
+  !4 = !{i32 1, !"wchar_size", i32 4}
+  !9 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+  !10 = !{!"clang version 17.0.0"}
+  !11 = distinct !DISubprogram(name: "fun", linkageName: "fun", scope: !1, file: !1, line: 1, type: !12, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !14)
+  !12 = !DISubroutineType(types: !13)
+  !13 = !{null}
+  !14 = !{}
+  !15 = distinct !DIAssignID()
+  !16 = !DILocalVariable(name: "x", scope: !11, file: !1, line: 2, type: !17)
+  !17 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !18, size: 64)
+  !18 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !19 = !DILocation(line: 0, scope: !11)
+  )");
+
+  bool BrokenDebugInfo = true;
+  verifyModule(*M, &errs(), &BrokenDebugInfo);
+  ASSERT_FALSE(BrokenDebugInfo);
+
+  Function &Fun = *cast<Function>(M->getNamedValue("fun"));
+  Value *Arg = Fun.getArg(0);
+
+  SmallVector<DbgVariableIntrinsic *> Users;
+  // Arg (%a) is used twice by a single dbg.assign. Check findDbgUsers returns
+  // only 1 pointer to it rather than 2.
+  findDbgUsers(Users, Arg);
+  EXPECT_EQ(Users.size(), 1u);
+
+  SmallVector<DbgValueInst *> Vals;
+  // Arg (%a) is used twice by a single dbg.assign. Check findDbgValues returns
+  // only 1 pointer to it rather than 2.
+  findDbgValues(Vals, Arg);
+  EXPECT_EQ(Vals.size(), 1u);
+}
+
 TEST(Local, ReplaceAllDbgUsesWith) {
   using namespace llvm::dwarf;
 
   LLVMContext Ctx;
 
   // Note: The datalayout simulates Darwin/x86_64.
   std::unique_ptr<Module> M = parseIR(Ctx,
                                       R"(