[InstCombine] Drop debug loc in TryToSinkInstruction

[vsk]

Summary

The advice in HowToUpdateDebugInfo.rst is to "... preserve the debug
location of an instruction if the instruction either remains in its
basic block, or if its basic block is folded into a predecessor that
branches unconditionally".

TryToSinkInstruction doesn't seem to satisfy the criteria as it's
sinking an instruction to some successor block. Preserving the debug loc
can make single-stepping appear to go backwards, or make a breakpoint
hit on that location happen "too late" (since single-stepping from that
breakpoint can cause the function to return unexpectedly).

So, drop the debug location.

[aprantl]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3358

Should we add anchors to the rst document and refer to the rule by anchor in the comment?

[vsk]

Thanks!

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

3358

We tend not to embed links pointing within .rst docs in llvm/lib (I think with good reason, as the source code and the docs can easily get out of sync), but I think it's a good idea to add a pointer to the doc. I'll do this before landing the patch.

[tbosch]

Hello Vedant,
this broke our internal llvm bootstrap with errors like this:

!dbg attachment points at wrong subprogram for function
!805 = distinct !DISubprogram(name: "terminate", linkageName: "_ZSt9terminatev", scope: !9, file: !798, line: 74, type: !69, scopeLine: 75, flags: DIFlagPrototyped | DIFlagNoReturn | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !797, retainedNodes: !806)
void ()* @_ZSt9terminatev
  call void @llvm.dbg.value(metadata %"struct.__cxxabiv1::__cxa_exception"* %11, metadata !2822, metadata !DIExpression(DW_OP_plus_uconst, 96, DW_OP_stack_value)), !dbg !40889
!40889 = !DILocation(line: 0, scope: !2823)
!2823 = distinct !DISubprogram(name: "__isOurExceptionClass", linkageName: "_ZN10__cxxabiv121__isOurExceptionClassEPK17_Unwind_Exception", scope: !52, file: !2288, line: 105, type: !2824, scopeLine: 105, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2287, retainedNodes: !2828)
!2823 = distinct !DISubprogram(name: "__isOurExceptionClass", linkageName: "_ZN10__cxxabiv121__isOurExceptionClassEPK17_Unwind_Exception", scope: !52, file: !2288, line: 105, type: !2824, scopeLine: 105, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2287, retainedNodes: !2828)
conflicting debug info for argument
  call void @llvm.dbg.value(metadata %struct._Unwind_Exception* %160, metadata !2822, metadata !DIExpression()), !dbg !40889
!42078 = !DILocalVariable(name: "arg", arg: 1, scope: !42076, file: !2499, line: 1198, type: !55)
!2822 = !DILocalVariable(name: "unwind_exception", arg: 1, scope: !2823, file: !2288, line: 105, type: !2826)
conflicting debug info for argument
  call void @llvm.dbg.value(metadata %struct._Unwind_Exception* %160, metadata !2831, metadata !DIExpression()), !dbg !42267
!2822 = !DILocalVariable(name: "unwind_exception", arg: 1, scope: !2823, file: !2288, line: 105, type: !2826)
!2831 = !DILocalVariable(name: "unwind_exception", arg: 1, scope: !2832, file: !2288, line: 95, type: !2826)
LLVM ERROR: Broken module found, compilation aborted!

I wonder whether some of the builders that do a bootstrap see this too.

Is this enough for you to know what is going on?

[vsk]

@tbosch I've reverted this commit in ee3620643dfc88a178fa4ca116cf83014e4ee547 as it was causing another issue (errors about inlinable instructions without debug locations). After fixing that problem, I could not reproduce the error you reported in a stage2 clang build.

[tbosch]

Thanks!

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
   });
   /// FIXME: We could remove droppable uses that are not dominated by
   /// the new position.
 
   BasicBlock::iterator InsertPos = DestBlock->getFirstInsertionPt();
   I->moveBefore(&*InsertPos);
   ++NumSunkInst;
 
+  // Drop the debug loc. This prevents single-stepping from going backwards.
+  // See HowToUpdateDebugInfo.rst for the full rationale.
+  I->setDebugLoc(DebugLoc());
+
   // Also sink all related debug uses from the source basic block. Otherwise we
   // get debug use before the def. Attempt to salvage debug uses first, to
   // maximise the range variables have location for. If we cannot salvage, then
   // mark the location undef: we know it was supposed to receive a new location
   // here, but that computation has been sunk.
   SmallVector<DbgVariableIntrinsic *, 2> DbgUsers;
   findDbgUsers(DbgUsers, I);
 

llvm/test/Transforms/InstCombine/sink_to_unreachable_dbg.ll

+; RUN: opt -instcombine -S < %s | FileCheck %s
+
+; CHECK-LABEL: @test(
+; CHECK: [[phi:%.*]] = phi i32
+; CHECK-NEXT: [[add:%.*]] = add i32 %x, 1{{$}}
+; CHECK-NEXT: add i32 [[phi]], [[add]], !dbg
+define i32 @test(i32 %x, i1 %c) !dbg !6 {
+bb0:
+  %a = add i32 %x, 1, !dbg !8
+  br i1 %c, label %bb1, label %bb2, !dbg !9
+
+bb1:                                              ; preds = %bb0
+  br label %bb3, !dbg !10
+
+bb2:                                              ; preds = %bb0
+  br label %bb3, !dbg !11
+
+bb3:                                              ; preds = %bb2, %bb1
+  %p = phi i32 [ 0, %bb1 ], [ 1, %bb2 ], !dbg !12
+  %r = add i32 %p, %a, !dbg !13
+  ret i32 %r, !dbg !14
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !4}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "sink_to_unreachable_dbg.ll", directory: "/")
+!2 = !{}
+!3 = !{i32 7}
+!4 = !{i32 0}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "test", linkageName: "test", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!7 = !DISubroutineType(types: !2)
+!8 = !DILocation(line: 1, column: 1, scope: !6)
+!9 = !DILocation(line: 2, column: 1, scope: !6)
+!10 = !DILocation(line: 3, column: 2, scope: !6)
+!11 = !DILocation(line: 4, column: 3, scope: !6)
+!12 = !DILocation(line: 5, column: 4, scope: !6)
+!13 = !DILocation(line: 6, column: 4, scope: !6)
+!14 = !DILocation(line: 7, column: 4, scope: !6)