Page MenuHomePhabricator

[DebugInfo][LiveDebugValues] Don't drop variable location tracking data across block boundaries
ClosedPublic

Authored by jmorse on Mon, Aug 19, 4:42 AM.

Details

Summary

LiveDebugValues propagates variable locations between blocks by creating new DBG_VALUE insts in the successors, then interpreting them when it passes back through the block at a later time. However, this flushes out any extra information about the location that LiveDebugValues holds: for example, connections between variable locations such as discussed in D65368. And as reported in PR42772 [0] this causes us to lose track of the fact that a spill-location is actually a spill, not a register location.

This patch fixes that by deferring the creation of propagated DBG_VALUEs until after propagation has completed: instead location propagation occurs only by sharing location ID numbers between blocks. As a bonus, this reduces a large amount of un-necessary extra DBG_VALUE interpretation, a clang3.4 w/asan build is 1.6% faster for me.

Details:

  • A collection of "pending" propagated-locations is maintained, and loaded into the OpenRanges object before propagation begins. This replicates the re-interpretation of propagated locations behaviour,
  • flushPendingLocs creates DBG_VALUEs from the "pending" collection, but only at the end of the pass,
  • "transferTerminator" must now be explicitly called per block, as a block may have neither terminator nor DBG_VALUE insts,
  • I've added a file-comment explaining how LiveDebugValues records locations, as it's relatively undocumented right now.

The added test function has a stack-spill in bb.1, which is restored in the exit block bb.3. Current LLVM doesn't recognise the restore because it's lost the fact that the variable location is a spill across the blocks. With this patch, it does recognise it.

[0] https://bugs.llvm.org/show_bug.cgi?id=42772

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Mon, Aug 19, 4:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Aug 19, 4:42 AM
aprantl added inline comments.Mon, Aug 19, 9:18 AM
lib/CodeGen/LiveDebugValues.cpp
12 ↗(On Diff #215857)

consistency between "DBG_VALUE instructions", "DBG_VALUE inst", and "DBG_VALUE" ? :-)

332 ↗(On Diff #215857)

I find the naming/comment pair confusing.. should this be called insertFromLocSet?

400 ↗(On Diff #215857)

inst -> instruction?

1113 ↗(On Diff #215857)

The way this comment is written it is better suited for a commit message. It would be better if the comment described what/how the code does, not a change over how it was before.

"When finalizing the LiveDebugValues pass, all new incoming locations are inserted as DBG_VALUEs at the beginning of each block" or something like that

1266 ↗(On Diff #215857)

.

jmorse updated this revision to Diff 215928.Mon, Aug 19, 9:26 AM

Adjust comment wordings

jmorse marked 5 inline comments as done.Mon, Aug 19, 9:27 AM
jmorse updated this revision to Diff 216168.Tue, Aug 20, 8:57 AM

Update: I noticed that in moving transferTerminatorInst, I'd changed the "process" method to always return false, so I've removed its return code.

The return value of transferTerminatorInst, which indicates "has this block changed", was also |'d into the wrong boolean by the previous patch, which I've fixed here, whoops.

aprantl accepted this revision.Tue, Aug 20, 9:20 AM
This revision is now accepted and ready to land.Tue, Aug 20, 9:20 AM
This revision was automatically updated to reflect the committed changes.