[PATCH] D118774: [DebugInfo][InstrRef][NFC] Use unique_ptr instead of raw pointers for value tables

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h:904-905
   /// Observe a single instruction while stepping through a block.
-  void process(MachineInstr &MI, ValueIDNum **MLiveOuts = nullptr,
-               ValueIDNum **MLiveIns = nullptr);
+  void process(MachineInstr &MI, FuncValueTable *MLiveOuts = nullptr,
+               FuncValueTable *MLiveIns = nullptr);
 
----------------
jmorse wrote:
> dblaikie wrote:
> > Looks like this adds an extra level of indirection that's not needed? `FuncValueTable` itself could be null (since it's a unique_ptr) rather than needing a pointer to it that is null?
> This is true, unfortunately it runs into some technical debt -- in an early phase, InstrRefBasedLDV steps through all instructions but it's too early to know how many registers/slots are going to be tracked, so these value tables aren't allocated yet. That's the only reason why these variables are pointers, so that on the early phase the tables can be optional.
> 
> The root cause is that the early phase shares code with later phases: something that can be fixed, but as part of a larger refactor.
Sorry, I'm not following.

If you remove the extra indirection - concretely, what breaks? The FuncValueTable type is std::unique_ptr, so it can be null when the tables aren't allocated yet, right?

I guess maybe because these are being passed by non-const ref, so they can't have an rvalue as a default argument? They could be passed by const ref instead (since unique_ptr has shallow const). That'd allow the nullptr default argument value.

Though, having tried that manually/locally, and made that compile - maybe going a step further: Pass this as ValueTable* (pointer to the unique_ptr's buffer) since the ownership isn't relevant to the callees here, they just care about a buffer of ValueTables? (call site would have to use `X.get()` rather than the current `&X` - so it doesn't make the call site tidier, but does remove the extra indirection)


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

https://reviews.llvm.org/D118774