[DebugInfo] Corrections for salvageDebugInfo

[bjope]

Summary

When salvaging a dbg.declare/dbg.addr we should not add
DW_OP_stack_value to the DIExpression
(see test/Transforms/InstCombine/salvage-dbg-declare.ll).

Consider this example

%vla = alloca i32, i64 2
call void @llvm.dbg.declare(metadata i32* %vla, metadata !1, metadata !DIExpression())

Instcombine will turn it into

%vla1 = alloca [2 x i32]
%vla1.sub = getelementptr inbounds [2 x i32], [2 x i32]* %vla, i64 0, i64 0
call void @llvm.dbg.declare(metadata [2 x i32]* %vla1.sub, metadata !19, metadata !DIExpression())

If the GEP can be eliminated, then the dbg.declare will be salvaged
and we should get

%vla1 = alloca [2 x i32]
call void @llvm.dbg.declare(metadata [2 x i32]* %vla1, metadata !19, metadata !DIExpression())

The problem was that salvageDebugInfo did not recognize dbg.declare
as being indirect (%vla1 points to the value, it does not hold the
value), so we incorrectly got

call void @llvm.dbg.declare(metadata [2 x i32]* %vla1, metadata !19, metadata !DIExpression(DW_OP_stack_value))

I also made sure that llvm::salvageDebugInfo and
DIExpression::prependOpcodes do not add DW_OP_stack_value to
the DIExpression in case no new operands are added to the
DIExpression. That way we avoid to, unneccessarily, turn a
register location expression into an implicit location expression
in some situations (see test11 in test/Transforms/LICM/sinking.ll).

[aprantl]

Thanks. This is an obvious improvement. It begs the question whether there is a better / more principled condition rather than "the expression is empty" to decide whether the stack_value should be added or not. Let me know if you have any ideas.

[vsk]

Thanks, LGTM, just one inline question.

lib/IR/DebugInfoMetadata.cpp

808 ↗

(On Diff #153736)

Is there a reason to allow callers to convert an expression into a stack value without adding any opcodes? Why not assert this can't happen?

[bjope]

lib/IR/DebugInfoMetadata.cpp

808 ↗

(On Diff #153736)

I was actually thinking about using an assert here. I did "git grep DIExpression::prepend" and found out that some users of DIExpression::prepend/DIExpression::prependOpcodes explicitly avoids calling those function with a zero Offset (or empty Ops vector), but some don't (an example is SelectionDAG::salvageDebugInfo). So I did not dare to add an assert.

[vsk]

lib/IR/DebugInfoMetadata.cpp

808 ↗

(On Diff #153736)

Thanks for pointing out that example. We can revisit tightening assertions in DIExpression::prepend* and see if that helps make it clearer whether or not some call turns an expression into a stack value.

llvm/trunk/lib/IR/DebugInfoMetadata.cpp

if (DerefAfter)
     Ops.push_back(dwarf::DW_OP_deref);
 
   return prependOpcodes(Expr, Ops, StackValue);
 }
 
 DIExpression *DIExpression::prependOpcodes(const DIExpression *Expr,
                                            SmallVectorImpl<uint64_t> &Ops,
                                            bool StackValue) {
+  // If there are no ops to prepend, do not even add the DW_OP_stack_value.
+  if (Ops.empty())
+    StackValue = false;
   if (Expr)
     for (auto Op : Expr->expr_ops()) {
       // A DW_OP_stack_value comes at the end, but before a DW_OP_LLVM_fragment.
       if (StackValue) {
         if (Op.getOp() == dwarf::DW_OP_stack_value)
           StackValue = false;
         else if (Op.getOp() == dwarf::DW_OP_LLVM_fragment) {
           Ops.push_back(dwarf::DW_OP_stack_value);

llvm/trunk/lib/Transforms/Utils/Local.cpp

void llvm::salvageDebugInfo(Instruction &I) {
   auto &DL = M.getDataLayout();
 
   auto wrapMD = [&](Value *V) {
     return MetadataAsValue::get(I.getContext(), ValueAsMetadata::get(V));
   };
 
   auto doSalvage = [&](DbgInfoIntrinsic *DII, SmallVectorImpl<uint64_t> &Ops) {
     auto *DIExpr = DII->getExpression();
-    DIExpr =
-        DIExpression::prependOpcodes(DIExpr, Ops, DIExpression::WithStackValue);
+    if (!Ops.empty()) {
+      // 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 WithStackValue = isa<DbgValueInst>(DII);
+      DIExpr = DIExpression::prependOpcodes(DIExpr, Ops, WithStackValue);
+    }
     DII->setOperand(0, wrapMD(I.getOperand(0)));
     DII->setOperand(2, MetadataAsValue::get(I.getContext(), DIExpr));
     LLVM_DEBUG(dbgs() << "SALVAGE: " << *DII << '\n');
   };
 
   auto applyOffset = [&](DbgInfoIntrinsic *DII, uint64_t Offset) {
     SmallVector<uint64_t, 8> Ops;
     DIExpression::appendOffset(Ops, Offset);

llvm/trunk/test/Transforms/InstCombine/salvage-dbg-declare.ll

+; RUN: opt -instcombine -S -o - %s | FileCheck %s
+
+declare dso_local i32 @bar(i8*)
+
+; Function Attrs: nounwind
+define internal i32 @foo() #0 !dbg !1 {
+; CHECK:  %[[VLA:.*]] = alloca [2 x i32]
+; CHECK:  call void @llvm.dbg.declare(metadata [2 x i32]* %[[VLA]], {{.*}}, metadata !DIExpression())
+
+entry:
+  %vla = alloca i32, i64 2, align 4, !dbg !16
+  call void @llvm.dbg.declare(metadata i32* %vla, metadata !19, metadata !DIExpression()), !dbg !20
+  %0 = bitcast i32* %vla to i8*, !dbg !21
+  %call = call i32 @bar(i8* %0), !dbg !22
+  unreachable
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!5}
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DISubprogram(name: "a", scope: !2, file: !2, line: 232, type: !3, isLocal: true, isDefinition: true, scopeLine: 234, flags: DIFlagPrototyped, isOptimized: true, unit: !5, retainedNodes: !6)
+!2 = !DIFile(filename: "b", directory: "c")
+!3 = !DISubroutineType(types: !4)
+!4 = !{}
+!5 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang version", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, retainedTypes: !4, globals: !4)
+!6 = !{!7, !11}
+!7 = !DILocalVariable(name: "__vla_expr", scope: !8, type: !10, flags: DIFlagArtificial)
+!8 = distinct !DILexicalBlock(scope: !9, file: !2, line: 238, column: 39)
+!9 = distinct !DILexicalBlock(scope: !1, file: !2, line: 238, column: 6)
+!10 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "ptr32", scope: !8, file: !2, line: 240, type: !12)
+!12 = !DICompositeType(tag: DW_TAG_array_type, baseType: !13, elements: !14)
+!13 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!14 = !{!15}
+!15 = !DISubrange(count: !7)
+!16 = !DILocation(line: 240, column: 3, scope: !17)
+!17 = distinct !DILexicalBlock(scope: !18, file: !2, line: 238, column: 39)
+!18 = distinct !DILexicalBlock(scope: !1, file: !2, line: 238, column: 6)
+!19 = !DILocalVariable(name: "ptr32", scope: !17, file: !2, line: 240, type: !12)
+!20 = !DILocation(line: 240, column: 12, scope: !17)
+!21 = !DILocation(line: 241, column: 65, scope: !17)
+!22 = !DILocation(line: 241, column: 11, scope: !17)

llvm/trunk/test/Transforms/LICM/sinking.ll

 ; CHECK-NEXT:  %tmp.6.le = sdiv i32 %N, %[[LCSSAPHI]]
 ; CHECK-NEXT:  ret i32 %tmp.6.le
 }
 
 ; Should delete, not sink, dead instructions.
 define void @test11() {
   br label %Loop
 Loop:
-  %dead = getelementptr %Ty, %Ty* @X2, i64 0, i32 0
+  %dead1 = getelementptr %Ty, %Ty* @X2, i64 0, i32 0
+  %dead2 = getelementptr %Ty, %Ty* @X2, i64 0, i32 1
   br i1 false, label %Loop, label %Out
 Out:
   ret void
 ; CHECK-LABEL: @test11(
 ; CHECK:     Out:
 ; CHECK-NEXT:  ret void
 
+; The GEP in dead1 is adding a zero offset, so the DIExpression can be kept as
+; a "register location".
+; The GEP in dead2 is adding a 4 bytes to the pointer, so the DIExpression is
+; turned into an "implicit location" using DW_OP_stack_value.
+;
 ; DEBUGIFY-LABEL: @test11(
-; DEBUGIFY: call void @llvm.dbg.value(metadata %Ty* @X2, metadata {{.*}}, metadata !DIExpression(DW_OP_stack_value))
+; DEBUGIFY: call void @llvm.dbg.value(metadata %Ty* @X2, metadata {{.*}}, metadata !DIExpression())
+; DEBUGIFY: call void @llvm.dbg.value(metadata %Ty* @X2, metadata {{.*}}, metadata !DIExpression(DW_OP_plus_uconst, 4, DW_OP_stack_value))
 }
 
 @c = common global [1 x i32] zeroinitializer, align 4
 
 ; Test a *many* way nested loop with multiple exit blocks both of which exit
 ; multiple loop nests. This exercises LCSSA corner cases.
 define i32 @PR18753(i1* %a, i1* %b, i1* %c, i1* %d) {
 entry: