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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

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!

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.

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?



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:204
+
+    auto loc_indices() const {
+      return map_range(
----------------
Good use of auto to avoid misery; definitely wants a docucomment to explain what's being produced here though -- a range over all the LocIdx's that are made use of?

While we're at it, perhaps this class needs renaming now that it refers to more than one location? ValueWithProperties?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:244-245
     DbgValueProperties Properties;
+    UseBeforeDef(ArrayRef<DbgOp> Values, DebugVariable Var,
+                 DbgValueProperties Properties)
+        : Values(Values.begin(), Values.end()), Var(Var),
----------------
Nit: const refs to Var / Properties


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:343
+      SmallVector<ResolvedDbgOp> ResolvedDbgOps;
+      bool IsValidValue = true;
+      unsigned LastUseBeforeDef = 0;
----------------
Splitting hairs, IMO `IsValueValid` reads closer to a full sentence, is easier to read.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:377-378
+          if (Num.getBlock() == (unsigned)MBB.getNumber() && !Num.isPHI()) {
+            if (LastUseBeforeDef < Num.getInst())
+              LastUseBeforeDef = Num.getInst();
+            continue;
----------------
`LastUseBeforeDef = std::max(LastUseBeforeDef, Num.getInst());`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:381-383
+          if (!Var.second.Properties.IsVariadic) {
+            recoverAsEntryValue(Var.first, Var.second.Properties, Num);
+          }
----------------
I'd suggest pushing this check into recoverAsEntryValue -- then we get free checking to ensure things like `clobberMloc` don't try to recover things as entry values.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:427
+    UseBeforeDef UBD(DbgOps, Var, Properties);
+    UseBeforeDefs[Inst].push_back(UBD);
     UseBeforeDefVariables.insert(Var);
----------------
Nit: is this somewhere we can use emplace_back / std::move? Makes a difference in the rare case that the operand vector in UBD allocates, I think.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:440-442
+    // Map of values to the locations that store them for every value used by
+    // the variables that may have become available.
+    DenseMap<ValueIDNum, LocIdx> ValueToLoc;
----------------
Could we use a SmallDenseMap to avoid the initial DenseMap heap allocation? I don't think this makes much of a difference seeing how it's out of the fast-path, and use-before-defs aren't too common.


================
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]) {
----------------
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?


================
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);
+            }
----------------
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?


================
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) {
----------------
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?


================
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()];
----------------
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?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:830-834
+      // Update all instances of Src in the variable's tracked values to Dst.
+      for (ResolvedDbgOp &Op : ActiveVLocIt->second.Locs) {
+        if (Op == SrcOp)
+          Op = DstOp;
+      }
----------------
`std::replace`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128211