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

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 23 09:24:48 PST 2022


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:
> > 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)
> > 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?
> 
> /me scratches head -- apparently I'd never considered unique_ptr acting like a ... normal pointer, which can be null. Yes, that'd work, I'll revise it in.
Thoughts on lowering this further, to `ValueTable*`? (since the clients know the length (unique_ptr doesn't carry the length anyway) & don't need to change the ownership/null out the original unique_ptr, etc) 

If it is going to stay as unique_ptr/FuncValueTable - should probably add `const` on these, since the unique_ptrs themselves aren't changing, only the things they point to, and unique_ptr doesn't have deep const, so it's sort of misleading/makes the reader think maybe the unique_ptr will change?


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

https://reviews.llvm.org/D118774



More information about the llvm-commits mailing list