[PATCH] D118453: [DebugInfo][InstrRef][NFC] Free resources at an earlier stage, to avoid double accounting

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1003
   // transfer function.
-  if (!VTracker)
+  if (!VTracker && !TTracker)
     return false;
----------------
Orlando wrote:
> When does this change come into effect (i.e., what stage of the algorithm/pass does this make a difference)?
Before this patch, `VTracker` being non-null implied it was the variable-assignments or install-transfers stage. That isn't true any more, hence using this changed condition to detect either of those stages.

(An actual enum would make this all better!)


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2790
   unsigned NumLocs = MTracker->getNumLocs();
+  VTracker = nullptr;
 
----------------
Orlando wrote:
> `VTracker` never owns what it's pointing to then, unlike `TTracker` and `MTracker`?
That's right; there are multiple of the former to contain per-block assignment data. Wheras the latter two are rinsed-and-reused per block.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2813-2814
+    // values. If we don't, we needlessy record the same info in memory twice.
+    delete[] MInLocs[bbnum];
+    delete[] MOutLocs[bbnum];
+    SavedLiveIns[bbnum].clear();
----------------
dblaikie wrote:
> Orlando wrote:
> > Is it worth setting to `nullptr` too, like you've done elsewhere?
> Any chance `MInLocs`/`MOutLocs` to a better memory management scheme? Looks like at least `std::unique_ptr<std::unique_ptr<ValueIDNum[]>[]>` would model the ownership here?
True about nullptr; for `std::unqiue_ptr` that ownership model works too, I'll give it a whirl on the compile-time-tracker first. (I imagine there's no overhead, but I'm getting into the habit now).


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:3045
                       << " variable assignments, exceeding limits.\n");
+
+    // Perform memory cleanup that emitLocations would do otherwise.
----------------
Orlando wrote:
> Having the clean-up moved up from the confluence into different codepaths makes me a little nervous (for future refactors etc). I'm not sure what else you can do though, just thought I'd mention it in case I'm missing an obvious alternative. I guess one option is to set the ptrs to `nullptr` after deleting on the other path, and keep the old delete loop lower down in this function (double-deleting in the common case), but I can see why you'd rather not.
Yeah, it might be better to fold the depth-first function in the child patch straight into ExtendRanges, with appropriate function stuff. Sadly, as everything gets optimised heavily, we're going to end up jumping through weird hoops even more often ._.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118453