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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D60913#2194847 <https://reviews.llvm.org/D60913#2194847>, @vsk wrote:

> In D60913#2191469 <https://reviews.llvm.org/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 <https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-merge-instruction-locations>),

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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60913/new/

https://reviews.llvm.org/D60913