[PATCH] D83895: [DebugInfo] Process DBG_VALUE_LIST in LiveDebugVariables

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

Orlando added a comment.

Hi, sorry for leaving this for so long! I think it will take me a couple of passes to fully understand it, but for now I left some inline nits/questions. Please feel free to leave the nit changes until later revisions.

I'm hoping you can clarify something about `addDefsFromCopies` for me. IIUC without the patch we are adding defs from copies at kill points if possible. And with the patch a def is only added on the _first_ kill. Is that right?



================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:103
 public:
-  DbgVariableValue(unsigned LocNo, bool WasIndirect,
-                   const DIExpression &Expression)
-      : LocNo(LocNo), WasIndirect(WasIndirect), Expression(&Expression) {
-    assert(getLocNo() == LocNo && "location truncation");
+  DbgVariableValue(ArrayRef<unsigned> NewLocs, bool WasIndirect, bool WasList,
+                   const DIExpression &Expr)
----------------
It looks like this class was trying to be small on purpose, and it is now relatively much larger. I don't know if it will make a significant impact or not, have you managed to run any kind of performance tests with this patch yet?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:105
+                   const DIExpression &Expr)
+      : WasIndirect(WasIndirect), WasList(WasList), Expression(&Expr) {
+    for (unsigned LocNo : NewLocs) {
----------------
IIRC you mentioned that DBG_VALUE_LIST don't have the 'IsIndirect' field. It may be worth adding an assert here that `!(WasIndirect && WasList)`?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:178-179
+  SmallVector<unsigned, 4> LocNos;
+  bool WasIndirect;
+  bool WasList;
   const DIExpression *Expression = nullptr;
----------------
IMO it'd be nice to make these default `= false` and remove the default constructor.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:350-356
   /// \param Idx Starting point for the definition.
   /// \param DbgValue value to propagate.
   /// \param LR Restrict liveness to where LR has the value VNI. May be null.
   /// \param VNI When LR is not null, this is the value to restrict to.
   /// \param [out] Kills Append end points of VNI's live range to Kills.
   /// \param LIS Live intervals analysis.
+  void extendDef(SlotIndex Idx, DbgVariableValue DbgValue,
----------------
Please can you update this comment, and the one for `addDefsFromCopies`?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:667-670
+  if (!MI.isDebugValue() || !MI.getDebugVariableOp().isMetadata() ||
+      MI.isNonListDebugValue() &&
+          (!(MI.getDebugOffset().isImm() || MI.getDebugOffset().isReg()) ||
+           MI.getNumOperands() != 4)) {
----------------
This `if` looks a bit gnarly to me. I would personally prefer a couple of simpler early exit checks instead, though I'm not sure if there's anything in the style guide to mandate it.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1516-1517
 
-    LLVM_DEBUG(dbgs() << "\t[" << Start << ';' << Stop
-                      << "):" << DbgValue.getLocNo());
+    LLVM_DEBUG(auto &dbg = dbgs(); dbg << "\t[" << Start << ';' << Stop << "):";
+               DbgValue.printLocNos(dbg));
     MachineFunction::iterator MBB = LIS.getMBBFromIndex(Start)->getIterator();
----------------
Is it not possible to do this instead?
```
    LLVM_DEBUG(dbgs() << "\t[" << Start << ';' << Stop << "):";
               DbgValue.printLocNos(dbgs()));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83895