Page MenuHomePhabricator

[DebugInfo] Pre-RA MachineSink: sink DBG_VALUEs that don't immediately follow the sunk instruction too
AcceptedPublic

Authored by jmorse on Feb 19 2019, 7:34 AM.

Details

Reviewers
aprantl
bjope
vsk
Summary

This patch improves the pre-register-allocation portions of MachineSink. Two things happen here -- firstly we collect DBG_VALUEs seen when walking through a machine basic block and record their virtual-register uses, so that it's easy to determine what debuginfo to sink when an instruction sinks. This avoids relying on the unsound collectDebugValues function, and so will collect DBG_VALUEs that don't immediately follow the sunk instruction.

Secondly, debug users of what an instruction defines may no longer be dominated after that instruction sinks (see: the added test case). It's possible to salvage some of these debug users if the sunk instruction was a copy (which can be propagated), and I've added a function to do this.

On a clang-3.4 build we save a fractional number of variable locations and increase covered scope-bytes by ~0.5%.

The copy-propagation to no-longer-dominated instructions makes zero contribution to that coverage improvement -- it's there mostly to ease some test failures that occur when CodeGenPrepare::placeDbgValues is deleted, a patch I will totally upload this week. If the copy-prop part of this patch is dubious, I can fold it into that later patch, where it's easier to see what's going on.

Diff Detail

Event Timeline

jmorse created this revision.Feb 19 2019, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 7:34 AM
aprantl added inline comments.Feb 21 2019, 8:22 AM
test/DebugInfo/MIR/X86/machinesink.mir
29

do you need all these attributes? They are usually a pain to upgrade in testcases.

59

Do you need the TBAA analysis results?

jmorse updated this revision to Diff 209442.Jul 12 2019, 2:48 AM

Refresh this patch:

  • Rebase it onto the current open stack
  • Remove a bunch of needless attributes and metadata

There's a minor test change too: one of the parent patches now no longer marks DBG_VALUEs undef where it can recover them by copy-propagating. This isn't a behaviour that this patch tests for, so I've updated the checks for the first DBG_VALUE in the test to not be marked "undef".

aprantl added inline comments.Jul 29 2019, 1:28 PM
lib/CodeGen/MachineSink.cpp
164

doxygen comment?

411

assert, since we already checked this above?

958

this looks very similar to the body of ProcessDbgInst...

1003

, -> . ?

1007

&& "???"

jmorse updated this revision to Diff 218360.Sep 2 2019, 7:00 AM

Revise some comments & assertions.

Apologies for the long latency involved in getting this turned around.

jmorse marked 6 inline comments as done.Sep 2 2019, 7:08 AM
jmorse added inline comments.
lib/CodeGen/MachineSink.cpp
958

It's sort-of it's compliment, ProcessDbgInst builds up the contents of SeenDbgUsers as we walk backwards through a block spotting DBG_VALUEs, while this portion of code is detecting vreg defs that are going to be sunk and collecting those seen debug insts into one vector.

The common code would be "return the SeenDbgUsers element corresponding to this operand or nullptr", for me this is a sufficiently small similarity that duplication is OK, YMMV.

aprantl accepted this revision.Sep 3 2019, 9:11 AM
This revision is now accepted and ready to land.Sep 3 2019, 9:11 AM