[DWARF] LICM should null out the debug loc of hoisted loop invariant instructions

[wolfgangp]

Summary

Another instance of nulling out debug locations when instructions are moved to different basic blocks. This time it is LICM that is hoisting loop invariant code into the loop preheader. Not nulling out the debug locs can lead to poor stepping. Merging of debug locations does not apply here as the instructions is moved and not commoned.

One exception are call instructions. Even they can be hoisted out of loops, and if they are, we leave their debug locs in place in case the call is inlined later. This does not appear to be common, though.

Keeping in mind we'd like to do better than removing the debug loc eventually, I've added a FIXME comment.

[aprantl]

lib/Transforms/Scalar/LICM.cpp

774 ↗

(On Diff #83327)

Whate happens when isa<DebugIntrinsicInst>?

[aprantl]

This is consistent with D27857.

lib/Transforms/Scalar/LICM.cpp

774 ↗

(On Diff #83327)

Oh nevermind. They are calls.

[vsk]

As an alternative to removing the debug location, what about setting a line 0 location (with appropriate scope/inlinedAt info) to the hoisted instruction? It seems like that allows debuggers to give more helpful/specific backtraces. It also doesn't affect stepping (at least not in lldb, which collapses line 0 ranges). Example:

int load(int *p) { return *p; }

int licm(int seed, int n, int *p /* Points to garbage memory. */) {
  int hash = seed;
  for (int i = 0; i < n; ++i)
    hash ^= hash + (seed >> i) + load(p); // <- Crash occurs here.
  return hash;
}

With no location (current behavior, the crash looks like it occurs somewhere in 'main'):

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xdead)                                                          
  * frame #0: 0x0000000100000f73 licm`main at licm.cc:0:3 [opt]
    frame #1: 0x0000000100000f6c licm`main(argc=<unavailable>, argv=<unavailable>) at licm.cc:18 [opt]

With line 0 location:

  * frame #0: 0x0000000100000f73 licm`main [inlined] load(p=<unavailable>) at licm.cc:0:3 [opt]
...
(lldb) up
frame #1: 0x0000000100000f73 licm`main at licm.cc:9 [opt]
   6    int licm(int seed, int n, int *p /* Points to garbage memory. */) {
   7      int hash = seed;
   8      for (int i = 0; i < n; ++i)
-> 9        hash ^= hash + (seed >> i) + load(p); // <- Crash occurs here.

@danielcdh @wolfgangp -- Would switching to line 0 locations for hoisted instructions have an adverse effect on Sample PGO?

[vsk]

ping

[wolfgangp]

@danielcdh @wolfgangp -- Would switching to line 0 locations for hoisted instructions have an adverse effect on Sample PGO?

It does indeed seem preferrable, since the hoisted instruction is currently attributed to the same line as the previous instruction, which is not what we want. I think our usage and understanding of 0 linenumbers has evolved since this change was made and we should examine all the places where we're removing the line number and see if setting it to 0 instead is better.

[vsk]

In D28390#1472790, @wolfgangp wrote:

@danielcdh @wolfgangp -- Would switching to line 0 locations for hoisted instructions have an adverse effect on Sample PGO?

It does indeed seem preferrable, since the hoisted instruction is currently attributed to the same line as the previous instruction, which is not what we want. I think our usage and understanding of 0 linenumbers has evolved since this change was made and we should examine all the places where we're removing the line number and see if setting it to 0 instead is better.

Sounds good!

[vsk]

See: https://reviews.llvm.org/D60913

llvm/trunk/lib/Transforms/Scalar/LICM.cpp

if (I.hasMetadataOtherThanDebugLoc() &&
       // time in isGuaranteedToExecute if we don't actually have anything to
       // drop.  It is a compile time optimization, not required for correctness.
       !isGuaranteedToExecute(I, DT, CurLoop, SafetyInfo))
     I.dropUnknownNonDebugMetadata();
 
   // Move the new node to the Preheader, before its terminator.
   I.moveBefore(Preheader->getTerminator());
 
+  // Do not retain debug locations when we are moving instructions to different
+  // basic blocks, because we want to avoid jumpy line tables. Calls, however,
+  // need to retain their debug locs because they may be inlined.
+  // FIXME: How do we retain source locations without causing poor debugging
+  // behavior?
+  if (!isa<CallInst>(I))
+    I.setDebugLoc(DebugLoc());
+
   if (isa<LoadInst>(I))
     ++NumMovedLoads;
   else if (isa<CallInst>(I))
     ++NumMovedCalls;
   ++NumHoisted;
   return true;
 }
 

llvm/trunk/test/DebugInfo/Generic/licm-hoist-debug-loc.ll

+; RUN: opt -S -licm %s | FileCheck %s
+;
+; LICM should null out debug locations when it hoists instructions out of a loop.
+;
+; Generated with
+; clang -O0 -S -emit-llvm test.cpp -g -gline-tables-only -o t.ll
+; opt -S -sroa -adce -simplifycfg -reassociate -domtree -loops \
+;     -loop-simplify -lcssa -basicaa -aa -scalar-evolution -loop-rotate t.ll > test.ll
+;
+; void bar(int *);
+; void foo(int k, int p)
+; {
+;    for (int i = 0; i < k; i++) {
+;      bar(&p + 4);
+;    }
+; }
+;
+; We make sure that the instruction that is hoisted into the preheader
+; does not have a debug location.
+; CHECK: for.body.lr.ph:
+; CHECK: getelementptr{{.*}}%p.addr, i64 4{{$}}
+; CHECK: for.body:
+;
+; ModuleID = 't.ll'
+source_filename = "test.c"
+
+; Function Attrs: nounwind sspstrong uwtable
+define void @foo(i32 %k, i32 %p) !dbg !7 {
+entry:
+  %p.addr = alloca i32, align 4
+  store i32 %p, i32* %p.addr, align 4
+  %cmp2 = icmp slt i32 0, %k, !dbg !9
+  br i1 %cmp2, label %for.body.lr.ph, label %for.end, !dbg !9
+
+for.body.lr.ph:                                   ; preds = %entry
+  br label %for.body, !dbg !9
+
+for.body:                                         ; preds = %for.body.lr.ph, %for.body
+  %i.03 = phi i32 [ 0, %for.body.lr.ph ], [ %inc, %for.body ]
+  %add.ptr = getelementptr inbounds i32, i32* %p.addr, i64 4, !dbg !11
+  call void @bar(i32* %add.ptr), !dbg !11
+  %inc = add nsw i32 %i.03, 1, !dbg !12
+  %cmp = icmp slt i32 %inc, %k, !dbg !9
+  br i1 %cmp, label %for.body, label %for.cond.for.end_crit_edge, !dbg !9, !llvm.loop !14
+
+for.cond.for.end_crit_edge:                       ; preds = %for.body
+  br label %for.end, !dbg !9
+
+for.end:                                          ; preds = %for.cond.for.end_crit_edge, %entry
+  ret void, !dbg !16
+}
+
+declare void @bar(i32*)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.9.1 (PS4 clang version 4.50.0.249 7e7cd823 checking)", isOptimized: false, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "D:\test")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"PIC Level", i32 2}
+!6 = !{!"clang version 3.9.1 (PS4 clang version 4.50.0.249 7e7cd823 checking)"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !8, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !2)
+!9 = !DILocation(line: 4, scope: !10)
+!10 = !DILexicalBlockFile(scope: !7, file: !1, discriminator: 1)
+!11 = !DILocation(line: 5, scope: !7)
+!12 = !DILocation(line: 4, scope: !13)
+!13 = !DILexicalBlockFile(scope: !7, file: !1, discriminator: 2)
+!14 = distinct !{!14, !15}
+!15 = !DILocation(line: 4, scope: !7)
+!16 = !DILocation(line: 7, scope: !7)