This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Terminate variable locations when overlapping fragments are assigned
ClosedPublic

Authored by jmorse on Nov 25 2021, 10:23 AM.

Details

Summary

In todays episode of "uncommon corner cases that LiveDebugValues handles": overlapping fragments. The modified test checks that if we have a variable where its fragments are split into overlapping segments:

DBG_VALUE $ax, $noreg, !123, !DIExpression(DW_OP_LLVM_fragment_0, 16)
...
DBG_VALUE $eax, $noreg, !123, !DIExpression(DW_OP_LLVM_fragment_0, 32)

that we only propagate the most recently assigned fragment out of a block. The test only deals with live-in variable locations, as overlaps within blocks is DbgEntityHistoryCalculators domain.

InstrRefBasedLDV needs to preserve this behaviour: we've kept the accumulateFragmentMap method from VarLocBasedLDV, we just need it to recognise DBG_INSTR_REFs. Once it's produce a mapping of variable / fragments to the overlapped variable / fragments, VLocTracker uses it to identify when a debug instruction needs to terminate the other parts it overlaps with.

The changes to live-debug-values-fragments.mir are:

  • $ax switches to $cx in two scenarios, because it's a value copied from one register to another, doesn't matter which is used,
  • The order of DBG_VALUEs produced changes with instruction referencing.

Diff Detail

Event Timeline

jmorse created this revision.Nov 25 2021, 10:23 AM
jmorse requested review of this revision.Nov 25 2021, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2021, 10:23 AM
Orlando accepted this revision.Nov 29 2021, 4:56 AM

One nit and an inline question, otherwise LGTM. The issue (if I've correctly identified an issue and not missed something) could be resolved in a separate patch IMO, so I'll hit Accept now.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1660

nit: possibly worth rewording to "A previously unprocessed debug instruction"?

1675

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.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
731

See other comment re: inlined-at and overlaps. Because of how the overlap map is constructed I think you're going to be treating fragments of 2 inlined instances of a variable as overlapping here. If I'm correct about that, I think the fix should be in how the overlap map is constructed (see other comment).

This revision is now accepted and ready to land.Nov 29 2021, 4:56 AM
jmorse added inline comments.Nov 29 2021, 3:37 PM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1675

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.

This revision was landed with ongoing or failed builds.Nov 29 2021, 3:37 PM
This revision was automatically updated to reflect the committed changes.