[PATCH] D102917: [LiveDebugVariables] Stop trimming locations of non-inlined vars

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

In D102917#2776626 <https://reviews.llvm.org/D102917#2776626>, @djtodoro wrote:

> @Orlando Thanks for looking into this!
>
>> I can see that in the tests you've updated in this patch some DBG_VALUEs are moved from just outside to inside the prologue.
>
> I think this is the main reason of the improvement. The motivation for this fix was to improve MIR and DBG_VALUE, so it becomes as close as possible to its COPY.

Sounds reasonable to me.

In D102917#2776763 <https://reviews.llvm.org/D102917#2776763>, @jmorse wrote:

> [...] or in the case of the prologue, it won't cover early instructions.

I was wondering whether `LexicalScope::getRange`s not including the prologue was a bug. There's no mention of this in the `getRanges` doxygen comment. Ah, I see that in `LexicalScopes::extractLexicalScopes` that instructions with an empty DebugLoc are skipped when trying to determine start/end of ranges. This means that the whole prologue, and any subsequent instructions with no DebugLoc, will not be included in any range. I think it might make sense to use the DISubprogram scope for the first instructions encountered if they have no DebugLoc? IIUC that would mean we don't need this change in LiveDebugVariables because all instructions in a function would be included in a range from `getRanges`. OTOH other parts of llvm also use `getRanges` so changing the behaviour may be undesirable or have unintended side effects, and also my speculations could just be wrong too. With that in mind, perhaps this (your patch) is the best way to approach the problem.

In D102917#2778963 <https://reviews.llvm.org/D102917#2778963>, @djtodoro wrote:

>> In D102917#2776763 <https://reviews.llvm.org/D102917#2776763>, @jmorse wrote:
>> Either way: it's my understanding that the range trimming is about avoiding excessive compile time costs. There's no functional reason why we shouldn't extend all ranges over the whole program, it just happens to create pathological behaviour in certain circumstances, particularly when there's heavy inlining. IMO, making this conditional on a variable not being inlined is a good trade-off, we produce as much information as possible for the small number of "top level" variables, and less for everything that's inlined.
>
> I agree.

+1. FWIW I'm happy with this change either way that it is implemented. I'm going to be "out of office" for a week starting tomorrow so please don't wait for me.


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

https://reviews.llvm.org/D102917