[BDCE/DebugInfo] Preserve llvm.dbg.value's argument

[davide]

Summary

BDCE has two phases:

1. It asks SimplifyDemandedBits if all the bits of an instruction are dead, and if so, replaces all its uses with the constant zero.
2. Then, it asks SimplifyDemandedBits again if the instruction is really dead (no side effects etc..) and if so, eliminates it.

Now, in 1) if all the bits of an instruction are dead, we may end up replacing a dbg use:

%call = tail call i32 (...) @g() #4, !dbg !15
tail call void @llvm.dbg.value(metadata i32 %call, i64 0, metadata !8, metadata !16), !dbg !17

->

%call = tail call i32 (...) @g() #4, !dbg !15
tail call void @llvm.dbg.value(metadata i32 0, i64 0, metadata !8, metadata !16), !dbg !17

but not eliminating the call because it may have arbitrary side effects.
In other words, we lose some debug informations.

There are various possible solutions to the problem. E.g.:

1. If the instruction has side effects and no non-dbg uses, BDCE should do nothing with it.
2. We could introduce a variant of RAUW, called RAUWExceptDbg that doesn't strip dbg uses.

I tried to implement 2), but things get very ugly very quickly. Therefore, this patch implements 1), after discussing the solution with David/Hal.

[davide]

Fixes https://llvm.org/bugs/show_bug.cgi?id=26587

[aprantl]

test/Transforms/BDCE/pr26587.ll

10 ↗

(On Diff #80444)

Does this also work with a non-asserts opt? I'm not sure if it is clang that is stripping out the values' names or if the parser does that, too.

CHECK-NEXT: %[[CALL:.*]] = tail call i32 (...) @g()

Probably don't need to check for the !dbg !10 here.

[davide]

test/Transforms/BDCE/pr26587.ll

10 ↗

(On Diff #80444)

Done. Confirmed that the test passes also with Release (non assert)

[aprantl]

lib/Transforms/Scalar/BDCE.cpp

45 ↗

(On Diff #80457)

It looks like this alters the behavior of the optimization beyond debug info. It would be great if someone else could sign off on this change.

[davide]

lib/Transforms/Scalar/BDCE.cpp

45 ↗

(On Diff #80457)

I discussed this with David M. and he was OK with this solution. I'm still a little nervous this may break optimizations, so I'll be more confident to get this in if Hal signs it off.

[efriedma]

lib/Transforms/Scalar/BDCE.cpp

44 ↗

(On Diff #80457)

"I.use_empty()" is a bit more straightforward than "!I.hasNUsesOrMore(1)".

45 ↗

(On Diff #80457)

I don't think this alters visible behavior without debug info: if the instruction has no uses, RAUW is a no-op, and if mayHaveSideEffects() is true, isInstructionDead() is false. So this looks fine, I think.

[davide]

lib/Transforms/Scalar/BDCE.cpp

45 ↗

(On Diff #80457)

Thanks for your review, Eli!

[davide]

Address Eli's comment.

[majnemer]

LGTM

llvm/trunk/lib/Transforms/Scalar/BDCE.cpp

 
 STATISTIC(NumRemoved, "Number of instructions removed (unused)");
 STATISTIC(NumSimplified, "Number of instructions trivialized (dead bits)");
 
 static bool bitTrackingDCE(Function &F, DemandedBits &DB) {
   SmallVector<Instruction*, 128> Worklist;
   bool Changed = false;
   for (Instruction &I : instructions(F)) {
+    // If the instruction has side effects and no non-dbg uses,
+    // BDCE should skip it.
+    if (I.mayHaveSideEffects() && I.use_empty())
+      continue;
+
     if (I.getType()->isIntegerTy() &&
         !DB.getDemandedBits(&I).getBoolValue()) {
       // For live instructions that have all dead bits, first make them dead by
       // replacing all uses with something else. Then, if they don't need to
       // remain live (because they have side effects, etc.) we can remove them.
       DEBUG(dbgs() << "BDCE: Trivializing: " << I << " (all bits dead)\n");
       // FIXME: In theory we could substitute undef here instead of zero.
       // This should be reconsidered once we settle on the semantics of

llvm/trunk/test/Transforms/BDCE/pr26587.ll

+; Test that BDCE doesn't destroy llvm.dbg.value's argument.
+; RUN: opt -bdce %s -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: define void @f()
+; CHECK-NEXT: entry:
+; CHECK-NEXT: tail call void (...) @h()
+; CHECK-NEXT: %[[CALL:.*]] = tail call i32 (...) @g()
+; CHECK-NEXT: tail call void @llvm.dbg.value(metadata i32 %[[CALL:.*]]
+
+define void @f() !dbg !6 {
+entry:
+  tail call void (...) @h(), !dbg !9
+  %call = tail call i32 (...) @g(), !dbg !10
+  tail call void @llvm.dbg.value(metadata i32 %call, i64 0, metadata !11, metadata !13), !dbg !14
+  tail call void (...) @h(), !dbg !15
+  ret void, !dbg !16
+}
+
+declare void @h(...)
+declare i32 @g(...)
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0 (trunk 288665) (llvm/trunk 288725)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "patatino.c", directory: "/home/davide/work/llvm/build-clang/bin")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang version 4.0.0 (trunk 288665) (llvm/trunk 288725)"}
+!6 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 3, type: !7, isLocal: false, isDefinition: true, scopeLine: 3, isOptimized: false, unit: !0, variables: !2)
+!7 = !DISubroutineType(types: !8)
+!8 = !{null}
+!9 = !DILocation(line: 4, column: 3, scope: !6)
+!10 = !DILocation(line: 5, column: 11, scope: !6)
+!11 = !DILocalVariable(name: "a", scope: !6, file: !1, line: 5, type: !12)
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = !DIExpression()
+!14 = !DILocation(line: 5, column: 7, scope: !6)
+!15 = !DILocation(line: 6, column: 3, scope: !6)
+!16 = !DILocation(line: 7, column: 1, scope: !6)