[PATCH] D128192: [GlobalISel][DebugInfo] Propagate debug location for localized constants

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D128192#3678150 <https://reviews.llvm.org/D128192#3678150>, @dzhidzhoev wrote:

> In D128192#3677771 <https://reviews.llvm.org/D128192#3677771>, @dblaikie wrote:
>
>> In D128192#3677707 <https://reviews.llvm.org/D128192#3677707>, @dzhidzhoev wrote:
>>
>>> In D128192#3659407 <https://reviews.llvm.org/D128192#3659407>, @dzhidzhoev wrote:
>>>
>>>> In D128192#3658500 <https://reviews.llvm.org/D128192#3658500>, @dblaikie wrote:
>>>>
>>>>> @aprantl @probinson I'm really not sure, but should we consider backpropagating the first non-zero location to cover zero-location instructions at the start of a basic block more generally than this patch is proposing?
>>>>
>>>> BTW discussion about backpropagation for zero-location-instructions at the start of a basic block took in place here https://lists.llvm.org/pipermail/lldb-dev/2018-October/014263.html . There was a point against backpropagation of location for arbitrary instructions.
>>>
>>> The objection was that it is incorrect to backpropagate debug locations of the nearest instruction below on arbitrary instructions, since there is no knowledge about their semantics on AsmPrinter stage. In contrast to that, in this commit we know that instructions being marked are generated from constants and are used by the nearest following instruction.
>>
>> We forward propagate such locations, though, right? So I'm not sure back propagating is especially worse/more problematic.
>
> I'm not sure I have enough experience to answer that question.
>
> But "backpropagating the first non-zero location to cover zero-location instructions at the start of a basic block" is not generalization of this commit, since here debug locations are propagated not only for instructions at the block beginning. During localization, instructions may be put not only at the block beginning.

We could potentially do it at other places too. It wouldn't hurt profile accuracy if we're propagating a location in the same basic block. It /might/ confuse interactive/human users if we ever did this over a call or possibly over a store instruction... eh, mixed feelings. I think if the instruction has no location (as opposed to zero location) and so we're willing/already letting previous locations cover the non-location instruction, then we should be willing to do the same thing backwards too.

In this case we're talking about dropping the location entirely (that's the current behavior, yes?) - so the only time an instruction like that causes more zero locations, is when it appears at the start of a block - so I think backpropagating at the start of a block would address the issue being discussed in this patch. Other zeros would not appear if the constant is put not at the beginning of a block - those would already be getting flow-on locations from the previous locations in a block.

It's not clear that singular constants getting the location of their original constant will address the location issues described here - they might still move up over some other location and cause just the same line table size problems, but without zero locations. So that's partly why I'm not sure about this path as a way to address the problem description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128192