[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.cpp:2892-2894
+    MTracker->loadFromArray(MInLocs[bbnum].get(), bbnum);
+    TTracker->loadInlocs(MBB, MInLocs[bbnum].get(), SavedLiveIns[MBB.getNumber()],
                          NumLocs);
----------------
if functions like these took their argument by reference instead of pointer this code might be simpler (`*MInLocs[bbnum]` instead of needing the `.get()` call)


================
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]);
----------------
prefer `std::make_unique` if possible


================
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:
> 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))


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118774