Page MenuHomePhabricator

[DebugInfo] Remove invalidated locations during LiveDebugValues

Authored by jmorse on Thu, Aug 22, 9:22 AM.



LiveDebugValues gives variable locations to blocks, but it should also take away. There are various circumstances where a variable location is known until a loop backedge with a different location is detected. In those circumstances, where there's no agreement on the variable location, it should be undef / removed, otherwise we end up picking a location that's valid on some loop iterations but not others.

However, LiveDebugValues doesn't currently do this, see the new testcase attached. Without this patch, the location of !3 is assumed to be %bar through the loop. Once it's added to the In-Locations list, it's never removed, even though the later dbg.value(0... of !3 makes the location un-knowable.

This patch checks during block-location-joining to see whether any previously-present locations have been removed in a predecessor. If they have, the live-ins have changed, and the block needs reprocessing. Similarly, in transferTerminator, assign rather than |= the Out-Locations after processing a block, as we may have deleted some previously valid locations. This will mean that LiveDebugValues performs more propagation -- but that's necessary for it being correct.

In the two tests that change, in pieces.ll some spurious variable locations are now no longer propagated. In fission-ranges.ll, we now notice that the "a" / !14 variable is not valid for most of the function, instead a backedge with a mismatching location invalidates its location early. The control flow in the newly added test is slightly more complex than it needs to be (loop2 isn't necessary) due to other bugs.

On a clang-3.4 build a tiny number of variable locations get dropped, however the percentage of scope-bytes-covered falls from 46.5% to 45.2%. IMO this is good: those are invalid locations.

Diff Detail


Event Timeline

jmorse created this revision.Thu, Aug 22, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 22, 9:22 AM
jmorse marked an inline comment as done.Thu, Aug 22, 9:28 AM
jmorse added inline comments.
1306–1312 ↗(On Diff #216642)

Not technically necessary change here but I think it's worthwhile: the "OnPending" set this function maintains clears itself by going out of scope and back in again. This misled me for a bit, thinking there was something magical going on that meant OnPending didn't need clearing. IMHO YMMV it's better to explicitly clear it (hunk below) for ease of understanding. (Or if it's too irrelevant, then never mind).

aprantl added inline comments.Thu, Aug 22, 10:34 AM
946 ↗(On Diff #216642)

Makes sense.

1306–1312 ↗(On Diff #216642)

I'm not sure about this change. Now OnPending has state that needs to be reasoned about. Let's not roll this into this patch.

20 ↗(On Diff #216642)

This is a really nice testcase!

jmorse updated this revision to Diff 216784.Fri, Aug 23, 2:12 AM

Remove an unnecessary change.

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