[DebugInfo][RemoveDIs] Prevent duplicate DPValues from being returned by findDbgIntrinsics

[SLTozer]

(Probably) fixes the error described here: a93a4ec#commitcomment-138965199

The function findDbgIntrinsics is used to return a list of debug intrinsics and DPValues that use a given value, with the intent that no duplicates are returned in either list. For DPValues, we've guarded against DPValues that use a value multiple times as part of a DIArgList, but we have not guarded against DPValues that use a value multiple times as separate operands (currently only possible for dbg_assigns, something I missed in my implementation of that type!). This patch adds a guard, and also updates a test to cover this case.

NB: I think that the llvm.dbg.assign in the test isn't having its address expression updated correctly by CoroFrame, which may be a (completely unrelated) error; something to look at later, but not relevant to this fix.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

(Probably) fixes the error described here: a93a4ec#commitcomment-138965199

The function findDbgIntrinsics is used to return a list of debug intrinsics and DPValues that use a given value, with the intent that no duplicates are returned in either list. For DPValues, we've guarded against DPValues that use a value multiple times as part of a DIArgList, but we have not guarded against DPValues that use a value multiple times as separate operands (currently only possible for dbg_assigns, something I missed in my implementation of that type!). This patch adds a guard, and also updates a test to cover this case.

NB: I think that the llvm.dbg.assign in the test isn't having its address expression updated correctly by CoroFrame, which may be a (completely unrelated) error; something to look at later, but not relevant to this fix.

---

Full diff: https://github.com/llvm/llvm-project/pull/82764.diff

2 Files Affected:

• (modified) llvm/lib/IR/DebugInfo.cpp (+4-3)
• (modified) llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll (+7)

diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp
index a4fec601908503..0b0f17d53a5f33 100644
--- a/llvm/lib/IR/DebugInfo.cpp
+++ b/llvm/lib/IR/DebugInfo.cpp
@@ -99,8 +99,8 @@ static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result, Value *V,
   SmallPtrSet<DPValue *, 4> EncounteredDPValues;
 
   /// Append IntrinsicT users of MetadataAsValue(MD).
-  auto AppendUsers = [&Ctx, &EncounteredIntrinsics, &Result,
-                      DPValues](Metadata *MD) {
+  auto AppendUsers = [&Ctx, &EncounteredIntrinsics, &EncounteredDPValues,
+                      &Result, DPValues](Metadata *MD) {
     if (auto *MDV = MetadataAsValue::getIfExists(Ctx, MD)) {
       for (User *U : MDV->users())
         if (IntrinsicT *DVI = dyn_cast<IntrinsicT>(U))
@@ -113,7 +113,8 @@ static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result, Value *V,
     if (LocalAsMetadata *L = dyn_cast<LocalAsMetadata>(MD)) {
       for (DPValue *DPV : L->getAllDPValueUsers()) {
         if (Type == DPValue::LocationType::Any || DPV->getType() == Type)
-          DPValues->push_back(DPV);
+          if (EncounteredDPValues.insert(DPV).second)
+            DPValues->push_back(DPV);
       }
     }
   };
diff --git a/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll b/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll
index 47b2ddafcfc650..9417859480e58a 100644
--- a/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll
+++ b/llvm/test/Transforms/Coroutines/coro-debug-dbg.values.ll
@@ -9,6 +9,11 @@
 ; CHECK-SAME:    !DIExpression(DW_OP_plus_uconst, [[OffsetX:[0-9]*]]))
 ;                                                                   ^ No deref at the end, as this variable ("x") is an array;
 ;                                                                     its value is its address. The entire array is in the frame.
+; CHECK:       call void @llvm.dbg.assign(metadata ptr %[[frame]]
+; CHECK-SAME:    !DIExpression(DW_OP_plus_uconst, [[OffsetX]])
+;; FIXME: Should we be updating the addresses on assigns here as well?
+; CHECK-SAME:    , metadata ptr %[[frame]], !DIExpression())
+
 ; CHECK:       call void @llvm.dbg.value(metadata ptr %[[frame]]
 ; CHECK-SAME:    !DIExpression(DW_OP_plus_uconst, [[OffsetSpill:[0-9]*]], DW_OP_deref))
 ; CHECK:       call void @llvm.dbg.value(metadata ptr %[[frame]]
@@ -78,6 +83,7 @@ init.ready:                                       ; preds = %init.suspend, %coro
   %i.init.ready.inc = add nsw i32 0, 1
   call void @llvm.dbg.value(metadata i32 %i.init.ready.inc, metadata !6, metadata !DIExpression()), !dbg !11
   call void @llvm.dbg.value(metadata ptr %x, metadata !12, metadata !DIExpression()), !dbg !17
+  call void @llvm.dbg.assign(metadata ptr %x, metadata !12, metadata !DIExpression(), metadata !30, metadata ptr %x, metadata !DIExpression()), !dbg !17
   call void @llvm.memset.p0.i64(ptr align 16 %x, i8 0, i64 40, i1 false), !dbg !17
   call void @print(i32 %i.init.ready.inc)
   %ready.again = call zeroext i1 @await_ready()
@@ -250,3 +256,4 @@ attributes #4 = { argmemonly nofree nosync nounwind willreturn writeonly }
 !21 = !DILocation(line: 43, column: 3, scope: !7)
 !22 = !DILocation(line: 43, column: 8, scope: !7)
 !23 = !DILocalVariable(name: "produced", scope: !7, file: !1, line:24, type: !10)
+!30 = distinct !DIAssignID()
\ No newline at end of file

[jmorse]

findDbgIntrinsics can be a hot path -- would you be able to pump this through the compile-time-tracker please?

I think an alternative adding the AllowEmpty=true flag to the relevant call to replaceVariableLocationOp, if we can be confident there's a legitimate reason for there to be a duplicate entry when replacing.

[SLTozer]

findDbgIntrinsics can be a hot path -- would you be able to pump this through the compile-time-tracker please?

http://llvm-compile-time-tracker.com/compare.php?from=ad49fe3e89c3b3950956548f14cdb5c159ba0aec&to=5fc56961c20dd877798e0158e7424ab7907a645a&stat=instructions:u

tl;dr this is a nothing for performance.

[OCHyams]

LGTM

FIXME: Should we be updating the addresses on assigns here as well?

Agreed (but should be fixed separately)