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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

In D102917#2779103 <https://reviews.llvm.org/D102917#2779103>, @Orlando wrote:

> 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 all the instructions at the start of the function up to (but not including) the first one with a 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.

Good catch, I think this might be interesting to play with, but I'd still go with this patch first.

> 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.

Thanks!

> EDIT: Slightly reworded to improve clarity.

@jmorse Is this ok now? :)


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

https://reviews.llvm.org/D102917