[LiveDebugValues][nfc] Reduce memory usage of InstrRef (#76051)

felipepiovezan · web-flow · commit acacec3bbf45 · 2023-12-23T13:44:45.000-03:00
Commit 1b531d5 (#74203) removed the usage of unique_ptrs of arrays in favour of using vectors, but inadvertently increased peak memory usage by removing the ability to deallocate vector memory that was no longer needed mid-LDV. In that same review, it was pointed out that `FuncValueTable` typedef could be removed, since it was "just a vector". This commit addresses both issues by making `FuncValueTable` a real data structure, capable of mapping BBs to ValueTables and able to free ValueTables as needed. This reduces peak memory usage in the compiler by 10% in the benchmarks flagged by the original review. As a consequence, we had to remove a handful of instances of the "declare-then-initialize" antipattern in unittests, as the FuncValueTable class is no longer default-constructible.
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -2413,7 +2413,7 @@ bool InstrRefBasedLDV::mlocJoin(
 
     // Pick out the first predecessors live-out value for this location. It's
     // guaranteed to not be a backedge, as we order by RPO.
-    ValueIDNum FirstVal = OutLocs[BlockOrders[0]->getNumber()][Idx.asU64()];
+    ValueIDNum FirstVal = OutLocs[*BlockOrders[0]][Idx.asU64()];
 
     // If we've already eliminated a PHI here, do no further checking, just
     // propagate the first live-in value into this block.
@@ -2430,8 +2430,7 @@ bool InstrRefBasedLDV::mlocJoin(
     bool Disagree = false;
     for (unsigned int I = 1; I < BlockOrders.size(); ++I) {
       const MachineBasicBlock *PredMBB = BlockOrders[I];
-      const ValueIDNum &PredLiveOut =
-          OutLocs[PredMBB->getNumber()][Idx.asU64()];
+      const ValueIDNum &PredLiveOut = OutLocs[*PredMBB][Idx.asU64()];
 
       // Incoming values agree, continue trying to eliminate this PHI.
       if (FirstVal == PredLiveOut)
@@ -2556,7 +2555,7 @@ void InstrRefBasedLDV::placeMLocPHIs(
 
   auto InstallPHIsAtLoc = [&PHIBlocks, &MInLocs](LocIdx L) {
     for (const MachineBasicBlock *MBB : PHIBlocks)
-      MInLocs[MBB->getNumber()][L.asU64()] = ValueIDNum(MBB->getNumber(), 0, L);
+      MInLocs[*MBB][L.asU64()] = ValueIDNum(MBB->getNumber(), 0, L);
   };
 
   // For locations with no reg units, just place PHIs.
@@ -2635,7 +2634,8 @@ void InstrRefBasedLDV::buildMLocValueMap(
 
   // Initialize entry block to PHIs. These represent arguments.
   for (auto Location : MTracker->locations())
-    MInLocs[0][Location.Idx.asU64()] = ValueIDNum(0, 0, Location.Idx);
+    MInLocs.tableForEntryMBB()[Location.Idx.asU64()] =
+        ValueIDNum(0, 0, Location.Idx);
 
   MTracker->reset();
 
@@ -2664,7 +2664,7 @@ void InstrRefBasedLDV::buildMLocValueMap(
 
       // Join the values in all predecessor blocks.
       bool InLocsChanged;
-      InLocsChanged = mlocJoin(*MBB, Visited, MOutLocs, MInLocs[CurBB]);
+      InLocsChanged = mlocJoin(*MBB, Visited, MOutLocs, MInLocs[*MBB]);
       InLocsChanged |= Visited.insert(MBB).second;
 
       // Don't examine transfer function if we've visited this loc at least
@@ -2673,7 +2673,7 @@ void InstrRefBasedLDV::buildMLocValueMap(
         continue;
 
       // Load the current set of live-ins into MLocTracker.
-      MTracker->loadFromArray(MInLocs[CurBB], CurBB);
+      MTracker->loadFromArray(MInLocs[*MBB], CurBB);
 
       // Each element of the transfer function can be a new def, or a read of
       // a live-in value. Evaluate each element, and store to "ToRemap".
@@ -2700,8 +2700,8 @@ void InstrRefBasedLDV::buildMLocValueMap(
       // the transfer function, and mlocJoin.
       bool OLChanged = false;
       for (auto Location : MTracker->locations()) {
-        OLChanged |= MOutLocs[CurBB][Location.Idx.asU64()] != Location.Value;
-        MOutLocs[CurBB][Location.Idx.asU64()] = Location.Value;
+        OLChanged |= MOutLocs[*MBB][Location.Idx.asU64()] != Location.Value;
+        MOutLocs[*MBB][Location.Idx.asU64()] = Location.Value;
       }
 
       MTracker->reset();
@@ -2844,7 +2844,6 @@ std::optional<ValueIDNum> InstrRefBasedLDV::pickOperandPHILoc(
   unsigned NumLocs = MTracker->getNumLocs();
 
   for (const auto p : BlockOrders) {
-    unsigned ThisBBNum = p->getNumber();
     auto OutValIt = LiveOuts.find(p);
     assert(OutValIt != LiveOuts.end());
     const DbgValue &OutVal = *OutValIt->second;
@@ -2863,7 +2862,7 @@ std::optional<ValueIDNum> InstrRefBasedLDV::pickOperandPHILoc(
       ValueIDNum ValToLookFor = OutValOp.ID;
       // Search the live-outs of the predecessor for the specified value.
       for (unsigned int I = 0; I < NumLocs; ++I) {
-        if (MOutLocs[ThisBBNum][I] == ValToLookFor)
+        if (MOutLocs[*p][I] == ValToLookFor)
           Locs.back().push_back(LocIdx(I));
       }
     } else {
@@ -2876,7 +2875,7 @@ std::optional<ValueIDNum> InstrRefBasedLDV::pickOperandPHILoc(
       // machine-value PHI locations.
       for (unsigned int I = 0; I < NumLocs; ++I) {
         ValueIDNum MPHI(MBB.getNumber(), 0, LocIdx(I));
-        if (MOutLocs[ThisBBNum][I] == MPHI)
+        if (MOutLocs[*p][I] == MPHI)
           Locs.back().push_back(LocIdx(I));
       }
     }
@@ -3498,19 +3497,15 @@ bool InstrRefBasedLDV::depthFirstVLocAndEmit(
   // Helper lambda for ejecting a block -- if nothing is going to use the block,
   // we can translate the variable location information into DBG_VALUEs and then
   // free all of InstrRefBasedLDV's data structures.
-  SmallPtrSet<const MachineBasicBlock *, 8> EjectedBBs;
   auto EjectBlock = [&](MachineBasicBlock &MBB) -> void {
-    if (EjectedBBs.insert(&MBB).second == false)
-      return;
     unsigned BBNum = MBB.getNumber();
     AllTheVLocs[BBNum].clear();
 
     // Prime the transfer-tracker, and then step through all the block
     // instructions, installing transfers.
     MTracker->reset();
-    MTracker->loadFromArray(MInLocs[BBNum], BBNum);
-    TTracker->loadInlocs(MBB, MInLocs[BBNum], DbgOpStore, Output[BBNum],
-                         NumLocs);
+    MTracker->loadFromArray(MInLocs[MBB], BBNum);
+    TTracker->loadInlocs(MBB, MInLocs[MBB], DbgOpStore, Output[BBNum], NumLocs);
 
     CurBB = BBNum;
     CurInst = 1;
@@ -3521,8 +3516,8 @@ bool InstrRefBasedLDV::depthFirstVLocAndEmit(
     }
 
     // Free machine-location tables for this block.
-    MInLocs[BBNum] = ValueTable();
-    MOutLocs[BBNum] = ValueTable();
+    MInLocs.ejectTableForBlock(MBB);
+    MOutLocs.ejectTableForBlock(MBB);
     // We don't need live-in variable values for this block either.
     Output[BBNum].clear();
     AllTheVLocs[BBNum].clear();
@@ -3587,7 +3582,8 @@ bool InstrRefBasedLDV::depthFirstVLocAndEmit(
   // anything for such out-of-scope blocks, but for the sake of being similar
   // to VarLocBasedLDV, eject these too.
   for (auto *MBB : ArtificialBlocks)
-    EjectBlock(*MBB);
+    if (MInLocs.hasTableFor(*MBB))
+      EjectBlock(*MBB);
 
   return emitTransfers(AllVarsNumbering);
 }
@@ -3686,8 +3682,8 @@ bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
   // machine values. The outer dimension is the block number; while the inner
   // dimension is a LocIdx from MLocTracker.
   unsigned NumLocs = MTracker->getNumLocs();
-  FuncValueTable MOutLocs(MaxNumBlocks, ValueTable(NumLocs));
-  FuncValueTable MInLocs(MaxNumBlocks, ValueTable(NumLocs));
+  FuncValueTable MOutLocs(MaxNumBlocks, NumLocs);
+  FuncValueTable MInLocs(MaxNumBlocks, NumLocs);
 
   // Solve the machine value dataflow problem using the MLocTransfer function,
   // storing the computed live-ins / live-outs into the array-of-arrays. We use
@@ -3725,7 +3721,7 @@ bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
     CurBB = MBB.getNumber();
     VTracker = &vlocs[CurBB];
     VTracker->MBB = &MBB;
-    MTracker->loadFromArray(MInLocs[CurBB], CurBB);
+    MTracker->loadFromArray(MInLocs[MBB], CurBB);
     CurInst = 1;
     for (auto &MI : MBB) {
       process(MI, &MOutLocs, &MInLocs);
@@ -3939,7 +3935,7 @@ class LDVSSAUpdater {
   /// Find the live-in value number for the given block. Looks up the value at
   /// the PHI location on entry.
   BlockValueNum getValue(LDVSSABlock *LDVBB) {
-    return MLiveIns[LDVBB->BB.getNumber()][Loc.asU64()].asU64();
+    return MLiveIns[LDVBB->BB][Loc.asU64()].asU64();
   }
 };
 
@@ -4179,8 +4175,7 @@ std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
   });
 
   for (auto &PHI : SortedPHIs) {
-    ValueIDNum ThisBlockValueNum =
-        MLiveIns[PHI->ParentBlock->BB.getNumber()][Loc.asU64()];
+    ValueIDNum ThisBlockValueNum = MLiveIns[PHI->ParentBlock->BB][Loc.asU64()];
 
     // Are all these things actually defined?
     for (auto &PHIIt : PHI->IncomingValues) {
@@ -4189,7 +4184,7 @@ std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
         return std::nullopt;
 
       ValueIDNum ValueToCheck;
-      const ValueTable &BlockLiveOuts = MLiveOuts[PHIIt.first->BB.getNumber()];
+      const ValueTable &BlockLiveOuts = MLiveOuts[PHIIt.first->BB];
 
       auto VVal = ValidatedValues.find(PHIIt.first);
       if (VVal == ValidatedValues.end()) {
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
@@ -207,9 +207,48 @@ using namespace llvm;
 /// Type for a table of values in a block.
 using ValueTable = SmallVector<ValueIDNum, 0>;
 
-/// Type for a table-of-table-of-values, i.e., the collection of either
-/// live-in or live-out values for each block in the function.
-using FuncValueTable = SmallVector<ValueTable, 0>;
+/// A collection of ValueTables, one per BB in a function, with convenient
+/// accessor methods.
+struct FuncValueTable {
+  FuncValueTable(int NumBBs, int NumLocs) {
+    Storage.reserve(NumBBs);
+    for (int i = 0; i != NumBBs; ++i)
+      Storage.push_back(
+          std::make_unique<ValueTable>(NumLocs, ValueIDNum::EmptyValue));
+  }
+
+  /// Returns the ValueTable associated with MBB.
+  ValueTable &operator[](const MachineBasicBlock &MBB) const {
+    return (*this)[MBB.getNumber()];
+  }
+
+  /// Returns the ValueTable associated with the MachineBasicBlock whose number
+  /// is MBBNum.
+  ValueTable &operator[](int MBBNum) const {
+    auto &TablePtr = Storage[MBBNum];
+    assert(TablePtr && "Trying to access a deleted table");
+    return *TablePtr;
+  }
+
+  /// Returns the ValueTable associated with the entry MachineBasicBlock.
+  ValueTable &tableForEntryMBB() const { return (*this)[0]; }
+
+  /// Returns true if the ValueTable associated with MBB has not been freed.
+  bool hasTableFor(MachineBasicBlock &MBB) const {
+    return Storage[MBB.getNumber()] != nullptr;
+  }
+
+  /// Frees the memory of the ValueTable associated with MBB.
+  void ejectTableForBlock(const MachineBasicBlock &MBB) {
+    Storage[MBB.getNumber()].reset();
+  }
+
+private:
+  /// ValueTables are stored as unique_ptrs to allow for deallocation during
+  /// LDV; this was measured to have a significant impact on compiler memory
+  /// usage.
+  SmallVector<std::unique_ptr<ValueTable>, 0> Storage;
+};
 
 /// Thin wrapper around an integer -- designed to give more type safety to
 /// spill location numbers.
diff --git a/llvm/unittests/CodeGen/InstrRefLDVTest.cpp b/llvm/unittests/CodeGen/InstrRefLDVTest.cpp
@@ -497,8 +497,7 @@ body:  |
 
   std::pair<FuncValueTable, FuncValueTable>
   allocValueTables(unsigned Blocks, unsigned Locs) {
-    return {FuncValueTable(Blocks, ValueTable(Locs)),
-            FuncValueTable(Blocks, ValueTable(Locs))};
+    return {FuncValueTable(Blocks, Locs), FuncValueTable(Blocks, Locs)};
   }
 };
 
@@ -917,8 +916,7 @@ TEST_F(InstrRefLDVTest, MLocSingleBlock) {
 
   // Set up live-in and live-out tables for this function: two locations (we
   // add one later) in a single block.
-  FuncValueTable MOutLocs, MInLocs;
-  std::tie(MOutLocs, MInLocs) = allocValueTables(1, 2);
+  auto [MOutLocs, MInLocs] = allocValueTables(1, 2);
 
   // Transfer function: nothing.
   SmallVector<MLocTransferMap, 1> TransferFunc;
@@ -983,8 +981,7 @@ TEST_F(InstrRefLDVTest, MLocDiamondBlocks) {
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->lookupOrTrackRegister(RAX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(4, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(4, 2);
 
   // Transfer function: start with nothing.
   SmallVector<MLocTransferMap, 1> TransferFunc;
@@ -1137,8 +1134,7 @@ TEST_F(InstrRefLDVTest, MLocDiamondSpills) {
   // There are other locations, for things like xmm0, which we're going to
   // ignore here.
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(4, 11);
+  auto [MInLocs, MOutLocs] = allocValueTables(4, 11);
 
   // Transfer function: start with nothing.
   SmallVector<MLocTransferMap, 1> TransferFunc;
@@ -1199,8 +1195,7 @@ TEST_F(InstrRefLDVTest, MLocSimpleLoop) {
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->lookupOrTrackRegister(RAX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(3, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(3, 2);
 
   SmallVector<MLocTransferMap, 1> TransferFunc;
   TransferFunc.resize(3);
@@ -1298,8 +1293,7 @@ TEST_F(InstrRefLDVTest, MLocNestedLoop) {
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->lookupOrTrackRegister(RAX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(5, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(5, 2);
 
   SmallVector<MLocTransferMap, 1> TransferFunc;
   TransferFunc.resize(5);
@@ -1500,8 +1494,7 @@ TEST_F(InstrRefLDVTest, MLocNoDominatingLoop) {
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->lookupOrTrackRegister(RAX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(5, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(5, 2);
 
   SmallVector<MLocTransferMap, 1> TransferFunc;
   TransferFunc.resize(5);
@@ -1656,8 +1649,7 @@ TEST_F(InstrRefLDVTest, MLocBadlyNestedLoops) {
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->lookupOrTrackRegister(RAX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(5, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(5, 2);
 
   SmallVector<MLocTransferMap, 1> TransferFunc;
   TransferFunc.resize(5);
@@ -1789,8 +1781,7 @@ TEST_F(InstrRefLDVTest, pickVPHILocDiamond) {
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->lookupOrTrackRegister(RAX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(4, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(4, 2);
 
   initValueArray(MOutLocs, 4, 2);
 
@@ -1986,8 +1977,7 @@ TEST_F(InstrRefLDVTest, pickVPHILocLoops) {
   Register RAX = getRegByName("RAX");
   LocIdx RaxLoc = MTracker->lookupOrTrackRegister(RAX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(3, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(3, 2);
 
   initValueArray(MOutLocs, 3, 2);
 
@@ -2117,8 +2107,7 @@ TEST_F(InstrRefLDVTest, pickVPHILocBadlyNestedLoops) {
   Register RBX = getRegByName("RBX");
   LocIdx RbxLoc = MTracker->lookupOrTrackRegister(RBX);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(5, 3);
+  auto [MInLocs, MOutLocs] = allocValueTables(5, 3);
 
   initValueArray(MOutLocs, 5, 3);
 
@@ -2635,8 +2624,7 @@ TEST_F(InstrRefLDVTest, VLocSingleBlock) {
   ASSERT_TRUE(MTracker->getNumLocs() == 1);
   LocIdx RspLoc(0);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(1, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(1, 2);
 
   ValueIDNum LiveInRsp = ValueIDNum(0, 0, RspLoc);
   DbgOpID LiveInRspID = addValueDbgOp(LiveInRsp);
@@ -2699,8 +2687,7 @@ TEST_F(InstrRefLDVTest, VLocDiamondBlocks) {
   DbgOpID LiveInRaxID = addValueDbgOp(LiveInRax);
   DbgOpID RspPHIInBlk3ID = addValueDbgOp(RspPHIInBlk3);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(4, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(4, 2);
 
   initValueArray(MInLocs, 4, 2);
   initValueArray(MOutLocs, 4, 2);
@@ -2921,8 +2908,7 @@ TEST_F(InstrRefLDVTest, VLocSimpleLoop) {
   DbgOpID RspDefInBlk1ID = addValueDbgOp(RspDefInBlk1);
   DbgOpID RaxPHIInBlk1ID = addValueDbgOp(RaxPHIInBlk1);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(3, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(3, 2);
 
   initValueArray(MInLocs, 3, 2);
   initValueArray(MOutLocs, 3, 2);
@@ -3200,8 +3186,7 @@ TEST_F(InstrRefLDVTest, VLocNestedLoop) {
   DbgOpID RspPHIInBlk2ID = addValueDbgOp(RspPHIInBlk2);
   DbgOpID RspDefInBlk2ID = addValueDbgOp(RspDefInBlk2);
 
-  FuncValueTable MInLocs, MOutLocs;
-  std::tie(MInLocs, MOutLocs) = allocValueTables(5, 2);
+  auto [MInLocs, MOutLocs] = allocValueTables(5, 2);
 
   initValueArray(MInLocs, 5, 2);
   initValueArray(MOutLocs, 5, 2);
