[LiveDebugValues][nfc] Reduce memory usage of InstrRef #76051
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit 1b531d5 (llvm#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.
|
@llvm/pr-subscribers-debuginfo Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesCommit 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 This commit addresses both issues by making 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. Full diff: https://github.com/llvm/llvm-project/pull/76051.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index aeb8a20e1f1221..9037f752dc4f36 100644
--- 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
index d6dbb1feda3e8e..65de4de4017925 100644
--- 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 ([[maybe_unused]] auto _ : llvm::seq(NumBBs))
+ Storage.push_back(
+ std::make_unique<ValueTable>(NumLocs, ValueIDNum::EmptyValue));
+ }
+
+ /// Returns the ValueTable associated with MBB.
+ ValueTable &operator[](const MachineBasicBlock &MBB) const {
+ return this->operator[](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->operator[](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.
|
dwblaikie
reviewed
Dec 20, 2023
| struct FuncValueTable { | ||
| FuncValueTable(int NumBBs, int NumLocs) { | ||
| Storage.reserve(NumBBs); | ||
| for ([[maybe_unused]] auto _ : llvm::seq(NumBBs)) |
There was a problem hiding this comment.
I think this'd be better written as a:
for (int i = 0; i != NumBBs; ++i)
Creating a sequence, iterators, etc, and then discarding the value iterated over and having to add [[maybe_unused]] seems a bit too far into the weeds to me.
There was a problem hiding this comment.
I got very close to writing this instead 😅
map_to_vector(seq(NumBBs),
[](auto){return std::make_unique<ValueTable>(NumLocs, ValueIDNum::EmptyValue));});
But I don't mind rewriting it as a traditional style loop. Will update the pr shortly
|
|
||
| /// Returns the ValueTable associated with MBB. | ||
| ValueTable &operator[](const MachineBasicBlock &MBB) const { | ||
| return this->operator[](MBB.getNumber()); |
There was a problem hiding this comment.
Up to you, but I'd probably write this as (*this)[MBB.getNumber()] - bit shorter?
jmorse
approved these changes
Dec 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FuncValueTabletypedef could be removed, since it was "just a vector".This commit addresses both issues by making
FuncValueTablea 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.