[PATCH] D114603: [DebugInfo][InstrRef] Terminate variable locations when overlapping fragments are assigned

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1663
     OneFragment.insert(ThisFragment);
     SeenFragments.insert({MIVar.getVariable(), OneFragment});
 
----------------
Orlando wrote:
> Rather than having `DILocalVariable` as a key, shouldn't we be using `{DILocalVariable, InlinedAt}`? Otherwise, fragments of different inlined instances of the same variable may be considered overlapping, which is incorrect.
Oooooo. That's right. And this is AFAIK the same code as in VarLocBasedLDV, so we're bug-for-bug compatible, as it were.

However, I suspect InstrRefBasedLDV is getting away with this OK:
 * We identify each fragment of a variable independently in the variable-value transfer function builder,
 * If a fragment of any variable at another inlining site overlaps, we'll try and terminate the overlapped fragments,
 * If the other inlining-sites don't use those fragments, they won't be terminated.

To illustrate, imagine we have variable !1, with fragments (0, 32), (32, 32) at inlining site A, and (16, 32) at another inlining site B. Over in `considerOverlaps`, if we're building a transfer function that has already read something like:
    {!1, A, (0, 32)} <- ValueIDNum(1, 0, 0)
And encounter a DBG_INSTR_REF for fragment (32, 32), then we'll first record the assignment to (32, 32), then terminate the location for the overlapped fragment. That makes the following transfer function:
    {!1, A, (0, 32)} <- ValueIDNum(1, 0, 0)
    {!1, A, (16, 32)} <- Undef
    {!1, A, (32, 32)} <- ValueIDNum(1, 0, 1)

Because each fragment is solved independently: the (0, 32) and (32, 32) fragments will be propagated through the block to any successors, but an overlapping fragment in (16, 32) won't (if it even exists).

This definitely deserves an extra test at the very least, and some more docs as it violates the principle of least suprise. It also raises interesting questions on how we might integrate `DbgEntityHistoryCalculator` with LiveDebugValues, which I think is the ultimate outcome of all this refactoring.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114603