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

Tue May 25 01:04:19 PDT 2021

djtodoro added a comment.

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

================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1172
       // may be further trimmed).
       if (RStart < IStop)
         I.insert(RStart, IStop, DbgValue);
----------------
jmorse wrote:
> Does this range-check need to be guarded by getInlinedAt() too?
Oh yes, nice catch. Thanks.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102917