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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looking good -- I think there are some test gaps, is there coverage for:

- extendDef for a DBG_VALUE_VAR -- As I understand the -movements test, it's designed for range splitting?
- DBG_VALUE_VAR with a $noreg operand isn't propagated / split
- Reduction / replaceArg of duplicate DBG_VALUE_VAR operands

In general I think this is good -- my greatest concern is using a std::vector for DbgVariableValue, and the fact that it's cloned whenever a modification takes place. This is going to introduce a lot more allocations in the common-case, including the number-pivoting. As I understand from the history, the problem is LiveInterval having to store a huge object? Is there a plan for solving / optimising this after this patch lands? I also saw std::move is applied to DbgVariableValue in a few places, does it need an rvalue constructor for that to be effective? (I've no idea).

There's an "isReal" flag that doesn't seem to do anything and isn't documented, is this necessary?

Finally, there's the business with SpillOffset being added to expressions in insertDebugValue, but as shown in the tests, there isn't an offset in any of the spilt expressions. That'd be what I expect, because the stack layout is determined when PEI runs. Can we ditch the offset adding code and assert that it's alway zero at this stage? (Not necessary for this patch to land).



================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:209
 private:
-  unsigned LocNo : 31;
-  unsigned WasIndirect : 1;
+  std::unique_ptr<const std::vector<unsigned>> LocNos;
+  bool WasIndirect : 1;
----------------
Can we make these `Register`s now that we're not bit-packing?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:352-353
     for (LocMap::iterator I = locInts.begin(); I.valid(); ++I) {
-      DbgVariableValue DbgValue = I.value();
-      if (!DbgValue.isUndef() && DbgValue.getLocNo() > LocNo)
-        I.setValueUnchecked(DbgValue.changeLocNo(DbgValue.getLocNo() - 1));
+      const DbgVariableValue &DbgValue = I.value();
+      I.setValueUnchecked(DbgValue.decrementLocNosAfterPivot(LocNo));
     }
----------------
The previous method had the benefit that only those elements above the pivot point were re-written -- this will rewrite all of them. Is there an easy / obvious optimisation? I reckon we can land with this and optimise later if it turns out to be a significant burden.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:766-767
   if (!Discard)
-    UV->addDef(Idx, MI.getDebugOperand(0), IsIndirect, *Expr);
+    UV->addDef(Idx, SmallVector<MachineOperand, 4>(MI.debug_operands()),
+               IsIndirect, IsList, *Expr);
   else {
----------------
Can we not use an ArrayRef for the operands reference?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:856
+        &LiveIntervalInfo,
+    Optional<std::pair<SlotIndex, std::vector<unsigned>>> &Kills,
+    LiveIntervals &LIS) {
----------------
IMO, should be a SmallVector rather than a std::vector, avoids a few additional allocs.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1034
+      if (Kills) {
+        SmallVector<std::pair<unsigned, LiveInterval *>, 2> LocIntervals;
+        bool AnySubreg = false;
----------------
Mega-nit: a more specific name like "`KilledLocIntervals`" would help readability IMO.


================
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)) {
----------------
Orlando wrote:
> 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.
Seconded; 'tis nasty


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83895