[PATCH] D128211: [DebugInfo][InstrRef][NFC] Handle transfers of variadic debug values in InstrRefLDV

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

In D128211#3685401 <https://reviews.llvm.org/D128211#3685401>, @jmorse wrote:

> Various comments inline, but this looks like the right direction and correct. A serious awkwardness is that we're adding an extra level of indirection to absolutely everything, which is painful, but it's fundamental to what variadic variable locations are. One thing that helps this is the loc_indices filter iterator you've added -- however that's not actually used in the patch? Please do use it, it'd eliminated numerous if conditionals!

Of course, I wrote a nice function for it and forgot to use it - consider it used!

> loadInLocs is getting pretty big now -- if there's an obvious portion that can be split into a method, or a helper lambda that can be defined with less indentation, that'd help.

Probably a good idea, I'll look to find a way to split it up a bit more.

> Adding iteration over all locations in checkInstForNewValues is awkward for performance: it doesn't happen for every `MachineInstr`, but there can be a _lot_ of locations when it does. And the reason it has to happen is hard to program for: tracking a value that's needed for a variable, but a half-formed variable that mustn't be given a full location yet. IIRC you were going to run this all over CTMark, were there any notable performance regressions there?

w.r.t. `checkInstForNewValues`, it is a somewhat naive implementation - a messier but typically-faster approach would be to keep track of //every// instruction in the block that defines values needed by the DbgValue, not just the last one to appear. Then in `checkInstForNewValues`, we can just look up the locations directly to see if they still contain the values we need; most of the time I'd imagine they will, and if they don't then we can either search every location or (if it's too harsh performance-wise) simply give up. Last I checked I think there was a surprisingly low performance impact, but I'll check again to make sure it's not an error in measurement.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:669
+      if (MTracker->readMLoc(NewLoc) != VarLocs[NewLoc.asU64()]) {
+        DenseMap<LocIdx, SmallSet<DebugVariable, 4>> LostMLocs;
+        for (auto &P : ActiveMLocs[NewLoc]) {
----------------
jmorse wrote:
> A dense map here is expensive IMO -- can we append to a vector of pair<LocIdx, DebugVariable> and erase later, or is the ordering in the loop important? Also, I get the feeling we need to erase all operands of the untracked variable from ActiveMLocs, not just the ones that have been clobbered, so could we just keep a collection of DebugVariables?
This could quite reasonably be a vector - the reason for using a map structure was so that we can erase all the variables for each MLoc in one go, but since we still need to make one call per LocIdx:Variable pair it's probably not worth the overhead of having the map. It could be made into a SmallDenseMap declared outside of the loop (there probably won't be any entries here most of the time) and cleared after each iteration, but the Vector is probably simpler given the lack of an "erase many" function.

> Also, I get the feeling we need to erase all operands of the untracked variable from ActiveMLocs, not just the ones that have been clobbered, so could we just keep a collection of DebugVariables?

That's what this loop is doing (see comment below).


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:670-677
+        for (auto &P : ActiveMLocs[NewLoc]) {
+          auto LostVLocIt = ActiveVLocs.find(P);
+          if (LostVLocIt != ActiveVLocs.end()) {
+            for (auto Op : LostVLocIt->second.Locs) {
+              if (!Op.IsConst && Op.Loc != NewLoc)
+                LostMLocs[Op.Loc].insert(P);
+            }
----------------
jmorse wrote:
> This is difficult to unpick -- could we make the two 'if' conditions early-continues instead? IMO a longer but shallower for loop is beneficial to readability.
> 
> I tried to think about how we could get rid of this, but we need to:
>  * Collect DILocalVariables, and
>  * Scan their operands to see if they're now untracked,
> Which necessitates two fors. I suppose this is the cost we pay for a lazy update to the tracking list.
> 
> Also: shouldn't `Op.Loc != NewLoc` be the opposite, we select this variable to be erased if one of its operands was at a location that's now clobbered?
> Also: shouldn't Op.Loc != NewLoc be the opposite, we select this variable to be erased if one of its operands was at a location that's now clobbered?

This is meant to erase the `ActiveMLocs` mappings for all of the variable's other operands after the variable is killed by the `NewLoc` clobber. For all the operands `Op.Loc == NewLoc`, we don't need the map - it's trivially solved by the line `ActiveMLocs[NewLoc.asU64()].clear()`. This check is to stop us manually erasing every variable one at a time from `ActiveMLocs[NewLoc]` when we're just going to clear the whole set anyway.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:738
+    // MLoc is dead, so end their existing MLoc->Var mappings as well.
+    DenseMap<LocIdx, SmallSet<DebugVariable, 4>> LostMLocs;
     for (auto &Var : ActiveMLocIt->second) {
----------------
jmorse wrote:
> Similar concerns as above, do we need this to be ordered during the search, do we not need to erase all operands of the DebugVariable from ActiveMLocs?
Can also be made into a vector; see comment above.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:819-820
     auto MovingVars = ActiveMLocs[Src];
-    ActiveMLocs[Dst] = MovingVars;
+    for (auto Var : MovingVars)
+      ActiveMLocs[Dst].insert(Var);
     VarLocs[Dst.asU64()] = VarLocs[Src.asU64()];
----------------
jmorse wrote:
> Insert the range perhaps? Also, isn't this a meaningful change from the prior implementation -- variables that were active at the destination used to be erased (in the overwrite), now they'll remain active. I can't remember whether this presents an issue, but is it necessary to change the behaviour?
I'll double check this one. To summarize: we assume the destination to have been pre-clobbered. The reason we do this instead of clobbering it here is that when there is an "identity" copy, we may 1) clobber the location `Dst`, 2) select a backup location for those values in `Src`, and 3) subsequently declare those variables to be live in `Dst`. This change to the logic happened in D128101; I'm not sure if this is an unnecessary change on top of that one, but I'm moderately sure I came across //some// case where it was an issue; I'll double check and get a test case out this time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128211