[Assignment Tracking][10/*] salvageDebugInfo for dbg.assign intrinsics

[Orlando]

Summary

Plumb in salvaging for the address part of dbg.assign intrinsics.

[nlopes]

llvm/lib/Transforms/Utils/Local.cpp

1823

Please use PoisonValue for placeholder/dummy values. We are trying to get rid of undef.
Thank you!

[Orlando]

llvm/lib/Transforms/Utils/Local.cpp

1823

Oh right, I wasn't aware of that - do you have a link to a discussion on this?

FWIW, this Undef is wrapped in ValueAsMetadata and is used by a debug intrinsic, if that makes a difference. This is currently standard practice for debug intrinsics to denote that the value of a variable is unknown.

I posted a question on discourse that might be relevant / you may be interested in - here.

[nlopes]

llvm/lib/Transforms/Utils/Local.cpp

1823

Traditionally all placeholders were undef, because there was no poison value.
But now that we have poison, we are trying to move away from undef. Since PoisonValue inherits from UndefValue, all matchers (e.g., isa<>) will work fine. So upgrades can be done incrementally.

I do have a rule in the review system to alert me when someone is trying to add a new use of UndefValue::get, as we want to avoid that.

[jmorse]

LGTM with a question mark over whether we can just start off using poison operands for addresses in dbg.assigns (doing it to the value field makes conformance with the existing dbg.value tests harder). Plus the caveat that to avoid regression, dbg.assign will need to support variadic value operands.

llvm/lib/Transforms/Utils/Local.cpp

1823

@Orlando given that this is a new field that doesn't match existing tests for dbg.value's, will there be much breakage of lit tests if we use Poison here? I think we should prefer to update debug-info use of poison in one fell swoop, but as this is A New Thing (TM) the implications of using poison here should be small, I would imagine.

1853–1854

Doesn't the rest of this function only apply to the Value field, and for DbgAssigns we've already checked whether the Value field contains the instruction? And if it does contain the instruction, this assertion can't fire, surely, so isn't DbgAssignAddrUse redundant?

llvm/test/DebugInfo/Generic/assignment-tracking/salvage-value.ll

2

I'd suggest an implicit check not for other dbg.assigns

[Orlando]

llvm/lib/Transforms/Utils/Local.cpp

1823

I see - it definitely makes sense to move debug intrinsics away from using Undef then.

While I would personally prefer that all debug intrinsics are updated at once, so that we have a uniform representation in IR, @jmorse is right that the problem surface area does shrink if we restrict the update to just this field as it is not shared by existing debug intrinsics.

However, I am still worried about the additional risk of potential complications given that the patch stack is already quite large, and I'm probably going to have my hands full dealing with the fallout for some time as it is.

@nlopes, would you be satisfied if removing this use of Undef came as a future patch and is put on this project's TODO-list (see D132220)?

1853–1854

That looks right. This was probably the result of refactoring things at some point.

[nlopes]

llvm/lib/Transforms/Utils/Local.cpp

1823

@nlopes, would you be satisfied if removing this use of Undef came as a future patch and is put on this project's TODO-list (see D132220)?

Sure, as long as it doesn't go to the part of the todo list that is never done :)
We are serious about removing undef from LLVM, so we would appreciate help in not adding more users of undef long term. Ofc if it's just something temporary, it's perfectly fine.
Thank you!

[Orlando]

+ Address review comments

Plus the caveat that to avoid regression, dbg.assign will need to support variadic value operands.

Is this also something that can be a TODO item, given that the feature is experimental? Similarly to the use of Poison there are no obvious problems that spring to mind, but it would involve further changes and testing (as well as there being the possibility of non-obvious problems lurking).

llvm/lib/Transforms/Utils/Local.cpp

1823

Of course. Thank you @nlopes I appreciate that! Added to the TODO list in the docs in D132220.

[jmorse]

In D133293#3779673, @Orlando wrote:

Is this also something that can be a TODO item, given that the feature is experimental? Similarly to the use of Poison there are no obvious problems that spring to mind, but it would involve further changes and testing (as well as there being the possibility of non-obvious problems lurking).

Sure, LGTM.

llvm/lib/Transforms/Utils/Local.cpp

 /// Where possible to salvage debug information for \p I do so.
 /// If not possible mark undef.
 void llvm::salvageDebugInfo(Instruction &I) {
   SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
   findDbgUsers(DbgUsers, &I);
   salvageDebugInfoForDbgValues(I, DbgUsers);
 }
 
+/// Salvage the address component of \p DAI.
+static void salvageDbgAssignAddress(DbgAssignIntrinsic *DAI) {
+  Instruction *I = dyn_cast<Instruction>(DAI->getAddress());
+  // Only instructions can be salvaged at the moment.
+  if (!I)
+    return;
+
+  assert(!DAI->getAddressExpression()->getFragmentInfo().has_value() &&
+         "address-expression shouldn't have fragment info");
+
+  // The address component of a dbg.assign cannot be variadic.
+  uint64_t CurrentLocOps = 0;
+  SmallVector<Value *, 4> AdditionalValues;
+  SmallVector<uint64_t, 16> Ops;
+  Value *NewV = salvageDebugInfoImpl(*I, CurrentLocOps, Ops, AdditionalValues);
+
+  // Check if the salvage failed.
+  if (!NewV)
+    return;
+
+  DIExpression *SalvagedExpr = DIExpression::appendOpsToArg(
+      DAI->getAddressExpression(), Ops, 0, /*StackValue=*/false);
+  assert(!SalvagedExpr->getFragmentInfo().has_value() &&
+         "address-expression shouldn't have fragment info");
+
+  // Salvage succeeds if no additional values are required.
+  if (AdditionalValues.empty()) {
+    DAI->setAddress(NewV);
+    DAI->setAddressExpression(SalvagedExpr);
+  } else {
+    DAI->setAddress(UndefValue::get(I->getType()));
+  }
+}
+
 void llvm::salvageDebugInfoForDbgValues(
     Instruction &I, ArrayRef<DbgVariableIntrinsic *> DbgUsers) {
   // These are arbitrary chosen limits on the maximum number of values and the
   // maximum size of a debug expression we can salvage up to, used for
   // performance reasons.
   const unsigned MaxDebugArgs = 16;
   const unsigned MaxExpressionSize = 128;
   bool Salvaged = false;
 
   for (auto *DII : DbgUsers) {
+    if (auto *DAI = dyn_cast<DbgAssignIntrinsic>(DII)) {
+      if (DAI->getAddress() == &I) {
+        salvageDbgAssignAddress(DAI);
+        Salvaged = true;
+      }
+      if (DAI->getValue() != &I)
+        continue;
+    }
+
     // Do not add DW_OP_stack_value for DbgDeclare and DbgAddr, because they
     // are implicitly pointing out the value as a DWARF memory location
     // description.
     bool StackValue = isa<DbgValueInst>(DII);
     auto DIILocation = DII->location_ops();
     assert(
         is_contained(DIILocation, &I) &&
         "DbgVariableIntrinsic must use salvaged instruction as its location");
     SmallVector<Value *, 4> AdditionalValues;
     // `I` may appear more than once in DII's location ops, and each use of `I`
     // must be updated in the DIExpression and potentially have additional
     // values added; thus we call salvageDebugInfoImpl for each `I` instance in
     // DIILocation.
     Value *Op0 = nullptr;
     DIExpression *SalvagedExpr = DII->getExpression();
     auto LocItr = find(DIILocation, &I);
     while (SalvagedExpr && LocItr != DIILocation.end()) {
for (auto *DII : DbgUsers) {
     // DbgUsers, or none of them.
     if (!Op0)
       break;
 
     DII->replaceVariableLocationOp(&I, Op0);
     bool IsValidSalvageExpr = SalvagedExpr->getNumElements() <= MaxExpressionSize;
     if (AdditionalValues.empty() && IsValidSalvageExpr) {
       DII->setExpression(SalvagedExpr);
-    } else if (isa<DbgValueInst>(DII) && IsValidSalvageExpr &&
+    } else if (isa<DbgValueInst>(DII) && !isa<DbgAssignIntrinsic>(DII) &&
+               IsValidSalvageExpr &&
                DII->getNumVariableLocationOps() + AdditionalValues.size() <=
                    MaxDebugArgs) {
       DII->addVariableLocationOps(AdditionalValues, SalvagedExpr);
     } else {
       // Do not salvage using DIArgList for dbg.addr/dbg.declare, as it is
-      // currently only valid for stack value expressions.
+      // currently only valid for stack value expressions. Do not salvage
+      // using DIArgList for dbg.assign yet. FIXME: support this.
       // Also do not salvage if the resulting DIArgList would contain an
       // unreasonably large number of values.
       Value *Undef = UndefValue::get(I.getOperand(0)->getType());
       DII->replaceVariableLocationOp(I.getOperand(0), Undef);
     }
     LLVM_DEBUG(dbgs() << "SALVAGE: " << *DII << '\n');
     Salvaged = true;
   }

llvm/test/DebugInfo/Generic/assignment-tracking/salvage-value.ll

+; RUN: opt %s -S -o - -passes=instcombine -experimental-assignment-tracking \
+; RUN: | FileCheck %s --implicit-check-not="call void @llvm.dbg"
+
+;; Hand-written (the debug info doesn't necessarily make sense and isn't fully
+;; formed). Test salvaging a dbg.assign value and address. Checks and comments
+;; are inline.
+
+define dso_local void @fun(i32  %x, i32 %y, ptr %p) !dbg !7 {
+entry:
+  %add = add nsw i32 %x, 1, !dbg !22
+  call void @llvm.dbg.assign(metadata i32 %add, metadata !14, metadata !DIExpression(), metadata !28, metadata ptr %p, metadata !DIExpression()), !dbg !16
+;; %add is salvaged.
+; CHECK: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value),{{.+}}, metadata ptr %p, metadata !DIExpression())
+
+ %add1 = add nsw i32 %x, %y, !dbg !29
+ call void @llvm.dbg.assign(metadata i32 %add1, metadata !14, metadata !DIExpression(), metadata !31, metadata ptr %p, metadata !DIExpression()), !dbg !16
+;; %add1 is not salvaged as it requires two values and DIArgList is
+;; not (yet) supported for dbg.assigns.
+; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 undef,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr %p, metadata !DIExpression())
+
+  %arrayidx0 = getelementptr inbounds i32, ptr %p, i32 0
+  call void @llvm.dbg.assign(metadata i32 %x, metadata !14, metadata !DIExpression(), metadata !17, metadata ptr %arrayidx0, metadata !DIExpression()), !dbg !16
+;; %arrayidx0 is salvaged (zero offset, so the gep is just replaced with %p).
+; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr %p, metadata !DIExpression())
+
+  %arrayidx1 = getelementptr inbounds i32, ptr %p, i32 1
+  call void @llvm.dbg.assign(metadata i32 %x, metadata !14, metadata !DIExpression(), metadata !18, metadata ptr %arrayidx1, metadata !DIExpression()), !dbg !16
+;; %arrayidx1 is salvaged.
+; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr %p, metadata !DIExpression(DW_OP_plus_uconst, 4))
+
+  %arrayidx2 = getelementptr inbounds i32, ptr %p, i32 %x
+  call void @llvm.dbg.assign(metadata i32 %x, metadata !14, metadata !DIExpression(), metadata !19, metadata ptr %arrayidx2, metadata !DIExpression()), !dbg !16
+;; Variadic DIExpressions for dbg.assign address component is not supported -
+;; set undef.
+; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr undef, metadata !DIExpression())
+
+  ret void
+}
+
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0", isOptimized: true, 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}
+!5 = !{i32 7, !"uwtable", i32 1}
+!6 = !{!"clang version 14.0.0"}
+!7 = distinct !DISubprogram(name: "fun", linkageName: "fun", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !10, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12, !13, !14}
+!12 = !DILocalVariable(name: "x", arg: 1, scope: !7, file: !1, line: 2, type: !10)
+!13 = !DILocalVariable(name: "y", arg: 2, scope: !7, file: !1, line: 2, type: !10)
+!14 = !DILocalVariable(name: "Local", scope: !7, file: !1, line: 3, type: !10)
+!16 = !DILocation(line: 0, scope: !7)
+!17 = distinct !DIAssignID()
+!18 = distinct !DIAssignID()
+!19 = distinct !DIAssignID()
+!21 = !DILocation(line: 3, column: 3, scope: !7)
+!22 = !DILocation(line: 3, column: 17, scope: !7)
+!28 = distinct !DIAssignID()
+!29 = !DILocation(line: 4, column: 13, scope: !7)
+!31 = distinct !DIAssignID()