This is an archive of the discontinued LLVM Phabricator instance.

[DWARF][PATCH] Keep track of spilled variables in LiveDebugValues
ClosedPublic

Authored by wolfgangp on Feb 3 2017, 11:18 AM.

Details

Summary

When variables are spilled to the stack by the register allocator, we sometimes lose track of their debug locations.
Specifically, this happens when the spill occurs in a basic block where the spilled register is not known to contain the value of a
user variable relevant for debugging, i.e. there is no DBG_VALUE instruction that associates variable and register preceding the
spill. LiveDebugValues propagates the location information down the dominator tree but ignores the spill.

Note that the LiveDebugVariables pass keeps track of spills that occur in BBs where there is already a DBG_VALUE instruction
describing the variable's debug loc.

This patch proposes to detect spills of variables in LiveDebugValues, insert appropriate DBG_VALUES instructions after them,
and ensure that the spill locations are propagated down the dominator tree by the existing mechanism.

Brief example:

  BB1:
    ...
    %r8d = MOV32rm %rip, 1, _, @glob1, _, debug-location !47 :: ...
    DBG_VALUE debug-use %r8d, debug-use _, !33, !39, debug-location !47
    ...
	
  BB2: 
    ...
    MOV32mr %rbp, 1, _, -68, _, killed %r8d :: (store 4 into %stack.5)  <= spill of r8
    ...
  !33 = !DILocalVariable(name: "intb", scope: !21, file: !3, line: 10, type: !8)

LiveDebugValues propagates the debug loc for "intb" but ignores the spill:

BB2:
  DBG_VALUE debug-use %r8d, debug-use _, !33, !39, debug-location !47
  ...
  MOV32mr %rbp, 1, _, -68, _, killed %r8d :: (store 4 into %stack.5)  <= spill of r8
  ...
  < r8 is killed by something, the range for "intb" ends >

The patch recognizes the spill, inserts a new DBG_VALUE instruction after it. The spill location
is propagated down the dominator tree.

  BB2:
    DBG_VALUE debug-use %r8d, debug-use _, !33, !39, debug-location !47
    ...
    MOV32mr %rbp, 1, _, -68, _, killed %r8d :: (store 4 into %stack.5)  <= spill of r8
    DBG_VALUE debug-use %rbp, -68, !33, !39, debug-location !47
    ...
    < the range for "intb" now includes rbp-68 >	
  BB3:
    DBG_VALUE debug-use %rbp, -68, !33, !39, debug-location !47

Implementation notes:

  • Adding a new method transferSpillInst() whose job is to recognize spills of debug-info relevant variables and to generate DBG_VALUE instructions for them.
  • Since we don't want to insert new instructions in transfer() because we're iterating over the BB, we track the spills in a separate map and insert the new DBG_VALUE instructions after we are done iterating.

Impact on compile time, debug info size and quality (measured using a large application):

  • no measurable compile time impact
  • very small increase (<.1%) in size of .debug_loc and .debug_info sections
  • using a simple quality measure based on the percentage of code covered by location information (relative to total code size) a slight improvement from 42% without the patch to 44% with the patch

Note: Calls kill the stack pointer, so ranges for spilled variables are ended with each call. This is an orthogonal issue, but it diminishes the improvements we are getting with this patch when we suppress the frame pointer.

Diff Detail

Repository
rL LLVM

Event Timeline

wolfgangp created this revision.Feb 3 2017, 11:18 AM
aprantl edited edge metadata.Feb 3 2017, 1:54 PM

Very interesting! Have you measured the performance impact of this? Is there any?

lib/CodeGen/LiveDebugValues.cpp
324 ↗(On Diff #86922)

auto ?

405 ↗(On Diff #86922)

perhaps factor this out into a bool isSpillInstruction()?

411 ↗(On Diff #86922)

It would be good if the comment could explain why we are looking for one.

454 ↗(On Diff #86922)

return?

test/DebugInfo/MIR/X86/live-debug-values-spill.mir
195 ↗(On Diff #86922)

most of the "string" attributes can probably be safely removed.

aprantl added inline comments.Feb 3 2017, 1:55 PM
lib/CodeGen/LiveDebugValues.cpp
176 ↗(On Diff #86922)

Does the code get more readable by making this a struct with named fields instead?

Very interesting! Have you measured the performance impact of this? Is there any?

I couldn't measure any impact with one of our large games. Aside from that, we've had a private version of this out in the field for a few months now. Nothing was reported that we could trace to this change.

wolfgangp added inline comments.Feb 3 2017, 2:08 PM
lib/CodeGen/LiveDebugValues.cpp
176 ↗(On Diff #86922)

Yes, I should think so. I'll do that.

wolfgangp updated this revision to Diff 87055.Feb 3 2017, 5:26 PM

Addressed Adrian's review comments.

aprantl accepted this revision.Feb 6 2017, 10:24 AM

Thanks!

This revision is now accepted and ready to land.Feb 6 2017, 10:24 AM
This revision was automatically updated to reflect the committed changes.
chapuni added a subscriber: chapuni.Feb 8 2017, 6:15 AM

Excuse me, I have reverted it in r294447.

It triggered undefined behavior.
http://bb.pgr.jp/builders/ninja-clang-i686-msc19-R/builds/11007
(and reproducible with valgrind)

I guess Offset (in VarLoc) should be signed-aware. See also a message in r294447.