[DebugInfo] Create merged locations for instructions other than calls

[vsk]

Summary

This lifts a restriction on DILocation::getMergedLocation(), allowing it
to create merged locations for instructions other than calls.

Instruction::applyMergedLocation() now defaults to creating merged
locations for all instructions.

The default behavior of getMergedLocation() is unchanged: callers which
invoke it directly are unaffected.

This change will enable a follow-up Mem2Reg fix which improves crash
reporting.

[dblaikie]

Do you have the stats on this change in terms of line table size increase?

Which callers use Instruction::applyMergedLocation versus DILocation::getMergedLocation directly?

lib/IR/DebugInfo.cpp

678–679

Maybe have the API use an enum instead of a bool (so callers pass the enum & are inherently readable), rather than relying on callers to name the parameter for readability?

[rnk]

lib/IR/DebugInfoMetadata.cpp

73

Who are the primary callers that pass false here? mem2reg, I guess? If the bool is short-lived, sounds good.

[vsk]

In D45396#1062505, @dblaikie wrote:

Do you have the stats on this change in terms of line table size increase?

I looked at a stage2 Release build of clang (-O3 -g), and there's about a 0.6% size increase:

$ size -m ~/Desktop/baseline.clang.dSYM/Contents/Resources/DWARF/clang | grep debug_line 
        Section __debug_line: 34433811
$ size -m ~/Desktop/after-diloc-patch.clang.dSYM/Contents/Resources/DWARF/clang | grep debug_line 
        Section __debug_line: 34646430

Which callers use Instruction::applyMergedLocation versus DILocation::getMergedLocation directly?

Users of applyMergedLocation (all of these use GenerateLocation = true, now):

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
lib/Transforms/InstCombine/InstCombinePHI.cpp
lib/Transforms/Utils/PromoteMemoryToRegister.cpp
lib/Transforms/Utils/SimplifyCFG.cpp

Users of DILocation::getMergedLocation (all of these use GenerateLocation = false):

lib/CodeGen/BranchFolding.cpp
lib/CodeGen/MachineBasicBlock.cpp
lib/CodeGen/MachineSink.cpp
lib/Transforms/Scalar/ConstantHoisting.cpp

[aprantl]

I'm fine with this change since the result is more accurate than before (where the instruction would inherit whatever the previous instruction's location was.

David: how do you feel about the size increase, since you brought that up?

include/llvm/IR/DebugInfoMetadata.h

1509

... common scope the the two locations.

[dblaikie]

0.6% increase to debug_line (assuming you checked & this didn't increase
anything else? Can't see how it could, but just checking - /maybe/ some
inlining/debug_ranges stuff?) seems acceptable to me.

include/llvm/IR/DebugInfoMetadata.h

public:
   /// When two instructions are combined into a single instruction we also
   /// need to combine the original locations into a single location.
   ///
   /// When the locations are the same we can use either location. When they
   /// differ, we need a third location which is distinct from either. If
   /// they have the same file/line but have a different discriminator we
   /// could create a location with a new discriminator. If they are from
   /// different files/lines the location is ambiguous and can't be
-  /// represented in a single line entry.  In this case, no location
-  /// should be set, unless the merged instruction is a call, which we will
-  /// set the merged debug location as line 0 of the nearest common scope
-  /// where 2 locations are inlined from. This only applies to Instruction;
-  /// for MachineInstruction, as it is post-inline, we will treat the call
-  /// instruction the same way as other instructions.
-  ///
-  /// \p ForInst: The Instruction the merged DILocation is for. If the
-  /// Instruction is unavailable or non-existent, use nullptr.
-  static const DILocation *
-  getMergedLocation(const DILocation *LocA, const DILocation *LocB,
-                    const Instruction *ForInst = nullptr);
+  /// represented in a single line entry.  In this case, if \p GenerateLocation
+  /// is true, we will set the merged debug location as line 0 of the nearest
+  /// common scope where 2 locations are inlined from.
+  //
+  /// \p GenerateLocation: Whether the merged location can be generated when
+  /// \p LocA and \p LocB differ.
+  static const DILocation *getMergedLocation(const DILocation *LocA,
+                                             const DILocation *LocB,
+                                             bool GenerateLocation = false);
 
   /// Returns the base discriminator for a given encoded discriminator \p D.
   static unsigned getBaseDiscriminatorFromDiscriminator(unsigned D) {
     if ((D & 1) == 0)
       return getUnsignedFromPrefixEncoding(D >> 1);
     else
       return 0;
   }

lib/IR/DebugInfo.cpp

unsigned llvm::getDebugMetadataVersionFromModule(const Module &M) {
   if (auto *Val = mdconst::dyn_extract_or_null<ConstantInt>(
           M.getModuleFlag("Debug Info Version")))
     return Val->getZExtValue();
   return 0;
 }
 
 void Instruction::applyMergedLocation(const DILocation *LocA,
                                       const DILocation *LocB) {
-  setDebugLoc(DILocation::getMergedLocation(LocA, LocB, this));
+  bool GenerateLocation = true;
+  setDebugLoc(DILocation::getMergedLocation(LocA, LocB, GenerateLocation));
 }
 
 //===----------------------------------------------------------------------===//
 // LLVM C API implementations.
 //===----------------------------------------------------------------------===//
 
 static unsigned map_from_llvmDWARFsourcelanguage(LLVMDWARFSourceLanguage lang) {
   switch (lang) {

lib/IR/DebugInfoMetadata.cpp

DILocation *DILocation::getImpl(LLVMContext &Context, unsigned Line,
   Ops.push_back(Scope);
   if (InlinedAt)
     Ops.push_back(InlinedAt);
   return storeImpl(new (Ops.size())
                        DILocation(Context, Storage, Line, Column, Ops),
                    Storage, Context.pImpl->DILocations);
 }
 
-const DILocation *
-DILocation::getMergedLocation(const DILocation *LocA, const DILocation *LocB,
-                              const Instruction *ForInst) {
+const DILocation *DILocation::getMergedLocation(const DILocation *LocA,
+                                                const DILocation *LocB,
+                                                bool GenerateLocation) {
   if (!LocA || !LocB)
     return nullptr;
 
   if (LocA == LocB || !LocA->canDiscriminate(*LocB))
     return LocA;
 
-  if (!dyn_cast_or_null<CallInst>(ForInst))
+  if (!GenerateLocation)
     return nullptr;
 
   SmallPtrSet<DILocation *, 5> InlinedLocationsA;
   for (DILocation *L = LocA->getInlinedAt(); L; L = L->getInlinedAt())
     InlinedLocationsA.insert(L);
   const DILocation *Result = LocB;
   for (DILocation *L = LocB->getInlinedAt(); L; L = L->getInlinedAt()) {
     Result = L;

test/DebugInfo/Generic/instcombine-phi.ll

 ; RUN: opt -instcombine -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"
 
 ; If all the operands to a phi node are of the same operation, instcombine
 ; will try to pull them through the phi node, combining them into a single
-; operation.  Check that when it does this the combined operation does not
-; have a debug location set.
+; operation. Check that when it does this the combined operation has a merged
+; debug location.
 
 ; Test folding of a binary operation.  Generated from source:
 
 ; extern int foo(void);
 ; extern int bar(void);
 ; 
 ; int binop(int a, int b) {
 ;   if(a)
 ;     b -= foo();
 ;   else
 ;     b -= bar();
 ;   return b;
 ; }
 
 ; CHECK: define i32 @binop
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32 [ %call, %if.then ], [ %call1, %if.else ]
-; CHECK: sub nsw i32 %b, %[[PHI]]
-; CHECK-NOT: !dbg
+; CHECK: sub nsw i32 %b, %[[PHI]], !dbg [[binopMergedLoc:![0-9]+]]
 ; CHECK: ret i32
 
 define i32 @binop(i32 %a, i32 %b) !dbg !6 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !8
   br i1 %tobool, label %if.then, label %if.else, !dbg !8
 
 if.then:                                          ; preds = %entry

 ;   else
 ;     r = bar() < b;
 ;   return r;
 ; }
 
 ; CHECK: define i32 @cmp
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32 [ %call, %if.then ], [ %call1, %if.else ]
-; CHECK: icmp slt i32 %[[PHI]], %b
-; CHECK-NOT: !dbg
+; CHECK: icmp slt i32 %[[PHI]], %b, !dbg [[cmpMergedLoc:![0-9]+]]
 ; CHECK: ret i32
 
 define i32 @cmp(i32 %a, i32 %b) !dbg !15 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !16
   br i1 %tobool, label %if.then, label %if.else, !dbg !16
 
 if.then:                                          ; preds = %entry

 ;   else
 ;     r = &b[bar2()];
 ;   return p;
 ; }
 
 ; CHECK: define i32* @gep
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i64 [ %call, %if.then ], [ %call1, %if.else ]
-; CHECK: getelementptr inbounds i32, i32* %b, i64 %[[PHI]]
-; CHECK-NOT: !dbg
+; CHECK: getelementptr inbounds i32, i32* %b, i64 %[[PHI]], !dbg [[gepMergedLoc:![0-9]+]]
 ; CHECK: ret i32*
 
 define i32* @gep(i32 %a, i32* %b) !dbg !23 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !24
   br i1 %tobool, label %if.then, label %if.else, !dbg !24
 
 if.then:                                          ; preds = %entry

 ;   else
 ;     r = *bar3();
 ;   return r;
 ; }
 
 ; CHECK: define i32 @load
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32* [ %call, %if.then ], [ %call1, %if.else ]
-; CHECK: load i32, i32* %[[PHI]]
-; CHECK-NOT: !dbg
+; CHECK: load i32, i32* %[[PHI]],{{.*}} !dbg [[loadMergedLoc:![0-9]+]]
 ; CHECK: ret i32
 
 define i32 @load(i32 %a) !dbg !31 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !32
   br i1 %tobool, label %if.then, label %if.else, !dbg !32
 
 if.then:                                          ; preds = %entry

 ;   else
 ;     r = bar();
 ;   return r;
 ; }
 
 ; CHECK: define i64 @cast
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32 [ %call, %if.then ], [ %call1, %if.else ]
-; CHECK: sext i32 %[[PHI]] to i64
-; CHECK-NOT: !dbg
+; CHECK: sext i32 %[[PHI]] to i64, !dbg [[castMergedLoc:![0-9]+]]
 ; CHECK: ret i64
 
 define i64 @cast(i32 %a) !dbg !39 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !40
   br i1 %tobool, label %if.then, label %if.else, !dbg !40
 
 if.then:                                          ; preds = %entry

 ;   else
 ;     r = bar() - 5;
 ;   return r;
 ; }
 
 ; CHECK: define i32 @binop_const
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32 [ %call, %if.then ], [ %call1, %if.else ]
-; CHECK: add nsw i32 %[[PHI]], -5
-; CHECK-NOT: !dbg
+; CHECK: add nsw i32 %[[PHI]], -5, !dbg [[binopConstMergedLoc:![0-9]+]]
 ; CHECK: ret i32
 
 define i32 @binop_const(i32 %a) !dbg !45 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !46
   br i1 %tobool, label %if.then, label %if.else, !dbg !46
 
 if.then:                                          ; preds = %entry

 ;   else
 ;     r = bar() < 10;
 ;   return r;
 ; }
 
 ; CHECK: define i32 @cmp_const
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32 [ %call, %if.then ], [ %call1, %if.else ]
-; CHECK: icmp slt i32 %[[PHI]], 10
-; CHECK-NOT: !dbg
+; CHECK: icmp slt i32 %[[PHI]], 10, !dbg [[cmpConstMergedLoc:![0-9]+]]
 ; CHECK: ret i32
 
 define i32 @cmp_const(i32 %a) !dbg !53 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !54
   br i1 %tobool, label %if.then, label %if.else, !dbg !54
 
 if.then:                                          ; preds = %entry

 
 declare i32 @foo()
 declare i32 @bar()
 declare i64 @foo2()
 declare i64 @bar2()
 declare i32* @foo3()
 declare i32* @bar3()
 
+; CHECK: [[binopMergedLoc]] = !DILocation(line: 0
+; CHECK: [[cmpMergedLoc]] = !DILocation(line: 0
+; CHECK: [[gepMergedLoc]] = !DILocation(line: 0
+; CHECK: [[loadMergedLoc]] = !DILocation(line: 0
+; CHECK: [[castMergedLoc]] = !DILocation(line: 0
+; CHECK: [[binopConstMergedLoc]] = !DILocation(line: 0
+; CHECK: [[cmpConstMergedLoc]] = !DILocation(line: 0
+
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
 !1 = !DIFile(filename: "test.c", directory: "")
 !2 = !{}
 !3 = !{i32 2, !"Dwarf Version", i32 4}
 !4 = !{i32 2, !"Debug Info Version", i32 3}

test/DebugInfo/Generic/simplifycfg_sink_last_inst.ll

 ; RUN: opt -simplifycfg -sink-common-insts -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"
 
 ; Simplify CFG will try to sink the last instruction in a series of basic
 ; blocks, creating a "common" instruction in the successor block.  If the
 ; debug locations of the commoned instructions have different file/line
-; numbers the debug location of the common instruction should not be set.
+; numbers the debug location of the common instruction should be a merged
+; location.
 
 ; Generated from source:
 
 ; extern int foo(void);
 ; extern int bar(void);
 ; 
 ; int test(int a, int b) {
 ;   if(a)
 ;     b -= foo();
 ;   else
 ;     b -= bar();
 ;   return b;
 ; }
 
 ; CHECK: define i32 @test
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32 [ %call1, %if.else ], [ %call, %if.then ]
-; CHECK: sub nsw i32 %b, %[[PHI]]
-; CHECK-NOT: !dbg
+; CHECK: sub nsw i32 %b, %[[PHI]], !dbg [[testMergedLoc:![0-9]+]]
 ; CHECK: ret i32
 
 define i32 @test(i32 %a, i32 %b) !dbg !6 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !8
   br i1 %tobool, label %if.then, label %if.else, !dbg !8
 
 if.then:                                          ; preds = %entry

 ; int test2(int a, int b) {
 ;   if(a) b -= foo(); else b -= bar();
 ;   return b;
 ; }
 
 ; CHECK: define i32 @test2
 ; CHECK-LABEL: if.end:
 ; CHECK: %[[PHI:.*]] = phi i32 [ %call1, %if.else ], [ %call, %if.then ]
-; CHECK: sub nsw i32 %b, %[[PHI]], !dbg ![[DBG:.*]]
+; CHECK: sub nsw i32 %b, %[[PHI]], !dbg [[test2Loc:![0-9]+]]
 ; CHECK: ret i32
-; CHECK: ![[DBG]] = !DILocation(line: 17, scope: !{{.*}})
 
 define i32 @test2(i32 %a, i32 %b) !dbg !15 {
 entry:
   %tobool = icmp ne i32 %a, 0, !dbg !16
   br i1 %tobool, label %if.then, label %if.else, !dbg !16
 
 if.then:                                          ; preds = %entry
   %call = call i32 @foo(), !dbg !16
   %sub = sub nsw i32 %b, %call, !dbg !16
   br label %if.end, !dbg !16
 
 if.else:                                          ; preds = %entry
   %call1 = call i32 @bar(), !dbg !16
   %sub2 = sub nsw i32 %b, %call1, !dbg !16
   br label %if.end
 
 if.end:                                           ; preds = %if.else, %if.then
   %b.addr.0 = phi i32 [ %sub, %if.then ], [ %sub2, %if.else ]
   ret i32 %b.addr.0, !dbg !17
 }
 
+; CHECK: [[testMergedLoc]] = !DILocation(line: 0
+; CHECK: [[test2Loc]] = !DILocation(line: 17
+
 declare i32 @foo()
 declare i32 @bar()
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "", isOptimized: false, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
 !1 = !DIFile(filename: "test.c", directory: "")

test/DebugInfo/Generic/store-tail-merge.ll

 ;   }
 ;
 ;   return a;
 ; }
 ;
 ; CHECK:       define {{.*}}@foo
 ; CHECK:       if.end:
 ; CHECK-NEXT:  %storemerge = phi
-; This final check is the "real" test, verify no !dbg on the store.
-; CHECK-NEXT:  store i32 %storemerge{{.*}}, align 4{{$}}
+;
+; The debug location on the store should be a line-0 location.
+; CHECK-NEXT:  store i32 %storemerge{{.*}}, align 4, !dbg [[storeLoc:![0-9]+]]
+; CHECK: [[storeLoc]] = !DILocation(line: 0
 ;
 ; ModuleID = 'test1.ll'
 source_filename = "test.c"
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; Function Attrs: noinline nounwind uwtable
 define i32 @foo(i32 %a, i32* %d) !dbg !6 {

test/Transforms/SimplifyCFG/remove-debug-2.ll

 ; RUN: opt < %s -simplifycfg -S | FileCheck %s
 
-; Check if the debug info for hoisted store for "ret = 0" is removed
+; Check that the debug location for the hoisted store for "ret = 0" is a
+; line-0 location.
 ;
 ; int foo(int x) {
 ;   int ret = 1;
 ;   if (x)
 ;     ret = 0;
 ;   return ret;
 ; }
 ;
 ; CHECK: store i32 1,{{.+}}!dbg ![[DLOC1:[0-9]+]]
 ; CHECK: icmp ne {{.+}}!dbg ![[DLOC2:[0-9]+]]
 ; CHECK: [[VREG:%[^ ]+]] = select
-; CHECK: store i32 [[VREG]]
-; CHECK-NOT: !dbg
-; CHECK-SAME: {{$}}
+; CHECK: store i32 [[VREG]],{{.*}} !dbg [[storeLoc:![0-9]+]]
 ; CHECK: ret {{.+}}!dbg ![[DLOC3:[0-9]+]]
 ; CHECK: ![[DLOC1]] = !DILocation(line: 2
 ; CHECK: ![[DLOC2]] = !DILocation(line: 3
+; CHECK: [[storeLoc]] = !DILocation(line: 0
 ; CHECK: ![[DLOC3]] = !DILocation(line: 5
 
 target triple = "x86_64-unknown-linux-gnu"
 
 ; Function Attrs: noinline nounwind uwtable
 define i32 @foo(i32) !dbg !6 {
   %2 = alloca i32, align 4
   %3 = alloca i32, align 4

test/Transforms/SimplifyCFG/remove-debug.ll

 ; }
 
 target triple = "x86_64-unknown-linux-gnu"
 
 @x = global i32 0, align 4
 
 ; Function Attrs: uwtable
 define void @_Z3fooi(i32) #0 !dbg !6 {
-; CHECK: load i32, i32* %2, align 4, !tbaa
-; CHECK: store i32 %5, i32* @x, align 4, !tbaa
-; CHECK: call void @_Z3barv(), !dbg ![[BAR:[0-9]+]]
+; CHECK: load i32, i32* %2, align 4, !dbg ![[LOAD:[0-9]+]], !tbaa
+; CHECK: store i32 %5, i32* @x, align 4, !dbg ![[BAR:[0-9]+]], !tbaa
+; CHECK: call void @_Z3barv(), !dbg ![[BAR]]
 ; CHECK: call void @_Z3bazv(), !dbg ![[BAZ:[0-9]+]]
   %2 = alloca i32, align 4
   store i32 %0, i32* %2, align 4, !tbaa !8
   %3 = load i32, i32* %2, align 4, !dbg !12, !tbaa !8
   %4 = icmp eq i32 %3, 0, !dbg !13
   br i1 %4, label %5, label %7, !dbg !12
 
 ; <label>:5:

 
 declare void @_Z3barv() #1
 
 declare void @_Z3bazv() #1
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 
+; CHECK: ![[LOAD]] = !DILocation(line: 6, column: 7
 ; CHECK: ![[BAR]] = !DILocation(line: 0
 ; CHECK: ![[BAZ]] = !DILocation(line: 12, column: 5
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1)
 !1 = !DIFile(filename: "a", directory: "b/")
 !2 = !{}
 !3 = !{i32 2, !"Dwarf Version", i32 4}
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 !5 = !{}