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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

nit in commit message:

> This patch releases some memory from InstrRefBasedLDV earlier that it would otherwise. The underlying problem is:

"earlier that it would" -> "earlier than it would"

I have a couple of questions but SGTM.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1003
   // transfer function.
-  if (!VTracker)
+  if (!VTracker && !TTracker)
     return false;
----------------
When does this change come into effect (i.e., what stage of the algorithm/pass does this make a difference)?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:2790
   unsigned NumLocs = MTracker->getNumLocs();
+  VTracker = nullptr;
 
----------------
`VTracker` never owns what it's pointing to then, unlike `TTracker` and `MTracker`?


================
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();
----------------
Is it worth setting to `nullptr` too, like you've done elsewhere?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:3045
                       << " variable assignments, exceeding limits.\n");
+
+    // Perform memory cleanup that emitLocations would do otherwise.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118453