[PATCH] D105026: [2/2][LiveDebugVariables] Avoid reduntant DBG_VALUEs after virtregrewrite

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

Hi @djtodoro,

Thanks for splitting the review into two. If D105025 <https://reviews.llvm.org/D105025> gets accepted as it is then this one makes sense and LGTM. The patch summary could probably be a little more descriptive though. Maybe something like "Ignore instructions that do not modify the variable location when searching backwards for duplicate DBG_VALUEs, rather than stopping the search"?

Lastly, I think it might be worth having a dedicated test for this - or if not, at least adding a comment into the modified test so that it's clear that this feature is being tested. I'm worried that the test could change or go away without us noticing and we'd lose the testing coverage. wdyt?



================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1640
 // Avoid duplicates by scanning the machine basic block backward, up to the
 // first non-debug instr or up to the MBBStart.
 static bool shouldSkipThisDbgValue(MachineBasicBlock *MBB, MachineInstr &MI,
----------------
Please can you update the comment since this function does now look through some non-debug instrs?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1661-1662
+
+      it = std::next(it);
+      continue;
+    }
----------------
nit: It might be nice if there was a way to arrange this loop that avoids the `it = std:next(it)` requirement on every `continue` - I can't think of an easy way to do it though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105026