[PATCH] D67393: [DebugInfo] LiveDebugValues: Defer all DBG_VALUE creation during analysis

[Vedant Kumar via Phabricator via llvm-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

In D67393#1666414 <https://reviews.llvm.org/D67393#1666414>, @jmorse wrote:

> In D67393#1666272 <https://reviews.llvm.org/D67393#1666272>, @aprantl wrote:
>
> > First, DBG_VALUEs aren't necessarily source-level assignments *before* LiveDebugValues either, they update the SSA value that a (fragment) of a source-level variable can be found in, but that SSA value could have been created by the compiler and has not necessarily any relation to a source-level assignment (think about salvageDebugInfo, for example).
>
>
> (Unfortunately I'm not known for operating the English language effectively). My meaning in the wording was about the placement of a dbg.value/DBG_VALUE within a block, ignoring its operand. AFAIUI, for any LLVM-IR instruction `i` in a function, and a (fragment of) variable, to determine the variables location at `i` one has to do an entire dominance-frontier analysis to work out which dbg.value/DBG_VALUE dominates `i` (potentially none of them). This method of recording the _position_ where variable locations _change_, closely matches the original source program: if you had five assignments to a variable in a program, you'd have five dbg.values, regardless of their operands.
>
> > The fact that the DBG_VALUE has no effect outside of the current basic block just falls out of DbgEntityHistoryCalculator not doing a LiveDebugVariable-style data flow analysis, but IMO that isn't a change in semantics, it would be *legal* for it to perform one, it just would be pointless after LiveDebugValues has propagated DBG_VALUEs across basic blocks and reached a fixed point.
>
> All true; this is where a question of "what is the design" de-jure and de-facto comes in. Because DbgEntityHistoryCalculator currently relies on LiveDebugValues having run its analysis, does that not _make_ it a semantic change? At the very least, because that's what I saw when trying to document these things, that's what I wrote.
>
> Alternately, we could document the change in interpretation as being an optimisation that DbgEntityHistoryCalculator performs/relies on, rather than being a change in semantics. (I think these are both sides of the same coin when it comes to explaining internal state).


Does the location calculator rely on LiveDebugValues for correctness? IIUC it should be possible to run the location calculator without doing LiveDebugValues, and that should produce correct (if incomplete) information.

If that's right, then we can edit the docs to 1) clarify that, 2) describe LiveDebugValues as a pass that lets the location calculator "get away with" taking a narrow, one-block-at-a-time view of the program, and 3) state that the semantics of DBG_VALUE never change.

Now, for //this// patch, I think the change looks good, and the test case is really neat :). Please let @aprantl chime in as well, though.


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

https://reviews.llvm.org/D67393