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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
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;
----------------
jmorse wrote:
> Can we make these `Register`s now that we're not bit-packing?
These ones are actually indices, not Registers.


================
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));
     }
----------------
jmorse wrote:
> 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.
Possibly - I'm not sure what the actual performance cost of `setValueUnchecked` is - I assume it'd be equivalent to a lookup, i.e. small. The performance cost of `decrementLocNosAfterPivot` shouldn't be too large, but copying the full object every time might be bad. The easiest way to handle this is probably to create a new method to check `if(DbgValue.getMaxLocNo() > LocNo)`, or more efficiently (since this would be its only use) `if(DbgValue.hasLocNoGreaterThan(LocNo))`; it's still a more expensive check than `getLocNo()` since it requires iteration and comparison, but there's no way around that without caching, which isn't an option when we need to minimize object size.


================
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)) {
----------------
jmorse wrote:
> 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
I think so as well, but I'm not sure I prefer the idea of having the same checks spread out over multiple lines with this repeated between all of them:

```
if (...) {
  LLVM_DEBUG(dbgs() << "Can't handle " << MI);
  return false;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83895