[DebugInfo] Sink related dbg users when sinking in InstCombine

[bjope]

Summary

When sinking an instruction in InstCombine we now also sink
the DbgInfoIntrinsics that are using the sunken value.

Example)

When sinking the load in this input

bb.X:
  %0 = load i64, i64* %start, align 4, !dbg !31
  tail call void @llvm.dbg.value(metadata i64 %0, ...)
  br i1 %cond, label %for.end, label %for.body.lr.ph
for.body.lr.ph:
  br label %for.body

we now also move the dbg.value, like this

bb.X:
  br i1 %cond, label %for.end, label %for.body.lr.ph
for.body.lr.ph:
  %0 = load i64, i64* %start, align 4, !dbg !31
  tail call void @llvm.dbg.value(metadata i64 %0, ...)
  br label %for.body

In the past we haven't moved the dbg.value so we got

bb.X:
  tail call void @llvm.dbg.value(metadata i64 %0, ...)
  br i1 %cond, label %for.end, label %for.body.lr.ph
for.body.lr.ph:
  %0 = load i64, i64* %start, align 4, !dbg !31
  br label %for.body

So in the past we got a debug-use before the def of %0.
And that dbg.value was also on the path jumping to %for.end, for
which %0 never was defined.

CodeGenPrepare normally comes to rescue later (when not moving
the dbg.value), since it moves dbg.value instrinsics quite
brutally, without really analysing if it is correct to move
the intrinsic (see PR31878).
So at the moment this patch isn't expected to have much impact,
besides that it is moving the dbg.value already in opt, making
the IR look more sane directly.

This can be seen as a preparation to (hopefully) make it possible
to turn off CodeGenPrepare::placeDbgValues later as a solution
to PR31878.

I also adjusted test/DebugInfo/X86/sdagsplit-1.ll to make the
IR in the test case up-to-date with this behavior in InstCombine.

[aprantl]

As long as the sunken load retains the same DILocation sinking the dbg.value with it should be correct.

Thanks!

[vsk]

Thanks, lgtm.

One possible follow-up would be to check for debug info use-before-defs in the verifier, if we aren't already.

[bjope]

In D45425#1069936, @vsk wrote:

Thanks, lgtm.

One possible follow-up would be to check for debug info use-before-defs in the verifier, if we aren't already.

The verifier does not catch that today.
There is even a test case verifying that it is OK to have use-before-def, test/Transforms/Inline/local-as-metadata-undominated-use.ll, and it has a comment like this:

; Make sure the inliner doesn't crash when a metadata-bridged SSA operand is an
; undominated use.
;
; If we ever add a verifier check to prevent the scenario in this file, it's
; fine to delete this testcase.  However, we would need a bitcode upgrade since
; such historical IR exists in practice.

There is also a PR27273 (indicating that there have been some attempts in the past by @dexonsmith to enable this kind of verification): https://bugs.llvm.org/show_bug.cgi?id=27273

So I don't know how big the problem is. I think that it would be nice to at least have an option to turn on the check in the verifier, to make it easier to track down this kind of problems.

llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp

 }
 
 /// Try to move the specified instruction from its current block into the
 /// beginning of DestBlock, which can only happen if it's safe to move the
 /// instruction past all of the instructions between it and the end of its
 /// block.
 static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
   assert(I->hasOneUse() && "Invariants didn't hold!");
+  BasicBlock *SrcBlock = I->getParent();
 
   // Cannot move control-flow-involving, volatile loads, vaarg, etc.
   if (isa<PHINode>(I) || I->isEHPad() || I->mayHaveSideEffects() ||
       isa<TerminatorInst>(I))
     return false;
 
   // Do not sink alloca instructions out of the entry block.
   if (isa<AllocaInst>(I) && I->getParent() ==
static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
   // the end of block that could change the value.
   if (I->mayReadFromMemory()) {
     for (BasicBlock::iterator Scan = I->getIterator(),
                               E = I->getParent()->end();
          Scan != E; ++Scan)
       if (Scan->mayWriteToMemory())
         return false;
   }
-
   BasicBlock::iterator InsertPos = DestBlock->getFirstInsertionPt();
   I->moveBefore(&*InsertPos);
   ++NumSunkInst;
+
+  // Also sink all related debug uses from the source basic block. Otherwise we
+  // get debug use before the def.
+  SmallVector<DbgInfoIntrinsic *, 1> DbgUsers;
+  findDbgUsers(DbgUsers, I);
+  for (auto *DII : DbgUsers) {
+    if (DII->getParent() == SrcBlock) {
+      DII->moveBefore(&*InsertPos);
+      DEBUG(dbgs() << "SINK: " << *DII << '\n');
+    }
+  }
   return true;
 }
 
 bool InstCombiner::run() {
   while (!Worklist.isEmpty()) {
     Instruction *I = Worklist.RemoveOne();
     if (I == nullptr) continue;  // skip null values.
 

llvm/trunk/test/DebugInfo/X86/sdagsplit-1.ll

 target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
 target triple = "i386"
 
 ; Function Attrs: nounwind readnone
 define i64 @foo(i64 %a, i64 %b) local_unnamed_addr #0 !dbg !8 {
 entry:
   tail call void @llvm.dbg.value(metadata i64 %a, metadata !13, metadata !16), !dbg !17
   tail call void @llvm.dbg.value(metadata i64 %b, metadata !14, metadata !16), !dbg !18
-  tail call void @llvm.dbg.value(metadata i64 %add, metadata !15, metadata !16), !dbg !19
   %cmp = icmp eq i64 %a, %b, !dbg !20
   %add = add nsw i64 %b, 1, !dbg !22
+  tail call void @llvm.dbg.value(metadata i64 %add, metadata !15, metadata !16), !dbg !19
   %retval.0 = select i1 %cmp, i64 %add, i64 0, !dbg !23
   ret i64 %retval.0, !dbg !24
 }
 
 ; Function Attrs: nounwind readnone speculatable
 declare void @llvm.dbg.value(metadata, metadata, metadata) #1
 
 attributes #0 = { nounwind readnone }

llvm/trunk/test/Transforms/InstCombine/debuginfo_add.ll

 define void @f(%struct.vm_object* %object, i64* nocapture readonly %start) local_unnamed_addr #0 !dbg !11 {
 entry:
   tail call void @llvm.dbg.value(metadata %struct.vm_object* %object, metadata !21, metadata !DIExpression()), !dbg !27
   tail call void @llvm.dbg.value(metadata i64* %start, metadata !22, metadata !DIExpression()), !dbg !28
   %0 = load i64, i64* %start, align 4, !dbg !29
   tail call void @llvm.dbg.value(metadata i64 %0, metadata !25, metadata !DIExpression()), !dbg !30
   %offset.08 = add i64 %0, -4096
   tail call void @llvm.dbg.value(metadata i64 %offset.08, metadata !26, metadata !DIExpression()), !dbg !31
-  ; CHECK: call void @llvm.dbg.value(metadata i64 %0, metadata !26, metadata !DIExpression(DW_OP_constu, 4096, DW_OP_minus, DW_OP_stack_value)), !dbg !30
   tail call void @llvm.dbg.value(metadata i32 undef, metadata !23, metadata !DIExpression()), !dbg !32
   br i1 undef, label %for.end, label %for.body.lr.ph, !dbg !32
 
 for.body.lr.ph:                                   ; preds = %entry
+  ; The 'load' and the 'add' are sunken to this basic block. So let's verify that the related dbg.values are sunken as well.
+  ; The add is later eliminated, so we verify that the dbg.value is salvaged by using DW_OP_minus.
+  ; CHECK-LABEL: for.body.lr.ph:
+  ; CHECK-NEXT: %0 = load
+  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i64 %0, metadata !25, metadata !DIExpression()), !dbg !
+  ; CHECK-NEXT: call void @llvm.dbg.value(metadata i64 %0, metadata !26, metadata !DIExpression(DW_OP_constu, 4096, DW_OP_minus, DW_OP_stack_value)), !dbg !
   br label %for.body, !dbg !32
 
 for.body:                                         ; preds = %for.body.lr.ph, %for.body
+  ; CHECK-LABEL: for.body:
   %offset.010 = phi i64 [ %offset.08, %for.body.lr.ph ], [ %offset.0, %for.body ]
   %head_size.09 = phi i32 [ undef, %for.body.lr.ph ], [ %sub2, %for.body ]
   tail call void @llvm.dbg.value(metadata i32 %head_size.09, metadata !23, metadata !DIExpression()), !dbg !31
   %call = tail call i32 bitcast (i32 (...)* @use to i32 (i64, %struct.vm_object*)*)(i64 %offset.010, %struct.vm_object* %object) #3, !dbg !34
   %sub2 = add i32 %head_size.09, -4096, !dbg !37
   %offset.0 = add i64 %offset.010, -4096
   tail call void @llvm.dbg.value(metadata i64 %offset.0, metadata !26, metadata !DIExpression()), !dbg !30
-  ; CHECK: call void @llvm.dbg.value(metadata i64 %offset.010, metadata !26, metadata !DIExpression(DW_OP_constu, 4096, DW_OP_minus, DW_OP_stack_value)), !dbg !29
+  ; CHECK: call void @llvm.dbg.value(metadata i64 %offset.010, metadata !26, metadata !DIExpression(DW_OP_constu, 4096, DW_OP_minus, DW_OP_stack_value)), !dbg !
   tail call void @llvm.dbg.value(metadata i32 %sub2, metadata !23, metadata !DIExpression()), !dbg !31
   %tobool = icmp eq i32 %sub2, 0, !dbg !32
   br i1 %tobool, label %for.end, label %for.body, !dbg !32, !llvm.loop !38
 
 for.end:                                          ; preds = %for.body, %entry
   ret void, !dbg !40
 }