[PATCH] D70121: [DebugInfo][LDV] Teach LDV how to identify source variables and handle fragments

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In D70121#1824298 <https://reviews.llvm.org/D70121#1824298>, @Orlando wrote:

> Thanks @djtodoro for looking at this. Sorry for this really delayed response!
>
> This was initially going to be a ping, but as a result of some light offline
>  discussion I have a few questions. For clarification an "overlap" here refers to
>  a partial overlap between fragments in DIExpressions.


Apologies, I'm not up-to-date on this patch and would appreciate some clarification / a few short examples of what overlapping fragments are. Is it when you have adjacent dbg.value / DBG_VALUEs, s.t. there is some overlap in the bits they describe? So, are these valid examples? --

  Example 1 (full overlap)
  dbg.value(..., "var", DIExpression())
  dbg.value(..., "var", DIExpression())
  
  Example 2 (partial overlap)
  dbg.value(..., "var", DIExpression(DW_OP_LLVM_fragment 0 16))
  dbg.value(..., "var", DIExpression(DW_OP_LLVM_fragment 8 24)) ; bits 8-16 overlap
  
  Example 3 (no overlap)
  dbg.value(..., "var", DIExpression(DW_OP_LLVM_fragment 0 16))
  dbg.value(..., "var", DIExpression(DW_OP_LLVM_fragment 16 16))
  
  Example 3 (also no overlap)
  dbg.value(..., "var", DIExpression(DW_OP_LLVM_fragment 0 16))
  dbg.value(..., "other_var", DIExpression())
  dbg.value(..., "var", DIExpression(DW_OP_LLVM_fragment 8 24))  ; bits 8-16 would overlap, but there's an intervening dbg.value



> There are other places in llvm which do not consider overlaps, and patches
>  which ignore overlaps are being accepted (e.g. D71478 <https://reviews.llvm.org/D71478>).

I'm fuzzy on the definition here so apologies for the probably naive question, but: why are overlaps a problem? Would you mind following up on the review for D71478 <https://reviews.llvm.org/D71478> describing what it's missing?

> 1. Do overlapping fragments in dbg.values ever occur before isel currently? I would be happy to write an IRVerifier check for this and document it somewhere if this is appropriate.
> 2. More radically and long term, would it be feasible to ever ban overlapping fragments in DBG_VALUEs (MIR) somehow?
> 
>   As demonstrated by this patch overlapping fragments are a pain to handle properly. The original patch D66415 <https://reviews.llvm.org/D66415> was much smaller but didn't handle overlaps.
> 
>   I have gathered some stats from clang and llvm-as RelWithDebInfo builds @ llvm 3.4 by counting overlaps found in the livedebugvars pass with this patch:
> 
>   Terminology: `Def`: A `{ Variable, Scope, Fragment }` tuple representing a dbg.value instance. `VarSet`: A set containing all the `Defs` with any given `Variable` and `Scope`. `Overlaps`: Sum of unique `Fragment` overlaps between `Def`s in a `VarSet` for each `VarSet`.

^ Sorry, I didn't quite parse this definition.

> 
> 
>   llvm 3.4 binary |     llvm-as |       clang
>   ----------------|--------------------------
>   Num VarSets     |     2284961 |    14430359
>   Overlaps        |           2 |           7
>    
> 
> These stats are only anecdotal and the results are C++ specific so any conclusion
>  would have to be drawn very tentatively. But:
> 
> 3. I would be okay with dropping this patch in favour of cleaning up D66415 <https://reviews.llvm.org/D66415> and adding a FIXME comment since this patch may be useless in the real world. What do you think?


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

https://reviews.llvm.org/D70121