[DebugInfo] Handle additional types of stores in assignment tracking

[SLTozer]

Fixes #126417.

Currently, assignment tracking recognizes allocas, stores, and mem intrinsics as valid instructions to tag with DIAssignID, with allocas representing the allocation for a variable and the others representing instructions that may assign to the variable. There are other intrinsics that can perform these assignments however, and if we transform a store instruction into one of these intrinsics and correctly transfer the DIAssignID over, this results in a verifier error. The AssignmentTrackingAnalysis pass also does not know how to handle these intrinsics if they are untagged, as it does not know how to extract assignment information (base address, offset, size) from them.

This patch adds some support for some intrinsics that may perform assignments: masked store/scatter, and vp store/strided store/scatter. This patch does not add support for extracting assignment information from these, as they may store with either non-constant size or to non-contiguous blocks of memory; instead it adds support for recognizing untagged stores with "unknown" assignment info, for which we assume that the memory location of the associated variable should not be used, as we can't determine which fragments of it should or should not be used.

In principle, it should be possible to handle the more complex cases mentioned above, but it would require more substantial changes to AssignmentTrackingAnalysis, and it is mostly only needed as a fallback if the DIAssignID is not preserved on these alternative stores.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

Fixes #126417.

Currently, assignment tracking recognizes allocas, stores, and mem intrinsics as valid instructions to tag with DIAssignID, with allocas representing the allocation for a variable and the others representing instructions that may assign to the variable. There are other intrinsics that can perform these assignments however, and if we transform a store instruction into one of these intrinsics and correctly transfer the DIAssignID over, this results in a verifier error. The AssignmentTrackingAnalysis pass also does not know how to handle these intrinsics if they are untagged, as it does not know how to extract assignment information (base address, offset, size) from them.

This patch adds some support for some intrinsics that may perform assignments: masked store/scatter, and vp store/strided store/scatter. This patch does not add support for extracting assignment information from these, as they may store with either non-constant size or to non-contiguous blocks of memory; instead it adds support for recognizing untagged stores with "unknown" assignment info, for which we assume that the memory location of the associated variable should not be used, as we can't determine which fragments of it should or should not be used.

In principle, it should be possible to handle the more complex cases mentioned above, but it would require more substantial changes to AssignmentTrackingAnalysis, and it is mostly only needed as a fallback if the DIAssignID is not preserved on these alternative stores.

---

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

4 Files Affected:

• (modified) llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp (+105-4)
• (modified) llvm/lib/IR/Verifier.cpp (+13-2)
• (added) llvm/test/CodeGen/RISCV/di-assignment-tracking-vector.ll (+65)
• (added) llvm/test/Verifier/diassignid-vector-stores.ll (+33)

diff --git a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
index dbc724629d3be..f74a014a0bafd 100644
--- a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
+++ b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
@@ -1103,6 +1103,8 @@ class AssignmentTrackingLowering {
   using UntaggedStoreAssignmentMap =
       DenseMap<const Instruction *,
                SmallVector<std::pair<VariableID, at::AssignmentInfo>>>;
+  using UnknownStoreAssignmentMap =
+      DenseMap<const Instruction *, SmallVector<VariableID>>;
 
 private:
   /// The highest numbered VariableID for partially promoted variables plus 1,
@@ -1113,6 +1115,9 @@ class AssignmentTrackingLowering {
   /// Map untagged stores to the variable fragments they assign to. Used by
   /// processUntaggedInstruction.
   UntaggedStoreAssignmentMap UntaggedStoreVars;
+  /// Map untagged unknown stores (e.g. strided/masked store intrinsics)
+  /// to the variables they may assign to. Used by processUntaggedInstruction.
+  UnknownStoreAssignmentMap UnknownStoreVars;
 
   // Machinery to defer inserting dbg.values.
   using InstInsertMap = MapVector<VarLocInsertPt, SmallVector<VarLocInfo>>;
@@ -1355,6 +1360,8 @@ class AssignmentTrackingLowering {
   /// Update \p LiveSet after encountering an instruciton without a DIAssignID
   /// attachment, \p I.
   void processUntaggedInstruction(Instruction &I, BlockInfo *LiveSet);
+  void processUnknownStoreToVariable(Instruction &I, VariableID &Var,
+                                     BlockInfo *LiveSet);
   void processDbgAssign(AssignRecord Assign, BlockInfo *LiveSet);
   void processDbgVariableRecord(DbgVariableRecord &DVR, BlockInfo *LiveSet);
   void processDbgValue(
@@ -1604,6 +1611,45 @@ void AssignmentTrackingLowering::processNonDbgInstruction(
     processUntaggedInstruction(I, LiveSet);
 }
 
+void AssignmentTrackingLowering::processUnknownStoreToVariable(
+    Instruction &I, VariableID &Var, BlockInfo *LiveSet) {
+  // We may have assigned to some unknown fragment of the variable, so
+  // treat the memory assignment as unknown for now.
+  addMemDef(LiveSet, Var, Assignment::makeNoneOrPhi());
+  // If we weren't already using a memory location, we don't need to do
+  // anything more.
+  if (getLocKind(LiveSet, Var) != LocKind::Mem)
+    return;
+  // If there is a live debug value for this variable, fall back to using
+  // that
+  Assignment DbgAV = LiveSet->getAssignment(BlockInfo::Debug, Var);
+  if (DbgAV.Status != Assignment::NoneOrPhi && DbgAV.Source) {
+    LLVM_DEBUG(dbgs() << "Switching to fallback debug value: ";
+               DbgAV.dump(dbgs()); dbgs() << "\n");
+    setLocKind(LiveSet, Var, LocKind::Val);
+    emitDbgValue(LocKind::Val, DbgAV.Source, &I);
+    return;
+  }
+  // Otherwise, find a suitable insert point, before the next instruction or
+  // DbgRecord after I.
+  auto InsertBefore = getNextNode(&I);
+  assert(InsertBefore && "Shouldn't be inserting after a terminator");
+
+  // Get DILocation for this unrecorded assignment.
+  DebugVariable V = FnVarLocs->getVariable(Var);
+  DILocation *InlinedAt = const_cast<DILocation *>(V.getInlinedAt());
+  const DILocation *DILoc = DILocation::get(
+      Fn.getContext(), 0, 0, V.getVariable()->getScope(), InlinedAt);
+
+  VarLocInfo VarLoc;
+  VarLoc.VariableID = static_cast<VariableID>(Var);
+  VarLoc.Expr = DIExpression::get(I.getContext(), {});
+  VarLoc.Values = RawLocationWrapper(
+      ValueAsMetadata::get(PoisonValue::get(Type::getInt1Ty(I.getContext()))));
+  VarLoc.DL = DILoc;
+  InsertBeforeMap[InsertBefore].push_back(VarLoc);
+}
+
 void AssignmentTrackingLowering::processUntaggedInstruction(
     Instruction &I, AssignmentTrackingLowering::BlockInfo *LiveSet) {
   // Interpret stack stores that are not tagged as an assignment in memory for
@@ -1619,8 +1665,22 @@ void AssignmentTrackingLowering::processUntaggedInstruction(
   // "early", for example.
   assert(!I.hasMetadata(LLVMContext::MD_DIAssignID));
   auto It = UntaggedStoreVars.find(&I);
-  if (It == UntaggedStoreVars.end())
+  if (It == UntaggedStoreVars.end()) {
+    // It is possible that we have an untagged unknown store, which we do
+    // not currently support - in this case we should undef the stack location
+    // of the variable, as if we had a tagged store that did not match the
+    // current assignment.
+    // FIXME: It should be possible to support unknown stores, but it
+    // would require more extensive changes to our representation of assignments
+    // which assumes a single offset+size.
+    if (auto UnhandledStoreIt = UnknownStoreVars.find(&I);
+        UnhandledStoreIt != UnknownStoreVars.end()) {
+      LLVM_DEBUG(dbgs() << "Processing untagged unknown store " << I << "\n");
+      for (auto &Var : UnhandledStoreIt->second)
+        processUnknownStoreToVariable(I, Var, LiveSet);
+    }
     return; // No variables associated with the store destination.
+  }
 
   LLVM_DEBUG(dbgs() << "processUntaggedInstruction on UNTAGGED INST " << I
                     << "\n");
@@ -2123,6 +2183,25 @@ getUntaggedStoreAssignmentInfo(const Instruction &I, const DataLayout &Layout) {
   return std::nullopt;
 }
 
+AllocaInst *getUnknownStore(const Instruction &I, const DataLayout &Layout) {
+  auto *II = dyn_cast<IntrinsicInst>(&I);
+  if (!II)
+    return nullptr;
+  Intrinsic::ID ID = II->getIntrinsicID();
+  if (ID != Intrinsic::experimental_vp_strided_store &&
+      ID != Intrinsic::masked_store && ID != Intrinsic::vp_scatter &&
+      ID != Intrinsic::masked_scatter)
+    return nullptr;
+  Value *MemOp = II->getArgOperand(1);
+  // We don't actually use the constant offsets for now, but we may in future,
+  // and the non-accumulating versions do not support a vector of pointers.
+  APInt Offset(Layout.getIndexTypeSizeInBits(MemOp->getType()), 0);
+  Value *Base = MemOp->stripAndAccumulateConstantOffsets(Layout, Offset, true);
+  // For Base pointers that are not a single alloca value we don't need to do
+  // anything, and simply return nullptr.
+  return dyn_cast<AllocaInst>(Base);
+}
+
 DbgDeclareInst *DynCastToDbgDeclare(DbgVariableIntrinsic *DVI) {
   return dyn_cast<DbgDeclareInst>(DVI);
 }
@@ -2145,7 +2224,8 @@ DbgVariableRecord *DynCastToDbgDeclare(DbgVariableRecord *DVR) {
 /// subsequent variables are either stack homed or fully promoted.
 ///
 /// Finally, populate UntaggedStoreVars with a mapping of untagged stores to
-/// the stored-to variable fragments.
+/// the stored-to variable fragments, and UnknownStoreVars with a mapping
+/// of untagged unknown stores to the stored-to variable aggregates.
 ///
 /// These tasks are bundled together to reduce the number of times we need
 /// to iterate over the function as they can be achieved together in one pass.
@@ -2153,6 +2233,7 @@ static AssignmentTrackingLowering::OverlapMap buildOverlapMapAndRecordDeclares(
     Function &Fn, FunctionVarLocsBuilder *FnVarLocs,
     const DenseSet<DebugAggregate> &VarsWithStackSlot,
     AssignmentTrackingLowering::UntaggedStoreAssignmentMap &UntaggedStoreVars,
+    AssignmentTrackingLowering::UnknownStoreAssignmentMap &UnknownStoreVars,
     unsigned &TrackedVariablesVectorSize) {
   DenseSet<DebugVariable> Seen;
   // Map of Variable: [Fragments].
@@ -2161,7 +2242,8 @@ static AssignmentTrackingLowering::OverlapMap buildOverlapMapAndRecordDeclares(
   // - dbg.declare    -> add single location variable record
   // - dbg.*          -> Add fragments to FragmentMap
   // - untagged store -> Add fragments to FragmentMap and update
-  //                     UntaggedStoreVars.
+  //                     UntaggedStoreVars, or add to UnknownStoreVars if
+  //                     we can't determine the fragment overlap.
   // We need to add fragments for untagged stores too so that we can correctly
   // clobber overlapped fragment locations later.
   SmallVector<DbgDeclareInst *> InstDeclares;
@@ -2224,6 +2306,25 @@ static AssignmentTrackingLowering::OverlapMap buildOverlapMapAndRecordDeclares(
           HandleDbgAssignForStore(DAI);
         for (DbgVariableRecord *DVR : at::getDVRAssignmentMarkers(Info->Base))
           HandleDbgAssignForStore(DVR);
+      } else if (auto *AI = getUnknownStore(I, Fn.getDataLayout())) {
+        // Find markers linked to this alloca.
+        auto HandleDbgAssignForUnknownStore = [&](auto *Assign) {
+          // Because we can't currently represent the fragment info for this
+          // store, we treat it as an unusable store to the whole variable.
+          DebugVariable DV =
+              DebugVariable(Assign->getVariable(), std::nullopt,
+                            Assign->getDebugLoc().getInlinedAt());
+          DebugAggregate DA = {DV.getVariable(), DV.getInlinedAt()};
+          if (!VarsWithStackSlot.contains(DA))
+            return;
+
+          // Cache this info for later.
+          UnknownStoreVars[&I].push_back(FnVarLocs->insertVariable(DV));
+        };
+        for (DbgAssignIntrinsic *DAI : at::getAssignmentMarkers(AI))
+          HandleDbgAssignForUnknownStore(DAI);
+        for (DbgVariableRecord *DVR : at::getDVRAssignmentMarkers(AI))
+          HandleDbgAssignForUnknownStore(DVR);
       }
     }
   }
@@ -2298,7 +2399,7 @@ bool AssignmentTrackingLowering::run(FunctionVarLocsBuilder *FnVarLocsBuilder) {
   // neither does LiveDebugVariables) because that is difficult to do and
   // appears to be rare occurance.
   VarContains = buildOverlapMapAndRecordDeclares(
-      Fn, FnVarLocs, *VarsWithStackSlot, UntaggedStoreVars,
+      Fn, FnVarLocs, *VarsWithStackSlot, UntaggedStoreVars, UnknownStoreVars,
       TrackedVariablesVectorSize);
 
   // Prepare for traversal.
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index b4f9273fea9fb..fa76ee90b83dd 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4956,8 +4956,19 @@ void Verifier::visitProfMetadata(Instruction &I, MDNode *MD) {
 
 void Verifier::visitDIAssignIDMetadata(Instruction &I, MDNode *MD) {
   assert(I.hasMetadata(LLVMContext::MD_DIAssignID));
-  bool ExpectedInstTy =
-      isa<AllocaInst>(I) || isa<StoreInst>(I) || isa<MemIntrinsic>(I);
+  // DIAssignID metadata must be attached to either an alloca or some form of
+  // store/memory-writing instruction.
+  // FIXME: Is there any simpler way to express this property than manually
+  // enumerating all instructions that could perform an assignment?
+  bool ExpectedInstTy = isa<AllocaInst>(I) || isa<StoreInst>(I);
+  if (auto *II = dyn_cast<IntrinsicInst>(&I)) {
+    const static Intrinsic::ID StoreIntrinsics[] = {
+        Intrinsic::vp_store, Intrinsic::vp_scatter,
+        Intrinsic::experimental_vp_strided_store, Intrinsic::masked_store,
+        Intrinsic::masked_scatter};
+    ExpectedInstTy |= is_contained(StoreIntrinsics, II->getIntrinsicID()) ||
+                      isa<MemIntrinsic>(II);
+  }
   CheckDI(ExpectedInstTy, "!DIAssignID attached to unexpected instruction kind",
           I, MD);
   // Iterate over the MetadataAsValue uses of the DIAssignID - these should
diff --git a/llvm/test/CodeGen/RISCV/di-assignment-tracking-vector.ll b/llvm/test/CodeGen/RISCV/di-assignment-tracking-vector.ll
new file mode 100644
index 0000000000000..0be9fcd830fd9
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/di-assignment-tracking-vector.ll
@@ -0,0 +1,65 @@
+; RUN: llc -mtriple=riscv64 < %s -o - | FileCheck %s --implicit-check-not=DEBUG_VALUE
+
+;; Verify that tagged and untagged non-contiguous stores are handled correctly
+;; by assignment tracking.
+;; * The store to "i" is untagged, and results in the memory location being
+;;   dropped in favour of the debug value 1010 after the store.
+;; * The store to "j" is tagged with a corresponding dbg_assign, which allows
+;;   us to keep using the memory location.
+
+; CHECK-LABEL: foo:
+; CHECK-NEXT:  .Lfunc_begin0:
+; CHECK:       # %bb.0
+; CHECK:         addi    a1, sp, 48
+; CHECK-NEXT:    #DEBUG_VALUE: foo:i <- [DW_OP_deref] $x12
+; CHECK-NEXT:    #DEBUG_VALUE: foo:j <- [DW_OP_deref] $x12
+; CHECK:         vsse32.v
+; CHECK-NEXT:    #DEBUG_VALUE: foo:i <- 1010
+; CHECK-NEXT:    vsse32.v
+
+
+target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128"
+target triple = "riscv64-unknown-linux-gnu"
+
+define void @foo() #0 !dbg !5 {
+entry:
+  %i = alloca i64, align 8, !DIAssignID !6
+  %j = alloca i64, align 8, !DIAssignID !12
+  %sar_height.i = getelementptr i8, ptr %i, i64 24
+  store ptr %sar_height.i, ptr null, align 8
+  %vui.i = getelementptr i8, ptr %i, i64 44
+  %0 = load i32, ptr %vui.i, align 4
+  %sar_width.i = getelementptr i8, ptr %i, i64 20
+  %i_sar_width.i = getelementptr i8, ptr %i, i64 48
+  %j_sar_width.j = getelementptr i8, ptr %j, i64 48
+    #dbg_assign(i32 1010, !7, !DIExpression(), !6, ptr %i_sar_width.i, !DIExpression(), !9)
+    #dbg_assign(i32 2121, !17, !DIExpression(), !12, ptr %i_sar_width.i, !DIExpression(), !9)
+  %1 = load <2 x i32>, ptr %sar_width.i, align 4
+  call void @llvm.experimental.vp.strided.store.v2i32.p0.i64(<2 x i32> %1, ptr align 4 %i_sar_width.i, i64 -4, <2 x i1> splat (i1 true), i32 2)
+  call void @llvm.experimental.vp.strided.store.v2i32.p0.i64(<2 x i32> %1, ptr align 4 %j_sar_width.j, i64 -4, <2 x i1> splat (i1 true), i32 2), !DIAssignID !13
+    #dbg_assign(i32 1010, !7, !DIExpression(), !14, ptr %i_sar_width.i, !DIExpression(), !9)
+    #dbg_assign(i32 2121, !17, !DIExpression(), !13, ptr %i_sar_width.i, !DIExpression(), !9)
+  ret void
+}
+
+attributes #0 = { "target-features"="+64bit,+a,+c,+d,+f,+m,+relax,+v,+zaamo,+zalrsc,+zicsr,+zifencei,+zmmul,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-b,-e,-experimental-sdext,-experimental-sdtrig,-experimental-smctr,-experimental-ssctr,-experimental-svukte,-experimental-xqcia,-experimental-xqciac,-experimental-xqcicli,-experimental-xqcicm,-experimental-xqcics,-experimental-xqcicsr,-experimental-xqciint,-experimental-xqcilo,-experimental-xqcilsm,-experimental-xqcisls,-experimental-zalasr,-experimental-zicfilp,-experimental-zicfiss,-experimental-zvbc32e,-experimental-zvkgs,-h,-sha,-shcounterenw,-shgatpa,-shtvala,-shvsatpa,-shvstvala,-shvstvecd,-smaia,-smcdeleg,-smcsrind,-smdbltrp,-smepmp,-smmpm,-smnpm,-smrnmi,-smstateen,-ssaia,-ssccfg,-ssccptr,-sscofpmf,-sscounterenw,-sscsrind,-ssdbltrp,-ssnpm,-sspm,-ssqosid,-ssstateen,-ssstrict,-sstc,-sstvala,-sstvecd,-ssu64xl,-supm,-svade,-svadu,-svbare,-svinval,-svnapot,-svpbmt,-svvptc,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xmipscmove,-xmipslsp,-xsfcease,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xsifivecdiscarddlone,-xsifivecflushdlone,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-xwchc,-za128rs,-za64rs,-zabha,-zacas,-zama16b,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmop,-zcmp,-zcmt,-zdinx,-zfa,-zfbfmin,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zic64b,-zicbom,-zicbop,-zicboz,-ziccamoa,-ziccif,-zicclsm,-ziccrse,-zicntr,-zicond,-zihintntl,-zihintpause,-zihpm,-zimop,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-ztso,-zvbb,-zvbc,-zvfbfmin,-zvfbfwma,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl4096b,-zvl512b,-zvl65536b,-zvl8192b" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, producer: "clang version 21.0.0git")
+!1 = !DIFile(filename: "test.c", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+!5 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 1, scopeLine: 1, type: !10, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!6 = distinct !DIAssignID()
+!7 = !DILocalVariable(name: "i", scope: !5, file: !1, line: 7, type: !8)
+!8 = !DIBasicType(name: "int32_t", size: 32, encoding: DW_ATE_signed)
+!9 = !DILocation(line: 5, scope: !5)
+!10 = !DISubroutineType(types: !2)
+!11 = !{!7, !17}
+!12 = distinct !DIAssignID()
+!13 = distinct !DIAssignID()
+!14 = distinct !DIAssignID()
+!17 = !DILocalVariable(name: "j", scope: !5, file: !1, line: 7, type: !8)
diff --git a/llvm/test/Verifier/diassignid-vector-stores.ll b/llvm/test/Verifier/diassignid-vector-stores.ll
new file mode 100644
index 0000000000000..7fe1ee85e6c9d
--- /dev/null
+++ b/llvm/test/Verifier/diassignid-vector-stores.ll
@@ -0,0 +1,33 @@
+; RUN: llvm-as -disable-output <%s 2>&1 | FileCheck %s --implicit-check-not="attached to unexpected instruction kind"
+;; Check that we allow vector store intrinsics to have !DIAssignID attachments,
+;; but we do not allow non-store intrinsics to have them.
+
+; CHECK: !DIAssignID attached to unexpected instruction kind
+; CHECK-NEXT: @llvm.vp.load.v2i8.p0
+
+define void @f() !dbg !5 {
+  call void @llvm.vp.store.v2i8.p0(<2 x i8> undef, ptr undef, <2 x i1> undef, i32 undef), !DIAssignID !6
+  call void @llvm.vp.scatter.v2i8.v2p0(<2 x i8> undef, <2 x ptr> undef, <2 x i1> undef, i32 undef), !DIAssignID !7
+  call void @llvm.experimental.vp.strided.store.v2i8.i64(<2 x i8> undef, ptr undef, i64 undef, <2 x i1> undef, i32 undef), !DIAssignID !8
+  call void @llvm.masked.store.v2i8.p0(<2 x i8> undef, ptr undef, i32 1, <2 x i1> undef), !DIAssignID !9
+  call void @llvm.masked.scatter.v2i8.v2p0(<2 x i8> undef, <2 x ptr> undef, i32 1, <2 x i1> undef), !DIAssignID !10
+  %r = call <2 x i8> @llvm.vp.load.v2i8.p0(ptr undef, <2 x i1> undef, i32 undef), !DIAssignID !11
+  ret void
+}
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_Swift, producer: "clang",
+                             file: !2, emissionKind: 2)
+!2 = !DIFile(filename: "path/to/file", directory: "/path/to/dir")
+!3 = !{null}
+!4 = !DISubroutineType(types: !3)
+!5 = distinct !DISubprogram(name: "f", scope: !2, file: !2, line: 1, type: !4, scopeLine: 2, unit: !1)
+!6 = distinct !DIAssignID()
+!7 = distinct !DIAssignID()
+!8 = distinct !DIAssignID()
+!9 = distinct !DIAssignID()
+!10 = distinct !DIAssignID()
+!11 = distinct !DIAssignID()

[github-actions]

✅ With the latest revision this PR passed the undef deprecator.

[jmorse]

Do we need a test covering the poison/undef path, i.e. an untagged unknown store to something where there's not a live dbg.value?

Can we use Ìnstruction::mayWriteToMemory` as the test for whether a store is unknown, or is that signing us up to too much maintenance? It might make sense if we allow any intrinsic that writes to memory to adopt this behaviour.

[SLTozer]

Do we need a test covering the poison/undef path, i.e. an untagged unknown store to something where there's not a live dbg.value?

It's unlikely to be a source of errors, but it should be covered.

Can we use Ìnstruction::mayWriteToMemory` as the test for whether a store is unknown, or is that signing us up to too much maintenance? It might make sense if we allow any intrinsic that writes to memory to adopt this behaviour.

We might use it in the verifier to determine whether a given instruction should be allowed to have a DIAssignID attached; in AssignmentTrackingAnalysis for the "unknown store" case, I'll have to look into it - the important part is that we can identify the base address that is being written to, which looks like it might be possible.

[OCHyams]

Tyvm for handling this.

Code changes SGTM, with one inline question.

More broadly, it's unfortunate to remove coverage for the entire variable. Part of me wonders whether it's better to go the other way, and state it's a def for the whole variable. This is obviously liable to introduce incorrect locations, but if we compare it to dbg.declare semantics (which this stuff sort-of replaces) I don't think it'd be a regression there. I'm not sure how convinced I am by my own argument; I'm not a massive fan of either option, but we do need to do something. cc @jmorse in case he wants to chime in.

[OCHyams]

Could a vp_scatter store to multiple different allocas? My question is basically whether it's a given that we can return a single "base" pointer for any given scatter?

[SLTozer]

It isn't a given, but all the examples I've seen do store to different offsets at a single alloca though, so I decided not to over-complicate this logic. In practice this is, as far as I can tell, quite a narrowly used intrinsic and the default behaviour of simply ignoring the intrinsic won't cause any errors, so it's probably not worth trying to cover all our bases here until we see this be a problem.

[asb]

Perhaps llvm.masked.compressstore be included here as well?

[asb]

I can confirm that with this patch applied. I can compile SPEC in the llvm-test-suite with -g and the vector extension. As commented inline, I think llvm.masked.compressstore is probably missed.

There's also the case of target-specific instrinsics, e.g. llvm.riscv.vse. These should only be produced by the C intrinsics and I guess don't currently have the metadata attached, but I'm not sure there's a fundamental reason a frontend wouldn't attach DIAssignID metadata.

I don't feel well placed to review the whole PR, but with the query about llvm.masked.compressstore answered the verifier changes LGTM.

[SLTozer]

There's also the case of target-specific instrinsics, e.g. llvm.riscv.vse. These should only be produced by the C intrinsics and I guess don't currently have the metadata attached, but I'm not sure there's a fundamental reason a frontend wouldn't attach DIAssignID metadata.

I think it's probably a good idea to follow the original suggestion and just relax the verifier check - the rest of the changes in this patch are still useful, and if a store intrinsic wants to be handled more correctly when it's untagged then it should be handled in getUnknownStore, but I don't think I'm in a position to add that for all possible store intrinsics. With that said, at least all of the general llvm intrinsics should be handled correctly, so I'll include llvm.masked.compressstore.

[SLTozer]

Ping for the last set of code changes, in which I relaxed the verifier to accept DIAssignIDs on all intrinsics and added support for masked_compressstore (as described above).

[OCHyams]

I don't want to block this, since it fixes an assertion. But I don't think my question was addressed / answered:

More broadly, it's unfortunate to remove coverage for the entire variable. Part of me wonders whether it's better to go the other way, and state it's a def for the whole variable. This is obviously liable to introduce incorrect locations, but if we compare it to dbg.declare semantics (which this stuff sort-of replaces) I don't think it'd be a regression there. I'm not sure how convinced I am by my own argument; I'm not a massive fan of either option, but we do need to do something. cc @jmorse in case he wants to chime in.

I'm leaning slightly more that way now, but it's better to do what you've got here already than assert.

[OCHyams]

(so... tentative LGTM)