This is an archive of the discontinued LLVM Phabricator instance.

[LDV][RAGreedy] Inform LiveDebugVariables about new VRegs added by InlineSpiller
ClosedPublic

Authored by bjope on Oct 29 2019, 1:32 PM.

Details

Summary

Make sure RAGreedy informs LiveDebugVariables about new VRegs
that is introduced at spill by InlineSpiller.

Consider this example

LDV: !"var" [48r;128r):0 Loc0=%2

48B %2 = ...
...
128B %7 = ADD %2, ...

If %2 is spilled the InlineSpiller will insert spill/reload
instructions and introduces some new vregs. So we get

48B %4 = ...
56B spill %4
...
120B reload %5
128B %3 = ADD %5, ...

In the past we did not inform LDV about this, and when reintroducing
DBG_VALUE instruction LDV still got information that "var" had the
location of the spilled register %2 for the interval [48r;128r).
The result was bad, since we mapped "var" to the spill slot even
before the spill happened:

%4 = ...
DBG_VALUE %spill.0, !"var"
spill %4 to %spill.0
...
reload %5
%3 = ADD %5, ...

This patch will inform LDV about the interval split introduced
due to spilling. So the location map in LDV will become

!"var" [48r;56r):1 [56r;120r):0 [120r;128r):2 Loc0=%2 Loc1=%4 Loc2=%5

And when inserting DBG_VALUE instructions we get

%4 = ...
DBG_VALUE %4, !"var"
spill %4 to %spill.0
DBG_VALUE %spill.0, !"var"
...
reload %5
DBG_VALUE %5, !"var"
%3 = ADD %5, ...

Fixes: https://bugs.llvm.org/show_bug.cgi?id=38899

Event Timeline

bjope created this revision.Oct 29 2019, 1:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 1:32 PM
dstenb added a subscriber: dstenb.Oct 29 2019, 3:22 PM

Ouch @ this category of error, thanks for the patch.

Looks good with some comment nitpicks inline.

llvm/lib/CodeGen/LiveDebugVariables.cpp
275

Thanks for the refactor!

1097–1098

It's unclear to me whether "then we will" is referring to the desired or undesired behaviour. To my mind the desired behaviour would be described:

"If an old vreg for example is spilled it might have been removed from LIS, in which case we should drop the old vreg from the locations vector"

But I may have missed something.

llvm/test/CodeGen/PowerPC/pr38899-split-register-at-spill.mir
17

Best to drop attributes if we can.

bjope marked an inline comment as done.Oct 30 2019, 8:49 AM
bjope added inline comments.
llvm/lib/CodeGen/LiveDebugVariables.cpp
1097–1098

Agree that "will keep" was a bad wording here.

Old behavior was that we did not even update LDV. So in LDV we still had locations referring to the spilled vreg. But when dumping LIS that vreg does not exist any longer. IMHO this is a little bit weird. But it seems like LDV is updated later (by VirtualRegRewriter pass), mapping those old registers to spill slots (I think the VirtRegMap is involved in this).

I've not really changed the way it works to map a no longer existing vreg to a spill slot. This patch only adjusts the SlotIndexes for the old vreg to only cover the intervals where the register is spilled. And then we add intervals for the slots where we have introduced new vregs (just before the spill and after the reload).

I was kind of aiming for documenting that we have references in LDV to registers that have been removed from the code and that can't be looked up in LIS (something that I realized when working on this patch).

Maybe it is possible to do this differently in the future. Somehow letting NewRegs include spill slots, to identify that the old vreg has been spilled already when doing splitRegister/splitLocation.

I'll try to rewrite this comment to avoid the ambiguity related to "then we will". It should be clear that it is the current behavior that is described. So something like this perhaps:

Finally, remove OldLocNo unless it is still used by some interval in the locInts map. One case when OldLocNo still is in use is when the register has been spilled. In such situations the spilled register is kept as a location until rewriteLocations is called (VirtRegMap is mapping the old register to the spill slot). So for awhile we can have locations that map to virtual registers that have been removed from both the MachineFunction and from LiveIntervals.
bjope updated this revision to Diff 227143.Oct 30 2019, 10:49 AM

Addressed review comments.

bjope marked 2 inline comments as done.Oct 30 2019, 10:50 AM
jmorse accepted this revision.Oct 30 2019, 11:28 AM

LGTM; I wonder whether there are other places in the register allocator that need to notify LDV of what's happening, but I don't know enough about allocators.

This revision is now accepted and ready to land.Oct 30 2019, 11:28 AM

LGTM; I wonder whether there are other places in the register allocator that need to notify LDV of what's happening, but I don't know enough about allocators.

RAGreedy is calling splitRegister is several other places already. This was at least one place in the code where it seemed to be missing (I've kind of quickly browsed through other uses of LiveRangeEdit in RAGreedy and at other places it seemed like we ended up in calls to splitRegister sooner or later). All those calls dates back to 2013 or something (maybe it was 2011), and maybe the call to InlineSpiller here didn't even exist back in those days.

I don't know much about RABasic, but it might have similar problems (at least it got a pass dependency toward LiveDebugVariables). The PR was for RAGreedy so I simply focused on that.

This revision was automatically updated to reflect the committed changes.