[Assignment Tracking][SROA] Handle createFragmentExpression failure

[Orlando]

Summary

createFragmentExpression will fail if it determines that the expression cannot be split over fragments. Handle this case in SROA. Similarly to D147312 this should be a rare occurrence as the dbg.assign will usually reference the Value being stored without modifying it with a DIExpression.

[jmorse]

LGTM, in that it's avoiding an assertion through graceful degradation of the debug-info. Out of curiosity, for what reason can't a fragment expression be created in the test?

[Orlando]

Thanks. It's the DW_OP_plus_uconst in the test causing createFragmentExpression to fail -- see createFragmentExpression in DebugInfoMetadata.cpp, which avoids splitting arithmetic expressions.

[StephenTozer]

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/fail-fragment.ll

12

I'm curious - why is this value not poison? I'm assuming it's the upper fragment for the dbg.assign that couldn't be split, so I'd have thought it would be a kill location as well.

15–16

Tiniest nit.

[Orlando]

I'm curious - why is this value not poison? I'm assuming it's the upper fragment for the dbg.assign that couldn't be split, so I'd have thought it would be a kill location as well.

Good question. It turns out it is poison in the call to migrateDebugInfo when the store is split, but then in another call to migrateDebugInfo later the poison value gets replaced. Nice spot.

The easy fix is to preserve poison-ness of the dbg.assign. However, a dbg.assign linked to a memcpy will have a poison value component. If SROA replaces that memcpy with stores the behaviour at the moment is to replace the poison with the stored-value.

The underlying issue here for both the original issue I was fixing and this poison situation is that the dbg.assign value isn't represented in the same way as the stored value. I think the fix for the original issue makes sense, the "easy fix" mention above also makes sense, and it's the memcpy-linked dbg.assign value representation that doesn't quite fit.

Not quite sure how to solve this just yet - ideas are welcome!

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/fail-fragment.ll

12

(replying with a main-line comment)

[Orlando]

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/fail-fragment.ll

12

I've added a FIXME to the test. I've landed this as-is for now to fix the assertion and live with this edge-case, I'll come back to this.

15–16

I turned the other CHECKS into the shorter-hand version

llvm/lib/Transforms/Scalar/SROA.cpp

static void migrateDebugInfo(AllocaInst *OldAlloca, bool IsSplit,
   auto &Ctx = Inst->getContext();
   DIBuilder DIB(*OldInst->getModule(), /*AllowUnresolved*/ false);
   assert(OldAlloca->isStaticAlloca());
 
   for (DbgAssignIntrinsic *DbgAssign : MarkerRange) {
     LLVM_DEBUG(dbgs() << "      existing dbg.assign is: " << *DbgAssign
                       << "\n");
     auto *Expr = DbgAssign->getExpression();
+    bool SetKillLocation = false;
 
     if (IsSplit) {
       std::optional<DIExpression::FragmentInfo> BaseFragment = std::nullopt;
       {
         auto R = BaseFragments.find(getAggregateVariable(DbgAssign));
         if (R == BaseFragments.end())
           continue;
         BaseFragment = R->second;
if (IsSplit) {
       if (!(NewFragment == CurrentFragment)) {
         if (CurrentFragment) {
           // Rewrite NewFragment to be relative to the existing one (this is
           // what createFragmentExpression wants).  CalculateFragment has
           // already resolved the size for us. FIXME: Should it return the
           // relative fragment too?
           NewFragment->OffsetInBits -= CurrentFragment->OffsetInBits;
         }
-
-        auto E = DIExpression::createFragmentExpression(
-            Expr, NewFragment->OffsetInBits, NewFragment->SizeInBits);
-        assert(E && "Failed to create fragment expr!");
+        // Add the new fragment info to the existing expression if possible.
+        if (auto E = DIExpression::createFragmentExpression(
+                Expr, NewFragment->OffsetInBits, NewFragment->SizeInBits)) {
           Expr = *E;
+        } else {
+          // Otherwise, add the new fragment info to an empty expression and
+          // discard the value component of this dbg.assign as the value cannot
+          // be computed with the new fragment.
+          Expr = *DIExpression::createFragmentExpression(
+              DIExpression::get(Expr->getContext(), std::nullopt),
+              NewFragment->OffsetInBits, NewFragment->SizeInBits);
+          SetKillLocation = true;
+        }
       }
     }
 
     // If we haven't created a DIAssignID ID do that now and attach it to Inst.
     if (!NewID) {
       NewID = DIAssignID::getDistinct(Ctx);
       Inst->setMetadata(LLVMContext::MD_DIAssignID, NewID);
     }
 
     ::Value *NewValue = Value ? Value : DbgAssign->getValue();
     auto *NewAssign = DIB.insertDbgAssign(
         Inst, NewValue, DbgAssign->getVariable(), Expr, Dest,
         DIExpression::get(Ctx, std::nullopt), DbgAssign->getDebugLoc());
 
     // If we've updated the value but the original dbg.assign has an arglist
     // then kill it now - we can't use the requested new value.
     // We can't replace the DIArgList with the new value as it'd leave
     // the DIExpression in an invalid state (DW_OP_LLVM_arg operands without
     // an arglist). And we can't keep the DIArgList in case the linked store
     // is being split - in which case the DIArgList + expression may no longer
     // be computing the correct value.
     // This should be a very rare situation as it requires the value being
     // stored to differ from the dbg.assign (i.e., the value has been
     // represented differently in the debug intrinsic for some reason).
-    if (DbgAssign->hasArgList() && Value)
+    SetKillLocation |= DbgAssign->hasArgList() && Value;
+    if (SetKillLocation)
       NewAssign->setKillLocation();
 
     // We could use more precision here at the cost of some additional (code)
     // complexity - if the original dbg.assign was adjacent to its store, we
     // could position this new dbg.assign adjacent to its store rather than the
     // old dbg.assgn. That would result in interleaved dbg.assigns rather than
     // what we get now:
     //    split store !1

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/fail-fragment.ll

+; RUN: opt -passes=sroa -S %s -o - \
+; RUN: | FileCheck %s --implicit-check-not="call void @llvm.dbg"
+
+;; Check that a dbg.assign for a promoted variable becomes a kill location if
+;; it used a fragment that can't be split (the first check directive below).
+;; NOTE: If createFragmentExpression gets smarter it may be necessary to create
+;; a new test case.
+
+; CHECK: if.then:
+; CHECK: dbg.value(metadata i32 poison, metadata ![[#]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
+;; FIXME: The value below should be poison. See https://reviews.llvm.org/D147431#4245260.
+; CHECK: dbg.value(metadata i32 %{{.*}}, metadata ![[#]], metadata !DIExpression(DW_OP_LLVM_fragment, 32, 32))
+
+; CHECK: if.else:
+; CHECK: dbg.value(metadata i32 2, metadata ![[#]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
+; CHECK: dbg.value(metadata i32 0, metadata ![[#]], metadata !DIExpression(DW_OP_LLVM_fragment, 32, 32))
+
+; CHECK: end:
+; CHECK: dbg.value(metadata i32 %{{.*}}, metadata ![[#]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
+
+declare i64 @get_i64()
+
+define internal fastcc i64 @fun() !dbg !18 {
+entry:
+  %codepoint = alloca i64, align 4, !DIAssignID !27
+  call void @llvm.dbg.assign(metadata i1 poison, metadata !15, metadata !DIExpression(), metadata !27, metadata ptr %codepoint, metadata !DIExpression()), !dbg !26
+   %0 = call i64 @get_i64() #2
+   %1 = add i64 %0, 1
+   %cmp = icmp ugt i64 %1, 100
+   br i1 %cmp, label %if.then, label %if.else
+
+if.then:
+  store i64 %1, ptr %codepoint, align 4, !DIAssignID !25
+  call void @llvm.dbg.assign(metadata i64 %1, metadata !15, metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value), metadata !25, metadata ptr %codepoint, metadata !DIExpression()), !dbg !26
+  br label %end
+
+if.else:
+  store i64 2, ptr %codepoint, align 4, !DIAssignID !28
+  call void @llvm.dbg.assign(metadata i32 2, metadata !15, metadata !DIExpression(), metadata !28, metadata ptr %codepoint, metadata !DIExpression()), !dbg !26
+  br label %end
+
+end:
+  %r = load i32, ptr %codepoint
+  %rr = zext i32 %r to i64
+  ret i64 %rr
+}
+
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!13, !14}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 17.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !2, imports: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "reduce.cpp", directory: "/")
+!2 = !{}
+!13 = !{i32 2, !"Debug Info Version", i32 3}
+!14 = !{i32 7, !"debug-info-assignment-tracking", i1 true}
+!15 = !DILocalVariable(name: "codepoint", scope: !18, file: !1, line: 10, type: !24)
+!18 = distinct !DISubprogram(name: "fun", linkageName: "fun", scope: !1, file: !1, line: 4, type: !19, scopeLine: 4, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!19 = distinct !DISubroutineType(types: !2)
+!24 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!25 = distinct !DIAssignID()
+!26 = !DILocation(line: 0, scope: !18)
+!27 = distinct !DIAssignID()
+!28 = distinct !DIAssignID()