[PATCH] D106856: [LiveDebugValues] Cleanup Transfers when removing Entry Value Location

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

As I understand it, this patch is aiming to recognise patterns where an entry value is being copied to a new register location and then being DBG_VALUE'd, which is effectively a no-op as far as tracking entry values is concerned.

I think the `EntryValTransfers` container is going to suffer from not being aware that it's part of a dataflow algorithm: one transfer gets inserted per emitEntryValues, and we can repeatedly explore the same block in a loop as part of the dataflow algorithm. But cleanupEntryValueTransfers will only delete one entry -- I suspect it should try and delete all matching entries. (The uh, `Transfers` collection suffers from the same problem, which I had a patch for at D67500 <https://reviews.llvm.org/D67500>, but didn't push it particularly hard).

The RegSetterInstructionMap is purely to find COPYs that might mean the entry value doesn't have to be dropped, yes? This seems OK, so long as it's not relied upon to determine whether a register contains a different value -- values can be defined when blocks merge and the `join` method runs.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1244-1245
 
+/// Do cleanup of \p EntryValTransfers created by \p TRInst, by removing the
+/// Transfer, which uses the to-be-deleted \p EntryVL.
+bool VarLocBasedLDV::cleanupEntryValueTransfers(const MachineInstr *TRInst,
----------------
Always returns true -> should instead be void? This disguises the fact that `removeEntryValue` always intends to return true when it calls this function.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1255-1256
+  auto TransRange = EntryValTransfers.equal_range(TRInst);
+  for (auto TDPair = TransRange.first, TDPEnd = TransRange.second;
+       TDPair != TDPEnd; ++TDPair) {
+    const VarLoc &EmittedEV = VarLocIDs[TDPair->second];
----------------
You might be able to use llvm::make_range and turn this into a range-based-loop, which would be a little more readable.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1258-1260
+    if (EntryVL.Var == EmittedEV.Var &&
+        EntryVL.Locs[0].Value.RegNo == EmittedEV.Locs[0].Value.RegNo &&
+        EntryVL.Expr == EmittedEV.Expr) {
----------------
Any reason to not use the VarLoc operator==?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1296-1298
+  // Case of the parameter's DBG_VALUE at the start of MBB.
+  if (!TransferInst && !LastNonDbgMI)
+    return false;
----------------
What about loop heads like this:

    entry:
      DBG_VALUE $rdi

    loop:
      DBG_VALUE $rdi
      $rdi = somedef
      JMP loop

The second DBG_VALUE refers to a different value, due to the def of `rdi` in the loop. On the first examination of `loop`, there wouldn't be any reason to remove an entry value; presumably we can rely on the `join` method to erase the `EntryValueBackupKind` locations when the loop is visited for a second time?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1335-1336
 
-  return true;
+  return cleanupEntryValueTransfers(TransferInst, OpenRanges, VarLocIDs,
+                                    EntryVL, EntryValTransfers);
 }
----------------
There are four identical return/calls like this after TransferInst is assigned, and two instances of "return false". Wouldn't it be better to switch the logic to `return cleanupEntryValue...` by default, and pick out the two instances where false is returned?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:1443
     LocIndices EntryValueIDs = VarLocIDs.insert(EntryLoc);
-    Transfers.push_back({&MI, EntryValueIDs.back()});
+    EntryValTransfers.insert({&MI, EntryValueIDs.back()});
     OpenRanges.insert(EntryValueIDs, EntryLoc);
----------------
Could I recommend `assert(EntryValueIDs.size() == 1 && "EntryValue loc should not be variadic");`.

(Purely for communicating the assumptions this is based on to the reader)


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

https://reviews.llvm.org/D106856