For functions with debug info, do not propagate the callsite debug location to inlined instructions.

[andreadb]

Summary

When a function F is inlined, InlineFunction extends the debug location of every instruction inlined from F by adding an InlinedAt.

However, if an instruction has a 'null' debug location, InlineFunction would propagate the callsite debug location to it. This behavior existed since revision 210459
(http://llvm.org/viewvc/llvm-project?view=revision&revision=210459).

Revision 210459 was originally committed specifically to workaround the lack of debug information for
instructions inlined from intrinsic functions (which are usually declared with attributes __always_inline__, __nodebug__).

The problem with revision 210459 is that it doesn't make any sort of distinction between instructions inlined from a 'nodebug' function and instructions which are inlined from a function built with debug info. When performing PGO on some game code, I noticed that r210459 was the main reason why some instructions in the sample had incorrect counts. As a consequence, a basic block was incorrectly seen as hot, and this was leading to a poor machine block placement.
Note that this issue may also lead to incorrect stepping in the debugger.

My fix works under the assumption that a nodebug function does not have a DISubprogram. When a function F is inlined into another function G, InlineFunction checks if it has debug info associated with it.

For nodebug functions, the InlineFunction logic is unchanged (i.e. it would still propagate the callsite debugloc to the inlined instructions). Otherwise, InlineFunction no longer propagates the callsite debug location.

Please let me know if okay to commit.

Thanks,
Andrea

[dblaikie]

lib/Transforms/Utils/InlineFunction.cpp

1375–1392 ↗

(On Diff #80416)

In a follow up commit it might be worth refactoring the body of this loop to something like:

if (DebugLoc DL = ...) {
  BI->setDebugLoc(updateInlinedAtInfo(...));
  continue;
}
if (CalleeHasDebugInfo)
  continue;
if (dyn_cast<AllocaInst>)
  if (allocaWouldBeStaticInEntry)
    continue;
BI->setDebugLoc(TheCallDL);

1377–1378 ↗

(On Diff #80416)

Leaving this instruction without a location at all may be problematic - it's still certainly inlined from this other function... - what can we do about that, if anything? (only thing that comes to mind is giving it a zero debug location, but that doesn't sound right)

Maybe no debug location is the right solution... - I wonder how that interacts with the inline range for the instruction, etc.

[probinson]

lib/Transforms/Utils/InlineFunction.cpp

1377–1378 ↗

(On Diff #80416)

If it had no location in the called function, seems like it's not inherently different for it to have no location when moved into the calling function. An instruction with no location implicitly inherits the preceding location, no? That's no different in the caller than the callee.

[andreadb]

Hi David,

lib/Transforms/Utils/InlineFunction.cpp

1377–1378 ↗

(On Diff #80416)

About the interaction with inline ranges:
as an experiment, I built test inline-debug-loc.ll with/without my patch.
Here are my findings:

Before my patch:

Contents of the .debug_info section:

<1><26>: Abbrev Number: 2 (DW_TAG_subprogram)
    <27>   DW_AT_name        : (indirect string, offset: 0x8): bar
 <1><2b>: Abbrev Number: 3 (DW_TAG_subprogram)
    <2c>   DW_AT_low_pc      : 0x10
    <34>   DW_AT_high_pc     : 0xd
    <38>   DW_AT_name        : (indirect string, offset: 0xc): baz
 <2><3c>: Abbrev Number: 4 (DW_TAG_inlined_subroutine)
    <3d>   DW_AT_abstract_origin: <0x26>
    <41>   DW_AT_ranges      : 0x0
    <45>   DW_AT_call_file   : 1
    <46>   DW_AT_call_line   : 12

Where ranges are:

Offset   Begin    End
00000000 0000000000000010 0000000000000015
00000000 0000000000000017 000000000000001a
00000000 <End of list>

Assembly in the .text:

0000000000000010 <baz>:
  10:   89 f8                   mov    %edi,%eax
  12:   83 c0 01                add    $0x1,%eax
  15:   39 f0                   cmp    %esi,%eax
  17:   0f 4c f7                cmovl  %edi,%esi
  1a:   89 f0                   mov    %esi,%eax
  1c:   c3                      retq

So, we have two ranges because instruction cmp %esi,%eax breaks the sequence. That is because it obtained its location from the callsite.

With my patch:

<2><3c>: Abbrev Number: 4 (DW_TAG_inlined_subroutine)
    <3d>   DW_AT_abstract_origin: <0x26>
    <41>   DW_AT_low_pc      : 0x10
    <49>   DW_AT_high_pc     : 0xa
    <4d>   DW_AT_call_file   : 1
    <4e>   DW_AT_call_line   : 12

I think this is more correct. Now we have a single uniform range [0x10, 0x1a). The assembly is obviously still the same (with same offsets).

My opinion is that no debug location is probably better. As Paul mentioned in his last comment, an instruction would implicitly inherit the preceding location (whathever that is..). This is also demonstrated by these findings.

llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp

 /// cannot be static.
 static bool allocaWouldBeStaticInEntry(const AllocaInst *AI ) {
   return isa<Constant>(AI->getArraySize()) && !AI->isUsedWithInAlloca();
 }
 
 /// Update inlined instructions' line numbers to
 /// to encode location where these instructions are inlined.
 static void fixupLineNumbers(Function *Fn, Function::iterator FI,
-                             Instruction *TheCall) {
+                             Instruction *TheCall, bool CalleeHasDebugInfo) {
   const DebugLoc &TheCallDL = TheCall->getDebugLoc();
   if (!TheCallDL)
     return;
 
   auto &Ctx = Fn->getContext();
   DILocation *InlinedAtNode = TheCallDL;
 
   // Create a unique call site, not to be confused with any other call from the
   // same location.
   InlinedAtNode = DILocation::getDistinct(
       Ctx, InlinedAtNode->getLine(), InlinedAtNode->getColumn(),
       InlinedAtNode->getScope(), InlinedAtNode->getInlinedAt());
 
   // Cache the inlined-at nodes as they're built so they are reused, without
   // this every instruction's inlined-at chain would become distinct from each
   // other.
   DenseMap<const DILocation *, DILocation *> IANodes;
 
   for (; FI != Fn->end(); ++FI) {
     for (BasicBlock::iterator BI = FI->begin(), BE = FI->end();
          BI != BE; ++BI) {
       DebugLoc DL = BI->getDebugLoc();
       if (!DL) {
+        if (CalleeHasDebugInfo)
+          continue;
         // If the inlined instruction has no line number, make it look as if it
         // originates from the call location. This is important for
         // ((__always_inline__, __nodebug__)) functions which must use caller
         // location for all instructions in their function body.
 
         // Don't update static allocas, as they may get moved later.
         if (auto *AI = dyn_cast<AllocaInst>(BI))
           if (allocaWouldBeStaticInEntry(AI))
if (ParentDeopt) {
         I->eraseFromParent();
       }
     }
 
     // Update the callgraph if requested.
     if (IFI.CG)
       UpdateCallGraphAfterInlining(CS, FirstNewBlock, VMap, IFI);
 
-    // Update inlined instructions' line number information.
-    fixupLineNumbers(Caller, FirstNewBlock, TheCall);
+    // For 'nodebug' functions, the associated DISubprogram is always null.
+    // Conservatively avoid propagating the callsite debug location to
+    // instructions inlined from a function whose DISubprogram is not null.
+    fixupLineNumbers(Caller, FirstNewBlock, TheCall,
+                     CalledFunc->getSubprogram() != nullptr);
 
     // Clone existing noalias metadata if necessary.
     CloneAliasScopeMetadata(CS, VMap);
 
     // Add noalias metadata if necessary.
     AddAliasScopeMetadata(CS, VMap, DL, CalleeAAR);
 
     // Propagate llvm.mem.parallel_loop_access if necessary.

llvm/trunk/test/DebugInfo/Generic/inline-debug-loc.ll

+; RUN: opt -inline -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+; Function @bar contains instruction %cmp which is not associated to any debug
+; location. This test verifies that the inliner doesn't incorrectly attribute
+; the callsite debug location to %cmp.
+
+define i32 @bar(i32 %a, i32 %b) #0 !dbg !6 {
+entry:
+  %inc = add i32 %a, 1, !dbg !8
+  %cmp = icmp slt i32 %inc, %b
+  %select = select i1 %cmp, i32 %a, i32 %b, !dbg !8
+  ret i32 %select, !dbg !8
+}
+
+
+; CHECK-LABEL: define i32 @baz(
+; CHECK: entry:
+; CHECK:   %[[INC:[a-z0-9.]+]] = add i32 %a, 1, !dbg ![[DL:[0-9]+]]
+; CHECK:   %[[CMP:[a-z0-9.]+]] = icmp slt i32 %[[INC]], %b
+; CHECK-NOT: !dbg
+; CHECK:   %[[SELECT:[a-z0-9.]+]] = select i1 %[[CMP]], i32 %a, i32 %b, !dbg ![[DL]]
+;
+; ![[DL]] = !DILocation(line: 3, scope: !{{.*}}, inlinedAt: {{.*}})
+
+define i32 @baz(i32 %a, i32 %b) !dbg !9 {
+entry:
+  %call = tail call i32 @bar(i32 %a, i32 %b), !dbg !10
+  ret i32 %call, !dbg !10
+}
+
+attributes #0 = { alwaysinline }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: true, 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}
+!6 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 2, type: !7, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !2)
+!7 = !DISubroutineType(types: !2)
+!8 = !DILocation(line: 3, scope: !6)
+!9 = distinct !DISubprogram(name: "baz", scope: !1, file: !1, line: 11, type: !7, isLocal: false, isDefinition: true, scopeLine: 11, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !2)
+!10 = !DILocation(line: 12, scope: !9)