This is an archive of the discontinued LLVM Phabricator instance.

[LiveDebugValues][InstrRef][1/2] Try harder to recover clobbered variable locations
ClosedPublic

Authored by jmorse on Sep 28 2020, 5:00 AM.

Details

Summary

Hi,

Here's a patch that improves variable location PC-range coverage when using the instruction-referencing LiveDebugValues implementation. I've mostly been focusing on ensuring there are no regressions in variable availability (i.e., the number of variables that have at least one location), and wasn't really looking at the 'PC Ranges covered' output of llvm-locstats. This patch rectifies some of that to give a modest improvement in covered PC ranges -- however the real benefits come with entry values (see next patch, an additional 5% PC ranges).

In various circumstances, when we clobber a register there may be alternative locations that the value is live in. The classic example would be a value loaded from the stack, and then clobbered: the value is still available on the stack. InstrRefBasedLDV was coping with this at block starts where it's forced to pick a location; however it wasn't searching for alternative locations when values were clobbered. This patch notifies the "TransferTracker" object when clobbers occur, and it's able to find alternatives and issue DBG_VALUEs for the that location. See: the added test.

More generally, it's pretty clear that this should be done by something like DbgEntityHistoryCalculator instead, which would also be able to avoid redundant location transfers. But that's an improvement for another day.

Here's the output of llvm-locstats when buildling clang-3.4 before and after this patch, when forcing the use of InstrRefBasedLV, for which I've added an -mllvm switch.

Before:

=================================================
    cov%           samples         percentage(~)
-------------------------------------------------
  0%               872495               26%
  (0%,10%)          47486                1%
  [10%,20%)         50330                1%
  [20%,30%)         57127                1%
  [30%,40%)         48881                1%
  [40%,50%)         46630                1%
  [50%,60%)         59643                1%
  [60%,70%)         57581                1%
  [70%,80%)         67858                2%
  [80%,90%)         77362                2%
  [90%,100%)       100012                2%
  100%            1866414               55%
=================================================
-the number of debug variables processed: 3351819
-PC ranges covered: 56%
-------------------------------------------------
-total availability: 60%
=================================================

After:

=================================================
    cov%           samples         percentage(~)
-------------------------------------------------
  0%               872508               26%
  (0%,10%)          44994                1%
  [10%,20%)         49960                1%
  [20%,30%)         56512                1%
  [30%,40%)         48070                1%
  [40%,50%)         46135                1%
  [50%,60%)         58738                1%
  [60%,70%)         55552                1%
  [70%,80%)         65360                1%
  [80%,90%)         74530                2%
  [90%,100%)        97362                2%
  100%            1882113               56%
=================================================
-the number of debug variables processed: 3351834
-PC ranges covered: 56%
-------------------------------------------------
-total availability: 60%
=================================================

So that's roughly 16,000 more variable locations in the 100% bucket, and other variable locations have shifted up a few buckets too.

This patch isn't based on any of the other instruction referencing work, only the LiveDebugValues implementation in tree. After a few months of prodding it, it could definitely do with a refactor, but I'd like to do that from a position where it's working well. Given that it's "officially" in an experimental state, it's not clear if it needs "full" review, I'll probably take my lead from the child patch that I'll upload in a moment.

Diff Detail

Event Timeline

jmorse created this revision.Sep 28 2020, 5:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 5:00 AM
jmorse requested review of this revision.Sep 28 2020, 5:00 AM
aprantl added inline comments.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1233

this seems to be always called with false? Does that mean the second half is untested?

jmorse marked an inline comment as done.Sep 30 2020, 7:39 AM
jmorse added inline comments.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1233

There's a call to clobberMloc with the default argument (true) on lines (pre) 1735 / (post) 1798 of this patch. The background to this is D66941, spill slots can be merged, and DbgEntityHistoryCalculator doesn't terminate any variable locations when the stack is written to, so LiveDebugValues has to explicitly terminate (with DBG_VALUE $noreg) any variable locations clobbered by stack writes. (Eww).

The test for D66941 should stimulate the code path through line 1798, as it needs an explicit DBG_VALUE $noreg to be generated by LiveDebugValues.

(This is another thing that would fixed by refactoring this responsibility into / with DbgEntityHistoryCalculator; I'm deep in the tar pit here, horray).

jmorse updated this revision to Diff 300027.Oct 22 2020, 10:05 AM
jmorse marked an inline comment as done.

Rebase onto existing patches to make this applicable through arc; one line marked "future work" in livedebugvalues_instrref_tolocs.mir now has a variable location, as this is the future work that implements it.

djtodoro accepted this revision.Oct 28 2020, 8:39 AM
djtodoro added a subscriber: djtodoro.

Super nit inline.

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

, ==> .

This revision is now accepted and ready to land.Oct 28 2020, 8:39 AM
djtodoro added inline comments.Oct 29 2020, 12:24 AM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1233

Nit: make_undef ==> MakeUndef