[SelectionDAG] Salvage debug info for non-constant ADDs

[dstenb]

Teach SelectionDAG::salvageDebugInfo() to salvage debug information for
ADD nodes where the RHS is non-constant.

Co-authored-by: Mikael Holmen mikael.holmen@ericsson.com

• [DebugInfo] Precommit testcase for pointer addition with unknown offset
• [SelectionDAG] Salvage debug info for non-constant ADDs

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-debuginfo

Author: David (dstenb)

Changes

Teach SelectionDAG::salvageDebugInfo() to salvage debug information for
ADD nodes where the RHS is non-constant.

Co-authored-by: Mikael Holmen <mikael.holmen@ericsson.com>

• [DebugInfo] Precommit testcase for pointer addition with unknown offset
• [SelectionDAG] Salvage debug info for non-constant ADDs

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+31-7)
• (added) llvm/test/DebugInfo/Sparc/pointer-add-unknown-offset-debug-info.mir (+60)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index e831316efff52ba..200123a376d8e98 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -27,6 +27,7 @@
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/Analysis/VectorUtils.h"
+#include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/CodeGen/Analysis.h"
 #include "llvm/CodeGen/FunctionLoweringInfo.h"
 #include "llvm/CodeGen/ISDOpcodes.h"
@@ -10781,8 +10782,11 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
     case ISD::ADD: {
       SDValue N0 = N.getOperand(0);
       SDValue N1 = N.getOperand(1);
-      if (!isa<ConstantSDNode>(N0) && isa<ConstantSDNode>(N1)) {
-        uint64_t Offset = N.getConstantOperandVal(1);
+      if (!isa<ConstantSDNode>(N0)) {
+        bool RHSConstant = isa<ConstantSDNode>(N1);
+        uint64_t Offset;
+        if (RHSConstant)
+          Offset = N.getConstantOperandVal(1);
 
         // Rewrite an ADD constant node into a DIExpression. Since we are
         // performing arithmetic to compute the variable's *value* in the
@@ -10791,7 +10795,8 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
         auto *DIExpr = DV->getExpression();
         auto NewLocOps = DV->copyLocationOps();
         bool Changed = false;
-        for (size_t i = 0; i < NewLocOps.size(); ++i) {
+        size_t OrigLocOpsSize = NewLocOps.size();
+        for (size_t i = 0; i < OrigLocOpsSize; ++i) {
           // We're not given a ResNo to compare against because the whole
           // node is going away. We know that any ISD::ADD only has one
           // result, so we can assume any node match is using the result.
@@ -10799,19 +10804,38 @@ void SelectionDAG::salvageDebugInfo(SDNode &N) {
               NewLocOps[i].getSDNode() != &N)
             continue;
           NewLocOps[i] = SDDbgOperand::fromNode(N0.getNode(), N0.getResNo());
-          SmallVector<uint64_t, 3> ExprOps;
-          DIExpression::appendOffset(ExprOps, Offset);
-          DIExpr = DIExpression::appendOpsToArg(DIExpr, ExprOps, i, true);
+          if (RHSConstant) {
+            SmallVector<uint64_t, 3> ExprOps;
+            DIExpression::appendOffset(ExprOps, Offset);
+            DIExpr = DIExpression::appendOpsToArg(DIExpr, ExprOps, i, true);
+          } else {
+            // Convert to a variadic expression (if not already).
+            // convertToVariadicExpression() returns a const pointer, so we use
+            // a temporary const variable here.
+            const auto *TmpDIExpr =
+                DIExpression::convertToVariadicExpression(DIExpr);
+            SmallVector<uint64_t, 3> ExprOps;
+            ExprOps.push_back(dwarf::DW_OP_LLVM_arg);
+            ExprOps.push_back(NewLocOps.size());
+            ExprOps.push_back(dwarf::DW_OP_plus);
+            SDDbgOperand RHS =
+                SDDbgOperand::fromNode(N1.getNode(), N1.getResNo());
+            NewLocOps.push_back(RHS);
+            DIExpr = DIExpression::appendOpsToArg(TmpDIExpr, ExprOps, i, true);
+          }
           Changed = true;
         }
         (void)Changed;
         assert(Changed && "Salvage target doesn't use N");
 
+        bool IsVariadic =
+            DV->isVariadic() || OrigLocOpsSize != NewLocOps.size();
+
         auto AdditionalDependencies = DV->getAdditionalDependencies();
         SDDbgValue *Clone = getDbgValueList(DV->getVariable(), DIExpr,
                                             NewLocOps, AdditionalDependencies,
                                             DV->isIndirect(), DV->getDebugLoc(),
-                                            DV->getOrder(), DV->isVariadic());
+                                            DV->getOrder(), IsVariadic);
         ClonedDVs.push_back(Clone);
         DV->setIsInvalidated();
         DV->setIsEmitted();
diff --git a/llvm/test/DebugInfo/Sparc/pointer-add-unknown-offset-debug-info.mir b/llvm/test/DebugInfo/Sparc/pointer-add-unknown-offset-debug-info.mir
new file mode 100644
index 000000000000000..a9eabf895f0e6ad
--- /dev/null
+++ b/llvm/test/DebugInfo/Sparc/pointer-add-unknown-offset-debug-info.mir
@@ -0,0 +1,60 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3
+# RUN: llc -mtriple=sparcv9 %s -start-before=sparc-isel -o - -stop-after=sparc-isel | FileCheck %s
+--- |
+  target datalayout = "E-m:e-i64:64-n32:64-S128"
+  target triple = "sparcv9"
+
+  define void @pointer_add_unknown_offset(ptr %base, i32 %offset) !dbg !7 {
+  entry:
+    %idx.ext = sext i32 %offset to i64
+    %add.ptr = getelementptr inbounds i32, ptr %base, i64 %idx.ext
+    call void @llvm.dbg.value(metadata ptr %add.ptr, metadata !13, metadata !DIExpression()), !dbg !16
+    call void @llvm.dbg.value(metadata ptr %add.ptr, metadata !14, metadata !DIExpression(DW_OP_plus_uconst, 3, DW_OP_stack_value)), !dbg !16
+    call void @llvm.dbg.value(metadata !DIArgList(ptr %add.ptr, ptr %add.ptr), metadata !15, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus)), !dbg !16
+    store i32 42, ptr %add.ptr, align 4
+    ret void
+  }
+
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+  !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", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test.c", directory: "/tmp", checksumkind: CSK_MD5, checksum: "4321633e52cefeee6e307c697a82574f")
+  !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"}
+  !7 = distinct !DISubprogram(name: "pointer_add_unknown_offset", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{null, !10, !11}
+  !10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64)
+  !11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !12 = !{!13, !14, !15}
+  !13 = !DILocalVariable(name: "p", scope: !7, file: !1, line: 2, type: !10)
+  !14 = !DILocalVariable(name: "q", scope: !7, file: !1, line: 2, type: !10)
+  !15 = !DILocalVariable(name: "r", scope: !7, file: !1, line: 2, type: !10)
+  !16 = !DILocation(line: 0, scope: !7)
+...
+---
+name:            pointer_add_unknown_offset
+alignment:       4
+tracksRegLiveness: true
+body:             |
+  ; CHECK-LABEL: name: pointer_add_unknown_offset
+  ; CHECK: bb.0.entry:
+  ; CHECK-NEXT:   liveins: $i0, $i1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   [[COPY:%[0-9]+]]:i64regs = COPY $i1
+  ; CHECK-NEXT:   [[COPY1:%[0-9]+]]:i64regs = COPY $i0
+  ; CHECK-NEXT:   [[SRAri:%[0-9]+]]:i64regs = SRAri [[COPY]], 0
+  ; CHECK-NEXT:   [[SLLXri:%[0-9]+]]:i64regs = SLLXri killed [[SRAri]], 2
+  ; CHECK-NEXT:   DBG_VALUE_LIST !13, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_stack_value), [[COPY1]], [[SLLXri]], debug-location !16
+  ; CHECK-NEXT:   DBG_VALUE_LIST !14, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus, DW_OP_plus_uconst, 3, DW_OP_stack_value), [[COPY1]], [[SLLXri]], debug-location !16
+  ; CHECK-NEXT:   DBG_VALUE_LIST !15, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 2, DW_OP_plus, DW_OP_LLVM_arg, 1, DW_OP_LLVM_arg, 3, DW_OP_plus, DW_OP_plus, DW_OP_stack_value), [[COPY1]], [[COPY1]], [[SLLXri]], [[SLLXri]], debug-location !16
+  ; CHECK-NEXT:   [[ORri:%[0-9]+]]:intregs = ORri $g0, 42
+  ; CHECK-NEXT:   STrr [[COPY1]], killed [[SLLXri]], killed [[ORri]] :: (store (s32) into %ir.add.ptr)
+  ; CHECK-NEXT:   RETL 8

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dstenb]

Fix clang-format issue.

[adrian-prantl]

Is this testing the non-variadic case, too?

[dstenb]

Yes, the debug values for "p" and "q" are still non-variadic when entering SelectionDAG::salvageDebugInfo():

 DbgVal(Order=3)(SDNODE=t9:0):"p"
 DbgVal(Order=3)(SDNODE=t9:0):"q"!DIExpression(DW_OP_plus_uconst, 3, DW_OP_stack_value)

[dstenb]

Thanks for the review! I'll land this shortly.

[dstenb]

This broke expensive-checks buildbots, e.g. https://lab.llvm.org/buildbot/#/builders/16/builds/56317, so I'll revert and investigate.

[dstenb]

Reverted in 6b25890.

[dstenb]

The test case was generated using -stop-before=sparc-isel, which produced MIR output with no body for the function. Running -start-before=sparc-isel on that output (as was done in the test case) resulted in the following verifier error which broke the expensive-checks bots:

*** Bad machine code: Function has NoVRegs property but there are VReg operands ***
- function:    pointer_add_unknown_offset
LLVM ERROR: Found 1 machine code errors.

I have extracted the IR code from the test file, and changed that a .ll test instead, still starting before sparc-isel. I saw that there are other tests starting before isel that looks like that, e.g. llvm/test/CodeGen/AArch64/callbr-asm-outputs-indirect-isel.ll. I verified that this works with update_mir_test_checks.py (the only difference is that the CHECKs are inserted in the IR code instead).

The patch was re-landed with that change in 22f1217.

[KanRobert]

We changed the value of IsVariadic here and getDbgValueList(line 10835) would call constructor of SDDbgValue
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/SDNodeDbgValue.h#L166

it seems the assertion assert(!(IsVariadic && IsIndirect)); in the constructor may fail. In fact, I did see such an error for x86 target.

[KanRobert]

#72630