[GVN+LICM] Use line 0 locations for better crash attribution

[vsk]

Summary

This is a follow-up to r291037+r291258, which used null debug locations
to prevent jumpy line tables.

Using line 0 locations achieves the same effect, but works better for
crash attribution because it preserves the right inline scope.

[wolfgangp]

LGTM

[dblaikie]

Using zero on non-call locations might bloat the line table a fair bit. May be better to let those locations get flow-on from whatever other instructions are around?

At least that, I think, is the current policy of "merge debug locations", is it not?

Perhaps we need a similar utility that can be kept in the same place (as merge debug locations)/enforce the same policy for hoisted locations.

[vsk]

In D60913#2191469, @dblaikie wrote:

Using zero on non-call locations might bloat the line table a fair bit. May be better to let those locations get flow-on from whatever other instructions are around?

At least that, I think, is the current policy of "merge debug locations", is it not?

The current policy recommends using a merged debug location here (ref), but that's not the same as not setting a debug location. The recommendation is to use the applyMergedLocation API. If there's a cheap way to keep track of the scopes of the instructions replaced by 'NewInsts', that would be a good fit here.

Perhaps we need a similar utility that can be kept in the same place (as merge debug locations)/enforce the same policy for hoisted locations.

Sorry, I don't follow. Is this some enforcement mechanism for debug location update rules?

[dblaikie]

In D60913#2194847, @vsk wrote:

In D60913#2191469, @dblaikie wrote:

Using zero on non-call locations might bloat the line table a fair bit. May be better to let those locations get flow-on from whatever other instructions are around?

At least that, I think, is the current policy of "merge debug locations", is it not?

The current policy recommends using a merged debug location here (ref),

I'm not sure it does - since this code isn't doing any merging - it's hoisting a single instruction, which I don't think is covered by that part of the documentation, is it?

but that's not the same as not setting a debug location. The recommendation is to use the applyMergedLocation API.

Specifically "applyMergedLocation" doesn't handle a single instruction - it handles two, and it's not for use when crossing a conditional boundary (taking code from somewhere that only executes under one condition and moving it to a place that might execute when that condition doesn't hold). Is that's what's happening here? (is code being hoisted across a basic block boundary?)

If there's a cheap way to keep track of the scopes of the instructions replaced by 'NewInsts', that would be a good fit here.

Perhaps we need a similar utility that can be kept in the same place (as merge debug locations)/enforce the same policy for hoisted locations.

Sorry, I don't follow. Is this some enforcement mechanism for debug location update rules?

No, sorry - I meant perhaps we should have a function like Instruction::applyMergedLocation that's for the case of hoisting code across conditionals without any merging - hmm, wait, no, now I've confused myself.

If the code is hoisting across a conditional, then there's nothing to preserve and we should be doing what the docs say - https://llvm.org/docs/HowToUpdateDebugInfo.html#id5 - drop the location.

If it's a call, even then we shouldn't preserve the inlinedAt information, because we could mislead users/profilers into believing the function was reached when it wasn't (because the condition may never be true). But we do need to set a location on calls (though this would only be for calls we know certain things about - pure/const, that sort of thing - that would allow us to hoist - is that the case here?) but it should probably be in the outer/concrete function, since we can't correctly attribute the instruction to any specific scope/inlined function, I believe.

[vsk]

In D60913#2194894, @dblaikie wrote:

In D60913#2194847, @vsk wrote:

In D60913#2191469, @dblaikie wrote:

Using zero on non-call locations might bloat the line table a fair bit. May be better to let those locations get flow-on from whatever other instructions are around?

At least that, I think, is the current policy of "merge debug locations", is it not?

The current policy recommends using a merged debug location here (ref),

I'm not sure it does - since this code isn't doing any merging - it's hoisting a single instruction, which I don't think is covered by that part of the documentation, is it?

Ah! Ok, got it. I had this confused earlier: I assumed GVN 'merged' instructions together, but this is not true for PerformLoadPRE (or for the hoist routine in LICM) touched in this patch.

but that's not the same as not setting a debug location. The recommendation is to use the applyMergedLocation API.

Specifically "applyMergedLocation" doesn't handle a single instruction - it handles two, and it's not for use when crossing a conditional boundary (taking code from somewhere that only executes under one condition and moving it to a place that might execute when that condition doesn't hold). Is that's what's happening here? (is code being hoisted across a basic block boundary?)

Right, it looks like both of the functions modified in this patch move an instruction across a basic block boundary.

If there's a cheap way to keep track of the scopes of the instructions replaced by 'NewInsts', that would be a good fit here.

Perhaps we need a similar utility that can be kept in the same place (as merge debug locations)/enforce the same policy for hoisted locations.

Sorry, I don't follow. Is this some enforcement mechanism for debug location update rules?

No, sorry - I meant perhaps we should have a function like Instruction::applyMergedLocation that's for the case of hoisting code across conditionals without any merging - hmm, wait, no, now I've confused myself.

If the code is hoisting across a conditional, then there's nothing to preserve and we should be doing what the docs say - https://llvm.org/docs/HowToUpdateDebugInfo.html#id5 - drop the location.

Now I think we're on the same page: I agree that that's what the docs recommend. It would be helpful to have some utility ('Instruction::applyHoistedLocation'?) to simplify setting the right location.

If it's a call, even then we shouldn't preserve the inlinedAt information, because we could mislead users/profilers into believing the function was reached when it wasn't (because the condition may never be true). But we do need to set a location on calls (though this would only be for calls we know certain things about - pure/const, that sort of thing - that would allow us to hoist - is that the case here?) but it should probably be in the outer/concrete function, since we can't correctly attribute the instruction to any specific scope/inlined function, I believe.

I think the hoist function can move a call, and afaik PerformLoadPRE cannot. If we were to use a helper like 'Instruction::applyHoistedLocation' in both cases, what would the helper have to look like? What I have in mind is:

• When hoisting an instruction that isn't a call, drop its location.
• If it _is_ a call, and the parent function has no debug scope, drop its location.
• Finally if it _is_ a call and the parent function has a debug scope, set its location to line 0 with the parent function's scope, and no inlinedAt location.

Does that seem reasonable?

[dblaikie]

In D60913#2201426, @vsk wrote:

In D60913#2194894, @dblaikie wrote:

In D60913#2194847, @vsk wrote:

In D60913#2191469, @dblaikie wrote:

Using zero on non-call locations might bloat the line table a fair bit. May be better to let those locations get flow-on from whatever other instructions are around?

At least that, I think, is the current policy of "merge debug locations", is it not?

The current policy recommends using a merged debug location here (ref),

I'm not sure it does - since this code isn't doing any merging - it's hoisting a single instruction, which I don't think is covered by that part of the documentation, is it?

Ah! Ok, got it. I had this confused earlier: I assumed GVN 'merged' instructions together, but this is not true for PerformLoadPRE (or for the hoist routine in LICM) touched in this patch.

but that's not the same as not setting a debug location. The recommendation is to use the applyMergedLocation API.

Specifically "applyMergedLocation" doesn't handle a single instruction - it handles two, and it's not for use when crossing a conditional boundary (taking code from somewhere that only executes under one condition and moving it to a place that might execute when that condition doesn't hold). Is that's what's happening here? (is code being hoisted across a basic block boundary?)

Right, it looks like both of the functions modified in this patch move an instruction across a basic block boundary.

If there's a cheap way to keep track of the scopes of the instructions replaced by 'NewInsts', that would be a good fit here.

Perhaps we need a similar utility that can be kept in the same place (as merge debug locations)/enforce the same policy for hoisted locations.

Sorry, I don't follow. Is this some enforcement mechanism for debug location update rules?

No, sorry - I meant perhaps we should have a function like Instruction::applyMergedLocation that's for the case of hoisting code across conditionals without any merging - hmm, wait, no, now I've confused myself.

If the code is hoisting across a conditional, then there's nothing to preserve and we should be doing what the docs say - https://llvm.org/docs/HowToUpdateDebugInfo.html#id5 - drop the location.

Now I think we're on the same page: I agree that that's what the docs recommend. It would be helpful to have some utility ('Instruction::applyHoistedLocation'?) to simplify setting the right location.

If it's a call, even then we shouldn't preserve the inlinedAt information, because we could mislead users/profilers into believing the function was reached when it wasn't (because the condition may never be true). But we do need to set a location on calls (though this would only be for calls we know certain things about - pure/const, that sort of thing - that would allow us to hoist - is that the case here?) but it should probably be in the outer/concrete function, since we can't correctly attribute the instruction to any specific scope/inlined function, I believe.

I think the hoist function can move a call, and afaik PerformLoadPRE cannot. If we were to use a helper like 'Instruction::applyHoistedLocation' in both cases, what would the helper have to look like? What I have in mind is:

• When hoisting an instruction that isn't a call, drop its location.
• If it _is_ a call, and the parent function has no debug scope, drop its location.
• Finally if it _is_ a call and the parent function has a debug scope, set its location to line 0 with the parent function's scope, and no inlinedAt location.

Does that seem reasonable?

Yeah, that sounds like what I'd think would be good. Can't guarantee it (if this function has no debug info, but is inlinable - then the call might be problematic?, if the call itself is inlinable too - I forget how we deal with this in other cases) , but that's certainly the general direction I'm on board with.

[vsk]

Sounds good, I've put this on my queue and hope to take a stab at it by Monday (8/10).

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

 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
LLVM_DEBUG(if (!NewInsts.empty()) dbgs()
              << "INSERTED " << NewInsts.size() << " INSTS: " << *NewInsts.back()
              << '\n');
 
   // Assign value numbers to the new instructions.
   for (Instruction *I : NewInsts) {
     // Instructions that have been inserted in predecessor(s) to materialize
     // the load address do not retain their original debug locations. Doing
     // so could lead to confusing (but correct) source attributions.
-    // FIXME: How do we retain source locations without causing poor debugging
-    // behavior?
-    I->setDebugLoc(DebugLoc());
+    if (const DebugLoc &DL = I->getDebugLoc())
+      I->setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
 
     // FIXME: We really _ought_ to insert these value numbers into their
     // parent's availability map.  However, in doing so, we risk getting into
     // ordering issues.  If a block hasn't been processed yet, we would be
     // marking a value as AVAIL-IN, which isn't what we intend.
     VN.lookupOrAdd(I);
   }
 

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

 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/Analysis/ScalarEvolution.h"
 #include "llvm/Analysis/ScalarEvolutionAliasAnalysis.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/IR/PredIteratorCache.h"
static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
 
   if (isa<PHINode>(I))
     // Move the new node to the end of the phi list in the destination block.
     moveInstructionBefore(I, *Dest->getFirstNonPHI(), *SafetyInfo, MSSAU);
   else
     // Move the new node to the destination block, before its terminator.
     moveInstructionBefore(I, *Dest->getTerminator(), *SafetyInfo, MSSAU);
 
-  // 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());
+  // Apply line 0 debug locations when we are moving instructions to different
+  // basic blocks because we want to avoid jumpy line tables.
+  if (const DebugLoc &DL = I.getDebugLoc())
+    I.setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
 
   if (isa<LoadInst>(I))
     ++NumMovedLoads;
   else if (isa<CallInst>(I))
     ++NumMovedCalls;
   ++NumHoisted;
 }
 

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

 ;    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: getelementptr{{.*}}%p.addr, i64 4{{.*}} !dbg [[zero:![0-9]+]]
 ; CHECK: for.body:
+; CHECK: [[zero]] = !DILocation(line: 0
 ;
 ; 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

llvm/trunk/test/Transforms/GVN/PRE/phi-translate.ll

 ; RUN: opt -basicaa -gvn -S < %s | FileCheck %s
 
 target datalayout = "e-p:64:64:64"
 
 ; CHECK-LABEL: @foo(
 ; CHECK: entry.end_crit_edge:
-; CHECK: %[[INDEX:[a-z0-9.]+]] = sext i32 %x to i64{{$}}
-; CHECK: %[[ADDRESS:[a-z0-9.]+]] = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %[[INDEX]]{{$}}
+; CHECK: %[[INDEX:[a-z0-9.]+]] = sext i32 %x to i64{{.*}} !dbg [[ZERO_LOC:![0-9]+]]
+; CHECK: %[[ADDRESS:[a-z0-9.]+]] = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %[[INDEX]]{{.*}} !dbg [[ZERO_LOC]]
 ; CHECK:   %n.pre = load i32, i32* %[[ADDRESS]], !dbg [[N_LOC:![0-9]+]]
 ; CHECK: br label %end
 ; CHECK: then:
 ; CHECK:   store i32 %z
 ; CHECK: end:
 ; CHECK:   %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ], !dbg [[N_LOC]]
 ; CHECK:   ret i32 %n
 
-; CHECK: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
+; CHECK-DAG: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
+; CHECK-DAG: [[ZERO_LOC]] = !DILocation(line: 0
 
 @G = external global [100 x i32]
 define i32 @foo(i32 %x, i32 %z) !dbg !6 {
 entry:
   %tobool = icmp eq i32 %z, 0, !dbg !7
   br i1 %tobool, label %end, label %then, !dbg !7
 
 then: