[DebugInfo][LICM] Drop DebugLoc from IntrinsicInst when hoisting

[jmmartinez]

Summary

The DebugLoc is conserved when hoisting function calls, to ensure the
DIScope is preserved if inlining occurs.

This commit drops the DebugLoc in the case the call is an intrinsic
call that won't be lowered into a function call.

[dblaikie]

What's the motivation for the change - reduced debug info size by having fewer zero locations?

[jmmartinez]

In D134429#3815441, @dblaikie wrote:

What's the motivation for the change - reduced debug info size by having fewer zero locations?

The idea is to reduce the amount zero locations since they can be confusing for the users.

I thought that dropping the debug location for these kind of intrinsics would consistent with what happens for the rest of the instructions. However, I'm fairly new to the debug-info so I'm not confident about the impact that these changes may have.

[probinson]

Offhand, handling non-call intrinsic functions the same way as hoisting any other instruction seems like the way to go.

The test case looks like it could be more focused, it doesn't look complicated enough to need to be generated from C source.

[dblaikie]

In D134429#3815561, @jmmartinez wrote:

In D134429#3815441, @dblaikie wrote:

What's the motivation for the change - reduced debug info size by having fewer zero locations?

The idea is to reduce the amount zero locations since they can be confusing for the users.

At least GDB mostly ignores line zero, I think - so what sort of user confusion are you encountering/trying to address? Good to know what the use cases are, etc.

I thought that dropping the debug location for these kind of intrinsics would consistent with what happens for the rest of the instructions. However, I'm fairly new to the debug-info so I'm not confident about the impact that these changes may have.

Yeah, fair enough - works OK for me.

In D134429#3815692, @probinson wrote:

Offhand, handling non-call intrinsic functions the same way as hoisting any other instruction seems like the way to go.

Yeah, sounds OK

The test case looks like it could be more focused, it doesn't look complicated enough to need to be generated from C source.

Yep, if there's something simpler that'd be great.

[rnk]

In D134429#3815692, @probinson wrote:

Offhand, handling non-call intrinsic functions the same way as hoisting any other instruction seems like the way to go.

+1,

The test case looks like it could be more focused, it doesn't look complicated enough to need to be generated from C source.

llvm/lib/IR/DebugInfo.cpp

834

We now have three instances of setDebugLoc(DebugLoc()) in this function. I think the "early return" guidance is leading us to code duplication.

Can you please refactor this so that we calculate the conditions under which we want to apply a line zero location, and have that be the early return case? It is, if this is an intrinsic which may lower to a call, or a non-intrinsic function call.

[jmmartinez]

• Updated dropLocation() to avoid redudant setDebugLoc(DebugLoc()); return;
• Simplify the licm-hoist-intrinsic-debug-loc.ll test

[jmmartinez]

In D134429#3816054, @dblaikie wrote:

In D134429#3815561, @jmmartinez wrote:

In D134429#3815441, @dblaikie wrote:

What's the motivation for the change - reduced debug info size by having fewer zero locations?

The idea is to reduce the amount zero locations since they can be confusing for the users.

At least GDB mostly ignores line zero, I think - so what sort of user confusion are you encountering/trying to address? Good to know what the use cases are, etc.

The issue was raised, in two separate occasions, from people developing static-analysis tools that maps the assembly back to the source code (for example, to map register spills back to source-code).

llvm/lib/IR/DebugInfo.cpp

834

Updated to avoid code duplication. Thanks!

[dblaikie]

In D134429#3818016, @jmmartinez wrote:

In D134429#3816054, @dblaikie wrote:

In D134429#3815561, @jmmartinez wrote:

In D134429#3815441, @dblaikie wrote:

What's the motivation for the change - reduced debug info size by having fewer zero locations?

The idea is to reduce the amount zero locations since they can be confusing for the users.

At least GDB mostly ignores line zero, I think - so what sort of user confusion are you encountering/trying to address? Good to know what the use cases are, etc.

The issue was raised, in two separate occasions, from people developing static-analysis tools that maps the assembly back to the source code (for example, to map register spills back to source-code).

Setting no location doesn't really make the instructions reliable though - they'll be arbitrary, based on whatever instructions happen to come before them. While it's useful to ignore location 0 to provide that flow-on location behavior for an interactive debugger (rather than stepping back and forth to "I don't know where I am" to "I'm on line 5", etc) so maybe this is suggesting that those users would like data that isn't available & still won't be reliably available with this change & might cause such tools to go from "we don't have an answer" to "now we sometimes have the wrong answer and we don't know which cases are reliable and which aren't" - those tools could implement "pretend line 0 has a flow-on location" and be able to show that to the user as "this is a best guess/might be wrong" and other places that don't have line zero might be more reliable. (I mean, not a lot more reliable, we're probably using no-location in a bunch of other places and the line table's probably too expensive to encode every no-location as line 0)

But, yeah, this is consistent with the existing code here and elsewhere that uses no location, I guess. Just some concerns about what it means that someone's finding this to be a problem ^.

[probinson]

Well, yes, there are different scenarios for consumers of the line table. Profilers really should care most about "why are we in this block" rather than "where exactly did this instruction come from" while someone using addr2line to try to track down a trapping instruction would really want the most accurate provenance possible. Line info can't realistically meet all needs equally. Until we get to a line table that can comfortably address all the needs, we have to make choices.
In this particular case, adding non-call-intrinsics to the set of instructions that are already handled a particular way seems like the most consistent behavior we can have, and consistency seems like a positive thing.

llvm/lib/IR/DebugInfo.cpp

 
 void Instruction::dropLocation() {
   const DebugLoc &DL = getDebugLoc();
   if (!DL)
     return;
 
   // If this isn't a call, drop the location to allow a location from a
   // preceding instruction to propagate.
-  if (!isa<CallBase>(this)) {
+  bool MayLowerToCall = false;
+  if (isa<CallBase>(this)) {
+    auto *II = dyn_cast<IntrinsicInst>(this);
+    MayLowerToCall =
+        !II || IntrinsicInst::mayLowerToFunctionCall(II->getIntrinsicID());
+  }
+
+  if (!MayLowerToCall) {
     setDebugLoc(DebugLoc());
     return;
   }
 
   // Set a line 0 location for calls to preserve scope information in case
   // inlining occurs.
   DISubprogram *SP = getFunction()->getSubprogram();
   if (SP)
     // If a function scope is available, set it on the line 0 location. When

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

+; RUN: opt -S -licm %s | FileCheck %s
+;
+; LICM should null out debug locations when it hoists intrinsics that won't lower to function calls out of a loop.
+; CHECK: define float @foo
+; CHECK-NEXT: entry:
+; CHECK-NEXT: call float @llvm.fma.f32(float %coef_0, float %coef_1, float 0.000000e+00){{$}}
+; CHECK-NEXT: br label %loop.header
+;
+define float @foo(float* %A, float %coef_0, float %coef_1, i32 %n) !dbg !2 {
+entry:
+  br label %loop.header
+
+loop.header:
+  %i = phi i32 [ 0, %entry ], [ %i.inc, %loop.backedge ]
+  %a = phi float [ 0.000000e+00, %entry ], [ %a.inc, %loop.backedge ]
+  %cond = icmp ult i32 %i, %n
+  br i1 %cond, label %loop.backedge, label %exit
+
+loop.backedge:
+  %i.cast = zext i32 %i to i64
+  %A.ptr = getelementptr inbounds float, float* %A, i64 %i.cast
+  %A.load = load float, float* %A.ptr
+  %fma = call float @llvm.fma.f32(float %coef_0, float %coef_1, float 0.000000e+00), !dbg !3
+  %mul = fmul float %fma, %A.load
+  %a.inc = fadd float %mul, %a
+  %i.inc = add i32 %i, 1
+  br label %loop.header
+
+exit:
+  ret float %a
+}
+
+declare float @llvm.fma.f32(float, float, float) #1
+
+attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 14", isOptimized: false, runtimeVersion: 0, emissionKind: LineTablesOnly, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "source.c", directory: "/")
+!2 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !4, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !5)
+!3 = !DILocation(line: 4, column: 17, scope: !2)
+!4 = !DISubroutineType(types: !5)
+!5 = !{}
+!6 = !{i32 2, !"Debug Info Version", i32 3}
 No newline at end of file