[BasicBlockUtils] Fix dbg.value elimination problem in MergeBlockIntoPredecessor

[bjope]

Summary

In commit d60f34c20a2f31335c8d5626e (llvm-svn 317128,
PR35113) MergeBlockIntoPredecessor was changed into
discarding some dbg.value intrinsics referring to
PHI values, post-splice due to loop rotation.

That elimination of dbg.value intrinsics did not
consider which dbg.value to keep depending on the
context (e.g. if the variable is changing it's value
several times inside the basic block).

In the past that hasn't been such a big problem since
CodeGenPrepare::placeDbgValues has moved the dbg.value
to be next to the PHI node anyway. But after commit
00e238896cd8ad3a7d7 CodeGenPrepare isn't doing that
any longer, so we need to be more careful when avoiding
duplicate dbg.value intrinsics in MergeBlockIntoPredecessor.

This patch replaces the code that tried to avoid duplicate
dbg.values by using the RemoveRedundantDbgInstrs helper.

[bjope]

llvm/test/DebugInfo/NVPTX/debug-loc-offset.ll

47

It should be noticed that this mapping of baz:z to undef was present in the baseline (but not covered by the checks). Now I verify that we get a single DEBUG_VALUE for baz:z here, and that happens to be the last one (i.e. undef).

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll

11

Both of these had same inlineAt, so the first one was redundant. Not sure if this makes the test case no longer verifying what it was supposed to verify.

[aprantl]

This patch looks obviously good because of a magic new RemoveRedundantDbgInstrs() function :-)

[aprantl]

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll

12

We might want to just delete the test now.

[vsk]

LGTM as well.

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

288

nit, since

[bjope]

Fix typo in code comments.

[bjope]

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll

12

I looked a bit more at this. The test case does not seem totally useless. I think the original problem still could be detected. And now we also verify that we remove the redundant dbg.value for this kind of CFG simplification.

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

if (PredecessorWithTwoSuccessors) {
     if (MSSAU)
       if (MemoryUseOrDef *MUD = cast_or_null<MemoryUseOrDef>(
               MSSAU->getMemorySSA()->getMemoryAccess(PredBB->getTerminator())))
         MSSAU->moveToPlace(MUD, PredBB, MemorySSA::End);
   }
   // Add unreachable to now empty BB.
   new UnreachableInst(BB->getContext(), BB);
 
-  // Eliminate duplicate dbg.values describing the entry PHI node post-splice.
-  for (auto Incoming : IncomingValues) {
-    if (isa<Instruction>(*Incoming)) {
-      SmallVector<DbgValueInst *, 2> DbgValues;
-      SmallDenseSet<std::pair<DILocalVariable *, DIExpression *>, 2>
-          DbgValueSet;
-      llvm::findDbgValues(DbgValues, Incoming);
-      for (auto &DVI : DbgValues) {
-        auto R = DbgValueSet.insert({DVI->getVariable(), DVI->getExpression()});
-        if (!R.second)
-          DVI->eraseFromParent();
-      }
-    }
-  }
+  // Eliminate duplicate/redundant dbg.values. This seems to be a good place to
+  // do that since we might end up with redundant dbg.values describing the
+  // entry PHI node post-splice.
+  RemoveRedundantDbgInstrs(PredBB);
 
   // Inherit predecessors name if it exists.
   if (!PredBB->hasName())
     PredBB->takeName(BB);
 
   if (LI)
     LI->removeBlock(BB);
 

llvm/test/DebugInfo/NVPTX/debug-loc-offset.ll

 
 declare void @llvm.dbg.value(metadata, metadata, metadata) #1
 
 ; CHECK: .visible .func _Z3baz1A(
 ; CHECK: {
 ; CHECK: .loc [[CU2:[0-9]+]] 6 0
 ; CHECK: Lfunc_begin1:
 ; CHECK: .loc [[CU2]] 6 0
-; CHECK: //DEBUG_VALUE: baz:z <- {{[0-9]+}}
-; CHECK: //DEBUG_VALUE: baz:z <- {{[0-9]+}}
+; CHECK-NOT: //DEBUG_VALUE: baz:z
+; CHECK: //DEBUG_VALUE: baz:z <- undef
 ; CHECK: .loc [[CU2]] 10 0
 ; CHECK: ret;
 ; CHECK: }
 
 define void @_Z3baz1A(%struct.A* %a) #2 !dbg !14 {
 entry:
   %z = alloca i32, align 4
   call void @llvm.dbg.declare(metadata %struct.A* %a, metadata !24, metadata !DIExpression(DW_OP_deref)), !dbg !25

llvm/test/Transforms/LoopRotate/dbg-value-duplicates-2.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -loop-rotate -S | FileCheck %s
 
 define dso_local i16 @main() local_unnamed_addr #0 !dbg !7 {
 ; CHECK-LABEL: @main(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    call void @llvm.dbg.value(metadata i16 9, metadata !12, metadata !DIExpression()), !dbg !13
 ; CHECK-NEXT:    br label [[BB2:%.*]], !dbg !14
 ; CHECK:       bb2:
 ; CHECK-NEXT:    [[TMP1:%.*]] = phi i16 [ 9, [[ENTRY:%.*]] ], [ [[TMP5:%.*]], [[BB2]] ]
 ; CHECK-NEXT:    call void @llvm.dbg.value(metadata i16 [[TMP1]], metadata !12, metadata !DIExpression()), !dbg !13
 ; CHECK-NEXT:    [[TMP4:%.*]] = call i16 @wibble(i16 [[TMP1]]), !dbg !14
 ; CHECK-NEXT:    [[TMP5]] = add nsw i16 [[TMP4]], [[TMP1]], !dbg !14
+; CHECK-NEXT:    call void @llvm.dbg.value(metadata i16 [[TMP5]], metadata !12, metadata !DIExpression()), !dbg !13
 ; CHECK-NEXT:    [[TMP6:%.*]] = call i16 @wibble(i16 [[TMP4]]), !dbg !14
 ; CHECK-NEXT:    [[TMP7:%.*]] = mul nsw i16 [[TMP6]], 3, !dbg !14
 ; CHECK-NEXT:    [[TMP8:%.*]] = call i16 @wibble(i16 [[TMP7]]), !dbg !14
-; BUG: This dbg.value is expected to be placed directly after the add above.
-;      Otherwise variable "x" (!12) won't be described as having the correct value at the call to wibble just after the add.
-; CHECK-NEXT:    call void @llvm.dbg.value(metadata i16 [[TMP5]], metadata !12, metadata !DIExpression()), !dbg !13
 ; CHECK-NEXT:    [[TMP2:%.*]] = icmp slt i16 [[TMP5]], 17, !dbg !14
 ; CHECK-NEXT:    br i1 [[TMP2]], label [[BB2]], label [[BB3:%.*]], !dbg !14
 ; CHECK:       bb3:
 ; CHECK-NEXT:    [[TMP10:%.*]] = call i16 @wibble(i16 8), !dbg !14
 ; CHECK-NEXT:    ret i16 [[TMP10]], !dbg !14
 ;
 entry:
   call void @llvm.dbg.value(metadata i16 9, metadata !12, metadata !DIExpression()), !dbg !13

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll

 ; RUN: opt -simplifycfg -S < %s | FileCheck %s
 ; Verify that we don't crash due an invalid !dbg location on the hoisted llvm.dbg.value
 
 define i64 @caller(i64* %ptr, i64 %flag) !dbg !10 {
 init:
   %v9 = icmp eq i64 %flag, 0
   br i1 %v9, label %a, label %b
 
 ; CHECK:  %vala = load i64, i64* %ptr
 ; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %vala, metadata [[MD:![0-9]*]]
-; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %vala, metadata [[MD]]
 ; CHECK-NEXT:  %valbmasked = and i64 %vala, 1
 
 a:                                              ; preds = %init
   %vala = load i64, i64* %ptr, align 8
   call void @llvm.dbg.value(metadata i64 %vala, metadata !8, metadata !DIExpression()), !dbg !12
   br label %test.exit
 
 b:                                              ; preds = %init
   %valb = load i64, i64* %ptr, align 8