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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked 2 inline comments as done.
jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:3042-3048
+  FuncValueTable MOutLocs = FuncValueTable(new ValueTable[MaxNumBlocks]);
+  FuncValueTable MInLocs = FuncValueTable(new ValueTable[MaxNumBlocks]);
   unsigned NumLocs = MTracker->getNumLocs();
   for (unsigned i = 0; i < MaxNumBlocks; ++i) {
     // These all auto-initialize to ValueIDNum::EmptyValue
-    MOutLocs[i] = new ValueIDNum[NumLocs];
-    MInLocs[i] = new ValueIDNum[NumLocs];
+    MOutLocs[i] = ValueTable(new ValueIDNum[NumLocs]);
+    MInLocs[i] = ValueTable(new ValueIDNum[NumLocs]);
----------------
dblaikie wrote:
> dblaikie wrote:
> > prefer `std::make_unique` if possible
> Hmm, this causes a lot of allocations - any chance of this being a single allocation of `MaxNumBlocks` x `NumLocs`? (of course C++ doesn't support this via "new ValueIDNum[MaxNumBlocks][NumLocs]" (can't have multiple dynamic dimensions) - but a small wrapper type could allocate a flat array and do the `x * width + y` indexing via operator overloading))
That'd certainly be quicker -- however then it'd be more difficult to reduce peak memory consumption, as the whole table could only be freed in one go.

There's probably a better representation of this information than "big tables in memory", it's not something I've gone near optimising for yet. 


================
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);
 
----------------
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.


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

https://reviews.llvm.org/D118774