[ISelDAG] Salvage debug info at isel by referring to frame indices. #109126

bevin-hansson · 2024-09-18T12:02:51Z

We can refer to frame index locations when salvaging debug info
for certain nodes, which prevents the compiler from optimizing
out the location.

We can refer to frame index locations when salvaging debug info for certain nodes, which prevents the compiler from optimizing out the location.

llvmbot · 2024-09-18T12:03:27Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-selectiondag

Author: Bevin Hansson (bevin-hansson)

Changes

We can refer to frame index locations when salvaging debug info
for certain nodes, which prevents the compiler from optimizing
out the location.

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

3 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+13-3)
(added) llvm/test/CodeGen/SPARC/salvage-debug-isel.ll (+85)
(modified) llvm/test/CodeGen/X86/pr57673.ll (+2-2)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 3918da3ef031b6..0b59a22b340bfc 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -11237,6 +11237,12 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
   if (!N.getHasDebugValue())
     return;
 
+  auto handle = [](SDNode *Node, unsigned ResNo) {
+    if (auto *FISDN = dyn_cast<FrameIndexSDNode>(Node))
+      return SDDbgOperand::fromFrameIdx(FISDN->getIndex());
+    return SDDbgOperand::fromNode(Node, ResNo);
+  };
+
   SmallVector<SDDbgValue *, 2> ClonedDVs;
   for (auto *DV : GetDbgValues(&N)) {
     if (DV->isInvalidated())
@@ -11272,7 +11278,7 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
           if (NewLocOps[i].getKind() != SDDbgOperand::SDNODE ||
               NewLocOps[i].getSDNode() != &N)
             continue;
-          NewLocOps[i] = SDDbgOperand::fromNode(N0.getNode(), N0.getResNo());
+          NewLocOps[i] = handle(N0.getNode(), N0.getResNo());
           if (RHSConstant) {
             SmallVector<uint64_t, 3> ExprOps;
             DIExpression::appendOffset(ExprOps, Offset);
@@ -11327,7 +11333,7 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
             NewLocOps[i].getSDNode() != &N)
           continue;
 
-        NewLocOps[i] = SDDbgOperand::fromNode(N0.getNode(), N0.getResNo());
+        NewLocOps[i] = handle(N0.getNode(), N0.getResNo());
         DbgExpression = DIExpression::appendOpsToArg(DbgExpression, ExtOps, i);
         Changed = true;
       }
@@ -11350,7 +11356,11 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
   }
 
   for (SDDbgValue *Dbg : ClonedDVs) {
-    assert(!Dbg->getSDNodes().empty() &&
+    assert((!Dbg->getSDNodes().empty() ||
+            llvm::any_of(Dbg->getLocationOps(),
+                         [&](const SDDbgOperand &Op) {
+                           return Op.getKind() == SDDbgOperand::FRAMEIX;
+                         })) &&
            "Salvaged DbgValue should depend on a new SDNode");
     AddDbgValue(Dbg, false);
   }
diff --git a/llvm/test/CodeGen/SPARC/salvage-debug-isel.ll b/llvm/test/CodeGen/SPARC/salvage-debug-isel.ll
new file mode 100644
index 00000000000000..af0a6ba25f97ca
--- /dev/null
+++ b/llvm/test/CodeGen/SPARC/salvage-debug-isel.ll
@@ -0,0 +1,85 @@
+; RUN: llc -march=sparc -O1 %s -o - | FileCheck %s
+
+; Debug info salvaging in isel means we should see a location for this variable.
+
+; CHECK-LABEL: a:
+; CHECK:    !DEBUG_VALUE: a:d <- [DW_OP_plus_uconst 98, DW_OP_plus_uconst 3, DW_OP_stack_value] $o6
+
+define dso_local zeroext i16 @a() local_unnamed_addr #0 !dbg !7 {
+entry:
+  %b = alloca [6 x i8], align 1, !DIAssignID !24
+    #dbg_assign(i1 undef, !14, !DIExpression(), !24, ptr %b, !DIExpression(), !25)
+  call void @llvm.lifetime.start.p0(i64 6, ptr nonnull %b) #2, !dbg !26
+  %arrayidx = getelementptr inbounds [6 x i8], ptr %b, i32 0, i32 undef, !dbg !27
+  store i8 4, ptr %arrayidx, align 1, !dbg !28, !tbaa !29
+  %arrayidx1 = getelementptr inbounds i8, ptr %b, i32 3, !dbg !32
+    #dbg_value(ptr %arrayidx1, !22, !DIExpression(), !25)
+  %0 = load i8, ptr %arrayidx1, align 1, !dbg !33, !tbaa !29
+  %tobool.not = icmp eq i8 %0, 0, !dbg !35
+  br i1 %tobool.not, label %if.end, label %for.cond, !dbg !36
+
+for.cond:                                         ; preds = %entry, %for.cond
+  br label %for.cond, !dbg !37, !llvm.loop !40
+
+if.end:                                           ; preds = %entry
+  call void @llvm.lifetime.end.p0(i64 6, ptr nonnull %b) #2, !dbg !44
+  ret i16 undef, !dbg !44
+}
+
+declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture) #1
+
+declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture) #1
+
+attributes #0 = { nofree noinline norecurse nosync nounwind memory(none) "no-builtins" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #1 = { mustprogress nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 20.0.0git.prerel", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "file.c", directory: "/path", checksumkind: CSK_MD5, checksum: "aa7b5139660a2329a6409414c44cc1f6")
+!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, !"debug-info-assignment-tracking", i1 true}
+!6 = !{!"clang version 20.0.0git.prerel"}
+!7 = distinct !DISubprogram(name: "a", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIDerivedType(tag: DW_TAG_typedef, name: "uint16_t", file: !11, line: 277, baseType: !12)
+!11 = !DIFile(filename: "stdint.h", directory: "", checksumkind: CSK_MD5, checksum: "d9e8f73f3756bbd642f1729623e09484")
+!12 = !DIBasicType(name: "unsigned short", size: 16, encoding: DW_ATE_unsigned)
+!13 = !{!14, !20, !22}
+!14 = !DILocalVariable(name: "b", scope: !7, file: !1, line: 3, type: !15)
+!15 = !DICompositeType(tag: DW_TAG_array_type, baseType: !16, size: 48, elements: !18)
+!16 = !DIDerivedType(tag: DW_TAG_typedef, name: "int8_t", file: !11, line: 298, baseType: !17)
+!17 = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+!18 = !{!19}
+!19 = !DISubrange(count: 6)
+!20 = !DILocalVariable(name: "c", scope: !7, file: !1, line: 4, type: !21)
+!21 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!22 = !DILocalVariable(name: "d", scope: !7, file: !1, line: 6, type: !23)
+!23 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !16, size: 32)
+!24 = distinct !DIAssignID()
+!25 = !DILocation(line: 0, scope: !7)
+!26 = !DILocation(line: 3, column: 3, scope: !7)
+!27 = !DILocation(line: 5, column: 3, scope: !7)
+!28 = !DILocation(line: 5, column: 8, scope: !7)
+!29 = !{!30, !30, i64 0}
+!30 = !{!"omnipotent char", !31, i64 0}
+!31 = !{!"Simple C/C++ TBAA"}
+!32 = !DILocation(line: 6, column: 16, scope: !7)
+!33 = !DILocation(line: 7, column: 33, scope: !34)
+!34 = distinct !DILexicalBlock(scope: !7, file: !1, line: 7, column: 7)
+!35 = !DILocation(line: 7, column: 7, scope: !34)
+!36 = !DILocation(line: 7, column: 7, scope: !7)
+!37 = !DILocation(line: 8, column: 5, scope: !38)
+!38 = distinct !DILexicalBlock(scope: !39, file: !1, line: 8, column: 5)
+!39 = distinct !DILexicalBlock(scope: !34, file: !1, line: 8, column: 5)
+!40 = distinct !{!40, !41, !42, !43}
+!41 = !DILocation(line: 8, column: 5, scope: !39)
+!42 = !DILocation(line: 9, column: 7, scope: !39)
+!43 = !{!"llvm.loop.unroll.disable"}
+!44 = !DILocation(line: 10, column: 1, scope: !7)
diff --git a/llvm/test/CodeGen/X86/pr57673.ll b/llvm/test/CodeGen/X86/pr57673.ll
index cf7717f420480b..4ca8ae91f9e6fc 100644
--- a/llvm/test/CodeGen/X86/pr57673.ll
+++ b/llvm/test/CodeGen/X86/pr57673.ll
@@ -37,7 +37,7 @@ define void @foo() {
   ; NORMAL-NEXT: {{  $}}
   ; NORMAL-NEXT:   [[MOVUPSrm:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i4, align 8)
   ; NORMAL-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm]] :: (store (s128) into `ptr null`, align 8)
-  ; NORMAL-NEXT:   DBG_VALUE $noreg, $noreg, !3, !DIExpression(), debug-location !8
+  ; NORMAL-NEXT:   DBG_VALUE_LIST !3, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 40, DW_OP_stack_value), %stack.1.i, %stack.1.i, debug-location !8
   ; NORMAL-NEXT:   [[MOVUPSrm1:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i6, align 8)
   ; NORMAL-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm1]] :: (store (s128) into `ptr null`, align 8)
   ; NORMAL-NEXT: {{  $}}
@@ -76,7 +76,7 @@ define void @foo() {
   ; INSTRREF-NEXT: {{  $}}
   ; INSTRREF-NEXT:   [[MOVUPSrm:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i4, align 8)
   ; INSTRREF-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm]] :: (store (s128) into `ptr null`, align 8)
-  ; INSTRREF-NEXT:   DBG_VALUE $noreg, $noreg, !3, !DIExpression(), debug-location !8
+  ; INSTRREF-NEXT:   DBG_VALUE_LIST !3, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 40, DW_OP_stack_value), %stack.1.i, %stack.1.i, debug-location !8
   ; INSTRREF-NEXT:   [[MOVUPSrm1:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i6, align 8)
   ; INSTRREF-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm1]] :: (store (s128) into `ptr null`, align 8)
   ; INSTRREF-NEXT: {{  $}}

llvmbot · 2024-09-18T12:03:27Z

@llvm/pr-subscribers-backend-x86

Author: Bevin Hansson (bevin-hansson)

Changes

We can refer to frame index locations when salvaging debug info
for certain nodes, which prevents the compiler from optimizing
out the location.

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

3 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+13-3)
(added) llvm/test/CodeGen/SPARC/salvage-debug-isel.ll (+85)
(modified) llvm/test/CodeGen/X86/pr57673.ll (+2-2)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 3918da3ef031b6..0b59a22b340bfc 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -11237,6 +11237,12 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
   if (!N.getHasDebugValue())
     return;
 
+  auto handle = [](SDNode *Node, unsigned ResNo) {
+    if (auto *FISDN = dyn_cast<FrameIndexSDNode>(Node))
+      return SDDbgOperand::fromFrameIdx(FISDN->getIndex());
+    return SDDbgOperand::fromNode(Node, ResNo);
+  };
+
   SmallVector<SDDbgValue *, 2> ClonedDVs;
   for (auto *DV : GetDbgValues(&N)) {
     if (DV->isInvalidated())
@@ -11272,7 +11278,7 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
           if (NewLocOps[i].getKind() != SDDbgOperand::SDNODE ||
               NewLocOps[i].getSDNode() != &N)
             continue;
-          NewLocOps[i] = SDDbgOperand::fromNode(N0.getNode(), N0.getResNo());
+          NewLocOps[i] = handle(N0.getNode(), N0.getResNo());
           if (RHSConstant) {
             SmallVector<uint64_t, 3> ExprOps;
             DIExpression::appendOffset(ExprOps, Offset);
@@ -11327,7 +11333,7 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
             NewLocOps[i].getSDNode() != &N)
           continue;
 
-        NewLocOps[i] = SDDbgOperand::fromNode(N0.getNode(), N0.getResNo());
+        NewLocOps[i] = handle(N0.getNode(), N0.getResNo());
         DbgExpression = DIExpression::appendOpsToArg(DbgExpression, ExtOps, i);
         Changed = true;
       }
@@ -11350,7 +11356,11 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
   }
 
   for (SDDbgValue *Dbg : ClonedDVs) {
-    assert(!Dbg->getSDNodes().empty() &&
+    assert((!Dbg->getSDNodes().empty() ||
+            llvm::any_of(Dbg->getLocationOps(),
+                         [&](const SDDbgOperand &Op) {
+                           return Op.getKind() == SDDbgOperand::FRAMEIX;
+                         })) &&
            "Salvaged DbgValue should depend on a new SDNode");
     AddDbgValue(Dbg, false);
   }
diff --git a/llvm/test/CodeGen/SPARC/salvage-debug-isel.ll b/llvm/test/CodeGen/SPARC/salvage-debug-isel.ll
new file mode 100644
index 00000000000000..af0a6ba25f97ca
--- /dev/null
+++ b/llvm/test/CodeGen/SPARC/salvage-debug-isel.ll
@@ -0,0 +1,85 @@
+; RUN: llc -march=sparc -O1 %s -o - | FileCheck %s
+
+; Debug info salvaging in isel means we should see a location for this variable.
+
+; CHECK-LABEL: a:
+; CHECK:    !DEBUG_VALUE: a:d <- [DW_OP_plus_uconst 98, DW_OP_plus_uconst 3, DW_OP_stack_value] $o6
+
+define dso_local zeroext i16 @a() local_unnamed_addr #0 !dbg !7 {
+entry:
+  %b = alloca [6 x i8], align 1, !DIAssignID !24
+    #dbg_assign(i1 undef, !14, !DIExpression(), !24, ptr %b, !DIExpression(), !25)
+  call void @llvm.lifetime.start.p0(i64 6, ptr nonnull %b) #2, !dbg !26
+  %arrayidx = getelementptr inbounds [6 x i8], ptr %b, i32 0, i32 undef, !dbg !27
+  store i8 4, ptr %arrayidx, align 1, !dbg !28, !tbaa !29
+  %arrayidx1 = getelementptr inbounds i8, ptr %b, i32 3, !dbg !32
+    #dbg_value(ptr %arrayidx1, !22, !DIExpression(), !25)
+  %0 = load i8, ptr %arrayidx1, align 1, !dbg !33, !tbaa !29
+  %tobool.not = icmp eq i8 %0, 0, !dbg !35
+  br i1 %tobool.not, label %if.end, label %for.cond, !dbg !36
+
+for.cond:                                         ; preds = %entry, %for.cond
+  br label %for.cond, !dbg !37, !llvm.loop !40
+
+if.end:                                           ; preds = %entry
+  call void @llvm.lifetime.end.p0(i64 6, ptr nonnull %b) #2, !dbg !44
+  ret i16 undef, !dbg !44
+}
+
+declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture) #1
+
+declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture) #1
+
+attributes #0 = { nofree noinline norecurse nosync nounwind memory(none) "no-builtins" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #1 = { mustprogress nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 20.0.0git.prerel", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "file.c", directory: "/path", checksumkind: CSK_MD5, checksum: "aa7b5139660a2329a6409414c44cc1f6")
+!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, !"debug-info-assignment-tracking", i1 true}
+!6 = !{!"clang version 20.0.0git.prerel"}
+!7 = distinct !DISubprogram(name: "a", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !13)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIDerivedType(tag: DW_TAG_typedef, name: "uint16_t", file: !11, line: 277, baseType: !12)
+!11 = !DIFile(filename: "stdint.h", directory: "", checksumkind: CSK_MD5, checksum: "d9e8f73f3756bbd642f1729623e09484")
+!12 = !DIBasicType(name: "unsigned short", size: 16, encoding: DW_ATE_unsigned)
+!13 = !{!14, !20, !22}
+!14 = !DILocalVariable(name: "b", scope: !7, file: !1, line: 3, type: !15)
+!15 = !DICompositeType(tag: DW_TAG_array_type, baseType: !16, size: 48, elements: !18)
+!16 = !DIDerivedType(tag: DW_TAG_typedef, name: "int8_t", file: !11, line: 298, baseType: !17)
+!17 = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+!18 = !{!19}
+!19 = !DISubrange(count: 6)
+!20 = !DILocalVariable(name: "c", scope: !7, file: !1, line: 4, type: !21)
+!21 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!22 = !DILocalVariable(name: "d", scope: !7, file: !1, line: 6, type: !23)
+!23 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !16, size: 32)
+!24 = distinct !DIAssignID()
+!25 = !DILocation(line: 0, scope: !7)
+!26 = !DILocation(line: 3, column: 3, scope: !7)
+!27 = !DILocation(line: 5, column: 3, scope: !7)
+!28 = !DILocation(line: 5, column: 8, scope: !7)
+!29 = !{!30, !30, i64 0}
+!30 = !{!"omnipotent char", !31, i64 0}
+!31 = !{!"Simple C/C++ TBAA"}
+!32 = !DILocation(line: 6, column: 16, scope: !7)
+!33 = !DILocation(line: 7, column: 33, scope: !34)
+!34 = distinct !DILexicalBlock(scope: !7, file: !1, line: 7, column: 7)
+!35 = !DILocation(line: 7, column: 7, scope: !34)
+!36 = !DILocation(line: 7, column: 7, scope: !7)
+!37 = !DILocation(line: 8, column: 5, scope: !38)
+!38 = distinct !DILexicalBlock(scope: !39, file: !1, line: 8, column: 5)
+!39 = distinct !DILexicalBlock(scope: !34, file: !1, line: 8, column: 5)
+!40 = distinct !{!40, !41, !42, !43}
+!41 = !DILocation(line: 8, column: 5, scope: !39)
+!42 = !DILocation(line: 9, column: 7, scope: !39)
+!43 = !{!"llvm.loop.unroll.disable"}
+!44 = !DILocation(line: 10, column: 1, scope: !7)
diff --git a/llvm/test/CodeGen/X86/pr57673.ll b/llvm/test/CodeGen/X86/pr57673.ll
index cf7717f420480b..4ca8ae91f9e6fc 100644
--- a/llvm/test/CodeGen/X86/pr57673.ll
+++ b/llvm/test/CodeGen/X86/pr57673.ll
@@ -37,7 +37,7 @@ define void @foo() {
   ; NORMAL-NEXT: {{  $}}
   ; NORMAL-NEXT:   [[MOVUPSrm:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i4, align 8)
   ; NORMAL-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm]] :: (store (s128) into `ptr null`, align 8)
-  ; NORMAL-NEXT:   DBG_VALUE $noreg, $noreg, !3, !DIExpression(), debug-location !8
+  ; NORMAL-NEXT:   DBG_VALUE_LIST !3, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 40, DW_OP_stack_value), %stack.1.i, %stack.1.i, debug-location !8
   ; NORMAL-NEXT:   [[MOVUPSrm1:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i6, align 8)
   ; NORMAL-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm1]] :: (store (s128) into `ptr null`, align 8)
   ; NORMAL-NEXT: {{  $}}
@@ -76,7 +76,7 @@ define void @foo() {
   ; INSTRREF-NEXT: {{  $}}
   ; INSTRREF-NEXT:   [[MOVUPSrm:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i4, align 8)
   ; INSTRREF-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm]] :: (store (s128) into `ptr null`, align 8)
-  ; INSTRREF-NEXT:   DBG_VALUE $noreg, $noreg, !3, !DIExpression(), debug-location !8
+  ; INSTRREF-NEXT:   DBG_VALUE_LIST !3, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_plus_uconst, 40, DW_OP_stack_value), %stack.1.i, %stack.1.i, debug-location !8
   ; INSTRREF-NEXT:   [[MOVUPSrm1:%[0-9]+]]:vr128 = MOVUPSrm %stack.1.i, 1, $noreg, 40, $noreg :: (load (s128) from %ir.i6, align 8)
   ; INSTRREF-NEXT:   MOVUPSmr $noreg, 1, $noreg, 0, $noreg, killed [[MOVUPSrm1]] :: (store (s128) into `ptr null`, align 8)
   ; INSTRREF-NEXT: {{  $}}

OCHyams

Thanks for the patch. Overall, LGTM with some minor nits.

At first I was confused about why the current behaviour doesn't cause any bugs, for future/others: without the patch the salvaged SDDbgValue refers to the the FrameIndex node (as an SDDbgOperand::SDNODE), and when that FrameIndex node gets removed later the operand gets replaced with undef. Note that when the FrameIndex node gets removed it goes through salvageDebugInfo too... which makes me wonder whether there's a more general solution we can add here? e.g., rather than calling handle, can we just handle salvaging of FrameIndex nodes independently. That is just speculation, no need to apply in this patch imo - not sure of all the consequences off the top of my head.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

llvm/test/CodeGen/SPARC/salvage-debug-isel.ll

Rename the lambda and clean up the test case a bit.

bevin-hansson · 2024-09-24T10:37:45Z

Thanks for the review! I think I addressed all of the comments.

OCHyams

One last thing that I missed first time round, then this LGTM, thanks

OCHyams · 2024-09-24T11:56:16Z

llvm/test/CodeGen/SPARC/salvage-debug-isel.ll

@@ -0,0 +1,69 @@
+; RUN: llc -march=sparc -O1 %s -o - | FileCheck %s


I think we should use llc -march=sparc %s -o - -stop-after=finalize-isel here to reduce the surface area we're testing to the interesting bit (isel) and change the CHECK(s) to:

CHECK: DBG_VALUE %stack.0.b, $noreg, ![[#]], !DIExpression(DW_OP_plus_uconst, 3, DW_OP_stack_value)

OCHyams · 2024-09-24T12:06:21Z

Thanks. Do you have commit access or should merge this for you?

bevin-hansson · 2024-09-24T12:17:12Z

I can commit it. Thanks!

[ISelDAG] Salvage debug info at isel by referring to frame indices.

bb902e2

We can refer to frame index locations when salvaging debug info for certain nodes, which prevents the compiler from optimizing out the location.

llvmbot added backend:X86 llvm:SelectionDAG SelectionDAGISel as well labels Sep 18, 2024

bevin-hansson requested a review from adrian-prantl September 18, 2024 12:13

arsenm added the debuginfo label Sep 18, 2024

bevin-hansson requested a review from jmorse September 19, 2024 07:07

OCHyams reviewed Sep 23, 2024

View reviewed changes

Address comments.

6bd1534

Rename the lambda and clean up the test case a bit.

OCHyams approved these changes Sep 24, 2024

View reviewed changes

Change test to stop after isel.

38cfca9

bevin-hansson merged commit 12033e5 into llvm:main Sep 24, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ISelDAG] Salvage debug info at isel by referring to frame indices. #109126

[ISelDAG] Salvage debug info at isel by referring to frame indices. #109126

Uh oh!

bevin-hansson commented Sep 18, 2024

Uh oh!

llvmbot commented Sep 18, 2024 •

edited

Loading

Uh oh!

llvmbot commented Sep 18, 2024

Uh oh!

OCHyams left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bevin-hansson commented Sep 24, 2024

Uh oh!

OCHyams left a comment

Uh oh!

OCHyams Sep 24, 2024

Uh oh!

OCHyams commented Sep 24, 2024

Uh oh!

bevin-hansson commented Sep 24, 2024

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1,69 @@
		; RUN: llc -march=sparc -O1 %s -o - \| FileCheck %s

[ISelDAG] Salvage debug info at isel by referring to frame indices. #109126

[ISelDAG] Salvage debug info at isel by referring to frame indices. #109126

Uh oh!

Conversation

bevin-hansson commented Sep 18, 2024

Uh oh!

llvmbot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 18, 2024

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bevin-hansson commented Sep 24, 2024

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

OCHyams Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

OCHyams commented Sep 24, 2024

Uh oh!

bevin-hansson commented Sep 24, 2024

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Sep 18, 2024 •

edited

Loading