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

[jmorse]

Summary

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

• We store a big table of "live in values for each block",
• We translate that into DBG_VALUE instructions in each block,

And both exist in memory at the same time, which needlessly doubles that information. The most of what this patch does is: as we progressively translate live-in information into DBG_VALUEs, we free the variable-value / machine-value tracking information as we go, which significantly reduces peak memory.

Rolled into this patch too: once we've build the per-block-live-in information, call "clear" on the collection of VLocTracker objects. These store all the variable assignments that happen in a block, there's no need to keep that around once we've made use of them. Doing so exposes another flaw: it seems we were spuriously collecting all variable assignments for a second time, into the final blocks assignment records. Stop this by setting the "VTracker" field of InstrRefBasedLDV to null, and in a couple of locations fix the condition for recording some information.

As a final twiddle, change a DenseMap into a SmallDenseMap to avoid what might be an un-necessary initial allocation.

[Orlando]

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.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1032

When does this change come into effect (i.e., what stage of the algorithm/pass does this make a difference)?

2830

VTracker never owns what it's pointing to then, unlike TTracker and MTracker?

2853–2854

Is it worth setting to nullptr too, like you've done elsewhere?

3094

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.

[dblaikie]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

2853–2854

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?

[jmorse]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

1032

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!)

2830

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.

2853–2854

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

3094

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 ._.

[jmorse]

Will land on the back of Orlando's SGTM; shifting to unique_ptr sounds good and doesn't appear to have a performance impact, I've spun that into D118774.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h

public:
   /// Needs to be a MapVector because we determine order-in-the-input-MIR from
   /// the order in this container.
   /// We only retain the last DbgValue in each block for each variable, to
   /// determine the blocks live-out variable value. The Vars container forms the
   /// transfer function for this block, as part of the dataflow analysis. The
   /// movement of values between locations inside of a block is handled at a
   /// much later stage, in the TransferTracker class.
   MapVector<DebugVariable, DbgValue> Vars;
-  DenseMap<DebugVariable, const DILocation *> Scopes;
+  SmallDenseMap<DebugVariable, const DILocation *, 8> Scopes;
   MachineBasicBlock *MBB = nullptr;
   const OverlapMap &OverlappingFragments;
   DbgValueProperties EmptyProperties;
 
 public:
   VLocTracker(const OverlapMap &O, const DIExpression *EmptyExpr)
       : OverlappingFragments(O), EmptyProperties(EmptyExpr, false) {}
 
for (auto FragmentInfo : Overlaps->second) {
 
       // Attempt insertion; overwrite if it's already mapped.
       auto Result = Vars.insert(std::make_pair(Overlapped, Rec));
       if (!Result.second)
         Result.first->second = Rec;
       Scopes[Overlapped] = Loc;
     }
   }
+
+  void clear() {
+    Vars.clear();
+    Scopes.clear();
+  }
 };
 
 // XXX XXX docs
 class InstrRefBasedLDV : public LDVImpl {
 public:
   friend class ::InstrRefLDVTest;
 
   using FragmentInfo = DIExpression::FragmentInfo;

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

 bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI,
                                              ValueIDNum **MLiveOuts,
                                              ValueIDNum **MLiveIns) {
   if (!MI.isDebugRef())
     return false;
 
   // Only handle this instruction when we are building the variable value
   // transfer function.
-  if (!VTracker)
+  if (!VTracker && !TTracker)
     return false;
 
   unsigned InstNo = MI.getOperand(0).getImm();
   unsigned OpNo = MI.getOperand(1).getImm();
 
   const DILocalVariable *Var = MI.getDebugVariable();
   const DIExpression *Expr = MI.getDebugExpression();
   const DILocation *DebugLoc = MI.getDebugLoc();
if (NewID && !SeenSubregs.empty()) {
     }
   }
 
   // We, we have a value number or None. Tell the variable value tracker about
   // it. The rest of this LiveDebugValues implementation acts exactly the same
   // for DBG_INSTR_REFs as DBG_VALUEs (just, the former can refer to values that
   // aren't immediately available).
   DbgValueProperties Properties(Expr, false);
+  if (VTracker)
     VTracker->defVar(MI, Properties, NewID);
 
   // If we're on the final pass through the function, decompose this INSTR_REF
   // into a plain DBG_VALUE.
   if (!TTracker)
     return true;
 
   // Pick a location for the machine value number, if such a location exists.
   // (This information could be stored in TransferTracker to make it faster).

 #endif
 
 void InstrRefBasedLDV::emitLocations(
     MachineFunction &MF, LiveInsT SavedLiveIns, ValueIDNum **MOutLocs,
     ValueIDNum **MInLocs, DenseMap<DebugVariable, unsigned> &AllVarsNumbering,
     const TargetPassConfig &TPC) {
   TTracker = new TransferTracker(TII, MTracker, MF, *TRI, CalleeSavedRegs, TPC);
   unsigned NumLocs = MTracker->getNumLocs();
+  VTracker = nullptr;
 
   // For each block, load in the machine value locations and variable value
   // live-ins, then step through each instruction in the block. New DBG_VALUEs
   // to be inserted will be created along the way.
   for (MachineBasicBlock &MBB : MF) {
     unsigned bbnum = MBB.getNumber();
     MTracker->reset();
     MTracker->loadFromArray(MInLocs[bbnum], bbnum);
     TTracker->loadInlocs(MBB, MInLocs[bbnum], SavedLiveIns[MBB.getNumber()],
                          NumLocs);
 
     CurBB = bbnum;
     CurInst = 1;
     for (auto &MI : MBB) {
       process(MI, MOutLocs, MInLocs);
       TTracker->checkInstForNewValues(CurInst, MI.getIterator());
       ++CurInst;
     }
+
+    // Our block information has now been converted into DBG_VALUEs, to be
+    // inserted below. Free the memory we allocated to track variable / register
+    // values. If we don't, we needlessy record the same info in memory twice.
+    delete[] MInLocs[bbnum];
+    delete[] MOutLocs[bbnum];
+    MInLocs[bbnum] = nullptr;
+    MOutLocs[bbnum] = nullptr;
+    SavedLiveIns[bbnum].clear();
   }
 
    emitTransfers(AllVarsNumbering);
 }
 
 void InstrRefBasedLDV::initialSetup(MachineFunction &MF) {
   // Build some useful data structures.
 
bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
   // bail out at this point. We've burnt some time doing analysis already,
   // however we should cut our losses.
   if ((unsigned)MaxNumBlocks > InputBBLimit &&
       VarAssignCount > InputDbgValLimit) {
     LLVM_DEBUG(dbgs() << "Disabling InstrRefBasedLDV: " << MF.getName()
                       << " has " << MaxNumBlocks << " basic blocks and "
                       << VarAssignCount
                       << " variable assignments, exceeding limits.\n");
+
+    // Perform memory cleanup that emitLocations would do otherwise.
+    for (int Idx = 0; Idx < MaxNumBlocks; ++Idx) {
+      delete[] MOutLocs[Idx];
+      delete[] MInLocs[Idx];
+    }
   } else {
     // Compute the extended ranges, iterating over scopes. There might be
     // something to be said for ordering them by size/locality, but that's for
     // the future. For each scope, solve the variable value problem, producing
     // a map of variables to values in SavedLiveIns.
     for (auto &P : ScopeToVars) {
       buildVLocValueMap(ScopeToDILocation[P.first], P.second,
                    ScopeToAssignBlocks[P.first], SavedLiveIns, MOutLocs, MInLocs,
                    vlocs);
     }
 
+    // Now that we've analysed variable assignments, free any tracking data.
+    vlocs.clear();
+
     // Using the computed value locations and variable values for each block,
     // create the DBG_VALUE instructions representing the extended variable
     // locations.
     emitLocations(MF, SavedLiveIns, MOutLocs, MInLocs, AllVarsNumbering, *TPC);
 
     // Did we actually make any changes? If we created any DBG_VALUEs, then yes.
     Changed = TTracker->Transfers.size() != 0;
   }
 
-  // Common clean-up of memory.
-  for (int Idx = 0; Idx < MaxNumBlocks; ++Idx) {
-    delete[] MOutLocs[Idx];
-    delete[] MInLocs[Idx];
-  }
+  // Elements of these arrays will be deleted by emitLocations.
   delete[] MOutLocs;
   delete[] MInLocs;
 
   delete MTracker;
   delete TTracker;
   MTracker = nullptr;
   VTracker = nullptr;
   TTracker = nullptr;