[Debugify] Move debug value intrinsics closer to their operand defs

Before this patch, debugify would insert debug value intrinsics before the
terminating instruction in a block. This had the advantage of being simple,
but was a bit too simple/unrealistic.

This patch teaches debugify to insert debug values immediately after their
operand defs. This enables better testing of the compiler.

For example, with this patch, `opt -debugify-each` is able to identify a
vectorizer DI-invariance bug fixed in llvm.org/PR32761. In this bug, the
vectorizer produced different output with/without debug info present.

Reverting Davide's bugfix locally, I see:

$ ~/scripts/opt-check-dbg-invar.sh ./bin/opt \
  .../SLPVectorizer/AArch64/spillcost-di.ll -slp-vectorizer
Comparing: -slp-vectorizer .../SLPVectorizer/AArch64/spillcost-di.ll
  Baseline: /var/folders/j8/t4w0bp8j6x1g6fpghkcb4sjm0000gp/T/tmp.iYYeL1kf
  With DI : /var/folders/j8/t4w0bp8j6x1g6fpghkcb4sjm0000gp/T/tmp.sQtQSeet
9,11c9,11
<   %5 = getelementptr inbounds %0, %0* %2, i64 %0, i32 1
<   %6 = bitcast i64* %4 to <2 x i64>*
<   %7 = load <2 x i64>, <2 x i64>* %6, align 8, !tbaa !0
---
>   %5 = load i64, i64* %4, align 8, !tbaa !0
>   %6 = getelementptr inbounds %0, %0* %2, i64 %0, i32 1
>   %7 = load i64, i64* %6, align 8, !tbaa !5
12a13
>   store i64 %5, i64* %8, align 8, !tbaa !0
14,15c15
<   %10 = bitcast i64* %8 to <2 x i64>*
<   store <2 x i64> %7, <2 x i64>* %10, align 8, !tbaa !0
---
>   store i64 %7, i64* %9, align 8, !tbaa !5
:: Found a test case ^

Running this over the *.ll files in tree, I found four additional examples
which compile differently with/without DI present. I plan on filing bugs for
these.

llvm-svn: 334118

diff --git a/llvm/test/Transforms/DeadStoreElimination/debuginfo.ll b/llvm/test/Transforms/DeadStoreElimination/debuginfo.ll
index 813c042..2d9b4fe 100644 (file)
--- a/llvm/test/Transforms/DeadStoreElimination/debuginfo.ll
+++ b/llvm/test/Transforms/DeadStoreElimination/debuginfo.ll
@@ -7,17 +7,17 @@ declare noalias i8* @malloc(i32)
 declare void @test_f()
 
 define i32* @test_salvage() {
+; Check that all four original local variables have their values preserved.
 ; CHECK-LABEL: @test_salvage()
 ; CHECK-NEXT: malloc
-; CHECK-NEXT: bitcast
-; CHECK-NEXT: call void @test_f()
-; CHECK-NEXT: store i32 0, i32* %P
-
-; Check that all four original local variables have their values preserved.
 ; CHECK-NEXT: @llvm.dbg.value(metadata i8* %p, metadata ![[p:.*]], metadata !DIExpression())
+; CHECK-NEXT: bitcast
 ; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[P:.*]], metadata !DIExpression())
 ; CHECK-NEXT: @llvm.dbg.value(metadata i32* %P, metadata ![[DEAD:.*]], metadata !DIExpression(DW_OP_deref))
 ; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %P, metadata ![[DEAD2:.*]], metadata !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 1, DW_OP_stack_value))
+; CHECK-NEXT: call void @test_f()
+; CHECK-NEXT: store i32 0, i32* %P
+
   %p = tail call i8* @malloc(i32 4)
   %P = bitcast i8* %p to i32*
   %DEAD = load i32, i32* %P

diff --git a/llvm/test/Transforms/GVN/PRE/phi-translate-2.ll b/llvm/test/Transforms/GVN/PRE/phi-translate-2.ll
index 2edc2c1..7911b52 100644 (file)
--- a/llvm/test/Transforms/GVN/PRE/phi-translate-2.ll
+++ b/llvm/test/Transforms/GVN/PRE/phi-translate-2.ll
@@ -141,9 +141,9 @@ critedge.loopexit:
 ; CHECK: br label %if.end3
 ; CHECK: if.end3:
 ; CHECK: %[[PREPHI:.*]] = phi i64 [ %sub.ptr.sub, %if.else ], [ %[[SUB]], %if.then2 ], [ %sub.ptr.sub, %entry ]
-; CHECK: %[[DIV:.*]] = ashr exact i64 %[[PREPHI]], 2
 ; CHECK: call void @llvm.dbg.value(metadata i32* %p.0, metadata [[var_p0:![0-9]+]], metadata !DIExpression())
 ; CHECK: call void @llvm.dbg.value(metadata i64 %sub.ptr.rhs.cast5.pre-phi, metadata [[var_sub_ptr:![0-9]+]], metadata !DIExpression())
+; CHECK: %[[DIV:.*]] = ashr exact i64 %[[PREPHI]], 2
 ; CHECK: ret i64 %[[DIV]]
 
 declare void @bar(...) local_unnamed_addr #1

diff --git a/llvm/test/Transforms/GVN/fence.ll b/llvm/test/Transforms/GVN/fence.ll
index 273c3b6..ab8e2bb 100644 (file)
--- a/llvm/test/Transforms/GVN/fence.ll
+++ b/llvm/test/Transforms/GVN/fence.ll
@@ -18,10 +18,10 @@ define i32 @test(i32* %addr.i) {
 ; Same as above
 define i32 @test2(i32* %addr.i) {
 ; CHECK-LABEL: @test2
-; CHECK-NEXT: fence
 ; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a:![0-9]+]], metadata !DIExpression(DW_OP_deref))
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a2:![0-9]+]], metadata !DIExpression(DW_OP_deref))
+; CHECK-NEXT: fence
 ; CHECK-NOT: load
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32* %addr.i, metadata [[var_a2:![0-9]+]], metadata !DIExpression(DW_OP_deref))
 ; CHECK: ret
   %a = load i32, i32* %addr.i, align 4
   fence release

diff --git a/llvm/tools/opt/Debugify.cpp b/llvm/tools/opt/Debugify.cpp
index 5ed3de3..32e0023 100644 (file)
--- a/llvm/tools/opt/Debugify.cpp
+++ b/llvm/tools/opt/Debugify.cpp
@@ -44,12 +44,11 @@ bool isFunctionSkipped(Function &F) {
   return F.isDeclaration() || !F.hasExactDefinition();
 }
 
-/// Find a suitable insertion point for debug values intrinsics.
+/// Find the basic block's terminating instruction.
 ///
-/// These must be inserted before the terminator. Special care is needed to
-/// handle musttail and deopt calls, as these behave like (but are in fact not)
-/// terminators.
-Instruction *findDebugValueInsertionPoint(BasicBlock &BB) {
+/// Special care is needed to handle musttail and deopt calls, as these behave
+/// like (but are in fact not) terminators.
+Instruction *findTerminatingInstruction(BasicBlock &BB) {
   if (auto *I = BB.getTerminatingMustTailCall())
     return I;
   if (auto *I = BB.getTerminatingDeoptimizeCall())
@@ -109,28 +108,35 @@ bool applyDebugifyMetadata(Module &M,
       if (BB.isEHPad())
         continue;
 
-      Instruction *LastInst = findDebugValueInsertionPoint(BB);
+      // Find the terminating instruction, after which no debug values are
+      // attached.
+      Instruction *LastInst = findTerminatingInstruction(BB);
+      assert(LastInst && "Expected basic block with a terminator");
 
-      // Attach debug values.
-      for (auto It = BB.begin(), End = LastInst->getIterator(); It != End;
-           ++It) {
-        Instruction &I = *It;
+      // Maintain an insertion point which can't be invalidated when updates
+      // are made.
+      BasicBlock::iterator InsertPt = BB.getFirstInsertionPt();
+      assert(InsertPt != BB.end() && "Expected to find an insertion point");
+      Instruction *InsertBefore = &*InsertPt;
 
+      // Attach debug values.
+      for (Instruction *I = &*BB.begin(); I != LastInst; I = I->getNextNode()) {
         // Skip void-valued instructions.
-        if (I.getType()->isVoidTy())
+        if (I->getType()->isVoidTy())
           continue;
 
-        // Skip any just-inserted intrinsics.
-        if (isa<DbgValueInst>(&I))
-          break;
+        // Phis and EH pads must be grouped at the beginning of the block.
+        // Only advance the insertion point when we finish visiting these.
+        if (!isa<PHINode>(I) && !I->isEHPad())
+          InsertBefore = I->getNextNode();
 
         std::string Name = utostr(NextVar++);
-        const DILocation *Loc = I.getDebugLoc().get();
+        const DILocation *Loc = I->getDebugLoc().get();
         auto LocalVar = DIB.createAutoVariable(SP, Name, File, Loc->getLine(),
-                                               getCachedDIType(I.getType()),
+                                               getCachedDIType(I->getType()),
                                                /*AlwaysPreserve=*/true);
-        DIB.insertDbgValueIntrinsic(&I, LocalVar, DIB.createExpression(), Loc,
-                                    LastInst);
+        DIB.insertDbgValueIntrinsic(I, LocalVar, DIB.createExpression(), Loc,
+                                    InsertBefore);
       }
     }
     DIB.finalizeSubprogram(SP);