Page MenuHomePhabricator

[LiveDebugVariables] Stop trimming locations of non-inlined vars
ClosedPublic

Authored by djtodoro on May 21 2021, 6:15 AM.

Details

Summary

The D35953, D62650 and D73691 introduced trimming of variables locations in LiveDebugVariables pass, since there are some cases where after the virtregrewrite we have exploded number of DBG_VALUEs created for some inlined variables. As it looks, all problematic cases were regarding inlined variables, so it seems reasonable to stop trimming the location ranges for non-inlined variables. It has very good impact on the llvm-locstats report.

By using GDB 7.11 compiled with -g -O2 as test bed, the numbers looks as follows:

(before this patch)

$ llvm-locstats ./gdb/gdb
 ================================================
            Debug Location Statistics
 ================================================
     cov%           samples         percentage(~)
 -----------------------------------------------
   0%                 2621                2%
   (0%,10%)           5675                5%
   [10%,20%)          5050                4%
   [20%,30%)          4997                4%
   [30%,40%)          4527                4%
   [40%,50%)          4237                4%
   [50%,60%)          4679                4%
   [60%,70%)          4737                4%
   [70%,80%)          5656                5%
   [80%,90%)          6481                6%
   [90%,100%)        12957               12%
   100%              41014               39%
 ================================================
 -the number of debug variables processed: 102631
 -PC ranges covered: 56%
 -----------------------------------------------
  -total availability: 86%
 ================================================

(after applying this patch)

$ llvm-locstats ./gdb/gdb
 ================================================
            Debug Location Statistics
 ================================================
     cov%           samples         percentage(~)
 -----------------------------------------------
   0%                 2620                2%
   (0%,10%)           5628                5%
   [10%,20%)          4972                4%
   [20%,30%)          4862                4%
   [30%,40%)          4431                4%
   [40%,50%)          4109                4%
   [50%,60%)          4539                4%
   [60%,70%)          4625                4%
   [70%,80%)          5517                5%
   [80%,90%)          6232                6%
   [90%,100%)        10154                9%
   100%              44942               43%
 ================================================
 -the number of debug variables processed: 102631
 -PC ranges covered: 57%
 -----------------------------------------------
 -total availability: 86%
 ================================================

Diff Detail

Event Timeline

djtodoro created this revision.May 21 2021, 6:15 AM
djtodoro requested review of this revision.May 21 2021, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2021, 6:15 AM

Hi @djtodoro,

It seems odd that not trimming the locations would have a positive impact on coverage since, IIUC, the only difference should be an increase in bytes covered outside the lexical scope (which is not counted towards "PC ranges covered" as of D85636). 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 have vague memories from my time working on D82129 that LexicalScope::getRanges doesn't include the prologue in any range or something like that. I'm unsure whether that is intended behaviour or not - I would be interested if anyone knows? I think my solution in D82129 was similarly to avoid trimming the location ranges for function-scoped variables.

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

This makes sense to me: the range extension mechanism puts in DBG_VALUEs at points where the live value switches from one register to another, something that LiveDebugValues can't know later on, and often gets wrong. Instruction spans with the "wrong" scope in the middle of a block will prematurely terminate ranges -- or in the case of the prologue, it won't cover early instructions. I think there's also potential for an argument that's moved more than once in the prologue to be dropped, not completely certain.

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.

(One comment inline).

llvm/lib/CodeGen/LiveDebugVariables.cpp
1197

Does this range-check need to be guarded by getInlinedAt() too?

@jmorse Thanks for your comment!

the range extension mechanism puts in DBG_VALUEs at points where the live value switches from one register to another, something that LiveDebugValues can't know later on, and often gets wrong.

Yes, exactly. We are trying to fix some things in LiveDebugValues by doing some kind of analysis based on assumption that DBG_VALUE (up to LiveDebugValues) maps to a source-level assignment (that may indicate a new value), but it turns that it isn't the truth in every situation (e.g. there are a lot of duplicated DBG_VALUEs in one basic block that doesn't represent new value/assignment -- it is not only from SelectionDAG's DBG_VALUEs following COPY -- it is rather that after virtregrewrite virtual registers gets rewrited into the same physical register as some existing DBG_VALUE and some COPYs gets deleted, so after returning back DBG_VALUEs we are ending up with duplicated/redundant DBG_VALUEs for the same var. I am about to post some fixes for this in LiveDebugVariables)...

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.

llvm/lib/CodeGen/LiveDebugVariables.cpp
1197

Oh yes, nice catch. Thanks.

I think we don't need anything from D35953 for non-inlined instances, so I'll update the patch.

djtodoro updated this revision to Diff 347600.May 25 2021, 1:20 AM
  • improve the condition
foad removed a subscriber: foad.May 25 2021, 2:16 AM
Orlando added a comment.EditedMay 25 2021, 2:26 AM

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

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

I was wondering whether LexicalScope::getRanges 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.

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

EDIT: Slightly reworded to improve clarity.

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

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

I was wondering whether LexicalScope::getRanges 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#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? :)

jmorse accepted this revision.May 28 2021, 3:37 AM

LGTM plus nit.

llvm/lib/CodeGen/LiveDebugVariables.cpp
1160

Nit -- un-necessary newline

This revision is now accepted and ready to land.May 28 2021, 3:37 AM
This revision was landed with ongoing or failed builds.May 31 2021, 3:00 AM
This revision was automatically updated to reflect the committed changes.