[PATCH] D69584: [LDV][RAGreedy] Inform LiveDebugVariables about new VRegs added by InlineSpiller

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope marked an inline comment as done.
bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1097-1098
+  // locInts map. If an old vreg for example is spilled it might have been
+  // removed from LIS, but then we will keep the old vreg as location anyway
+  // until we rewrite locations later.
+  removeLocationIfUnused(OldLocNo);
----------------
jmorse wrote:
> 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.
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.
```






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69584/new/

https://reviews.llvm.org/D69584