[InstrRef][nfc] Remove usage of unique_ptrs of arrays #74203
Conversation
|
@llvm/pr-subscribers-debuginfo Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThese are usually difficult to reason about, and they were being used to pass raw pointers around with array semantic (i.e., we were using operator [] on raw pointers). To put it in InstrRef terminology: we were passing a pointer to a ValueTable but using it as if it were a FuncValueTable. These could have easily been SmallVectors, which now allow us to have reference semantics in some places, as well as simpler initialization. In the future, we can use even more pass-by-reference with some extra changes in the code. Full diff: https://github.com/llvm/llvm-project/pull/74203.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index b2c2b40139eda..cb0954dc544eb 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -1429,7 +1429,7 @@ bool InstrRefBasedLDV::transferDebugValue(const MachineInstr &MI) {
std::optional<ValueIDNum> InstrRefBasedLDV::getValueForInstrRef(
unsigned InstNo, unsigned OpNo, MachineInstr &MI,
- const ValueTable *MLiveOuts, const ValueTable *MLiveIns) {
+ const FuncValueTable *MLiveOuts, const FuncValueTable *MLiveIns) {
// Various optimizations may have happened to the value during codegen,
// recorded in the value substitution table. Apply any substitutions to
// the instruction / operand number in this DBG_INSTR_REF, and collect
@@ -1495,7 +1495,8 @@ std::optional<ValueIDNum> InstrRefBasedLDV::getValueForInstrRef(
} else if (PHIIt != DebugPHINumToValue.end() && PHIIt->InstrNum == InstNo) {
// It's actually a PHI value. Which value it is might not be obvious, use
// the resolver helper to find out.
- NewID = resolveDbgPHIs(*MI.getParent()->getParent(), MLiveOuts, MLiveIns,
+ assert(MLiveOuts && MLiveIns);
+ NewID = resolveDbgPHIs(*MI.getParent()->getParent(), *MLiveOuts, *MLiveIns,
MI, InstNo);
}
@@ -1574,8 +1575,8 @@ std::optional<ValueIDNum> InstrRefBasedLDV::getValueForInstrRef(
}
bool InstrRefBasedLDV::transferDebugInstrRef(MachineInstr &MI,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns) {
+ const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns) {
if (!MI.isDebugRef())
return false;
@@ -2245,8 +2246,8 @@ void InstrRefBasedLDV::accumulateFragmentMap(MachineInstr &MI) {
AllSeenFragments.insert(ThisFragment);
}
-void InstrRefBasedLDV::process(MachineInstr &MI, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns) {
+void InstrRefBasedLDV::process(MachineInstr &MI, const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns) {
// Try to interpret an MI as a debug or transfer instruction. Only if it's
// none of these should we interpret it's register defs as new value
// definitions.
@@ -3503,7 +3504,10 @@ 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();
@@ -3517,14 +3521,14 @@ bool InstrRefBasedLDV::depthFirstVLocAndEmit(
CurBB = BBNum;
CurInst = 1;
for (auto &MI : MBB) {
- process(MI, MOutLocs.get(), MInLocs.get());
+ process(MI, &MOutLocs, &MInLocs);
TTracker->checkInstForNewValues(CurInst, MI.getIterator());
++CurInst;
}
// Free machine-location tables for this block.
- MInLocs[BBNum].reset();
- MOutLocs[BBNum].reset();
+ MInLocs[BBNum] = ValueTable();
+ MOutLocs[BBNum] = ValueTable();
// We don't need live-in variable values for this block either.
Output[BBNum].clear();
AllTheVLocs[BBNum].clear();
@@ -3589,8 +3593,7 @@ 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)
- if (MOutLocs[MBB->getNumber()])
- EjectBlock(*MBB);
+ EjectBlock(*MBB);
return emitTransfers(AllVarsNumbering);
}
@@ -3688,14 +3691,9 @@ bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
// Allocate and initialize two array-of-arrays for the live-in and live-out
// machine values. The outer dimension is the block number; while the inner
// dimension is a LocIdx from MLocTracker.
- FuncValueTable MOutLocs = std::make_unique<ValueTable[]>(MaxNumBlocks);
- FuncValueTable MInLocs = std::make_unique<ValueTable[]>(MaxNumBlocks);
unsigned NumLocs = MTracker->getNumLocs();
- for (int i = 0; i < MaxNumBlocks; ++i) {
- // These all auto-initialize to ValueIDNum::EmptyValue
- MOutLocs[i] = std::make_unique<ValueIDNum[]>(NumLocs);
- MInLocs[i] = std::make_unique<ValueIDNum[]>(NumLocs);
- }
+ FuncValueTable MOutLocs(MaxNumBlocks, ValueTable(NumLocs));
+ FuncValueTable MInLocs(MaxNumBlocks, ValueTable(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
@@ -3736,7 +3734,7 @@ bool InstrRefBasedLDV::ExtendRanges(MachineFunction &MF,
MTracker->loadFromArray(MInLocs[CurBB], CurBB);
CurInst = 1;
for (auto &MI : MBB) {
- process(MI, MOutLocs.get(), MInLocs.get());
+ process(MI, &MOutLocs, &MInLocs);
++CurInst;
}
MTracker->reset();
@@ -3917,9 +3915,9 @@ class LDVSSAUpdater {
/// Machine location where any PHI must occur.
LocIdx Loc;
/// Table of live-in machine value numbers for blocks / locations.
- const ValueTable *MLiveIns;
+ const FuncValueTable &MLiveIns;
- LDVSSAUpdater(LocIdx L, const ValueTable *MLiveIns)
+ LDVSSAUpdater(LocIdx L, const FuncValueTable &MLiveIns)
: Loc(L), MLiveIns(MLiveIns) {}
void reset() {
@@ -4075,12 +4073,8 @@ template <> class SSAUpdaterTraits<LDVSSAUpdater> {
} // end namespace llvm
std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIs(
- MachineFunction &MF, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
- assert(MLiveOuts && MLiveIns &&
- "Tried to resolve DBG_PHI before location "
- "tables allocated?");
-
+ MachineFunction &MF, const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
// This function will be called twice per DBG_INSTR_REF, and might end up
// computing lots of SSA information: memoize it.
auto SeenDbgPHIIt = SeenDbgPHIs.find(std::make_pair(&Here, InstrNum));
@@ -4094,8 +4088,8 @@ std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIs(
}
std::optional<ValueIDNum> InstrRefBasedLDV::resolveDbgPHIsImpl(
- MachineFunction &MF, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
+ MachineFunction &MF, const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns, MachineInstr &Here, uint64_t InstrNum) {
// Pick out records of DBG_PHI instructions that have been observed. If there
// are none, then we cannot compute a value number.
auto RangePair = std::equal_range(DebugPHINumToValue.begin(),
diff --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
index 5e94962f9d7e6..d6dbb1feda3e8 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
@@ -205,11 +205,11 @@ namespace LiveDebugValues {
using namespace llvm;
/// Type for a table of values in a block.
-using ValueTable = std::unique_ptr<ValueIDNum[]>;
+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 = std::unique_ptr<ValueTable[]>;
+using FuncValueTable = SmallVector<ValueTable, 0>;
/// Thin wrapper around an integer -- designed to give more type safety to
/// spill location numbers.
@@ -1200,12 +1200,12 @@ class InstrRefBasedLDV : public LDVImpl {
/// exists, otherwise returns std::nullopt.
std::optional<ValueIDNum> getValueForInstrRef(unsigned InstNo, unsigned OpNo,
MachineInstr &MI,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns);
+ const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns);
/// Observe a single instruction while stepping through a block.
- void process(MachineInstr &MI, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns);
+ void process(MachineInstr &MI, const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns);
/// Examines whether \p MI is a DBG_VALUE and notifies trackers.
/// \returns true if MI was recognized and processed.
@@ -1213,8 +1213,8 @@ class InstrRefBasedLDV : public LDVImpl {
/// Examines whether \p MI is a DBG_INSTR_REF and notifies trackers.
/// \returns true if MI was recognized and processed.
- bool transferDebugInstrRef(MachineInstr &MI, const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns);
+ bool transferDebugInstrRef(MachineInstr &MI, const FuncValueTable *MLiveOuts,
+ const FuncValueTable *MLiveIns);
/// Stores value-information about where this PHI occurred, and what
/// instruction number is associated with it.
@@ -1246,14 +1246,14 @@ class InstrRefBasedLDV : public LDVImpl {
/// \p InstrNum Debug instruction number defined by DBG_PHI instructions.
/// \returns The machine value number at position Here, or std::nullopt.
std::optional<ValueIDNum> resolveDbgPHIs(MachineFunction &MF,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns,
+ const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns,
MachineInstr &Here,
uint64_t InstrNum);
std::optional<ValueIDNum> resolveDbgPHIsImpl(MachineFunction &MF,
- const ValueTable *MLiveOuts,
- const ValueTable *MLiveIns,
+ const FuncValueTable &MLiveOuts,
+ const FuncValueTable &MLiveIns,
MachineInstr &Here,
uint64_t InstrNum);
diff --git a/llvm/unittests/CodeGen/InstrRefLDVTest.cpp b/llvm/unittests/CodeGen/InstrRefLDVTest.cpp
index 5a050bc520362..acbcd247fa9a0 100644
--- a/llvm/unittests/CodeGen/InstrRefLDVTest.cpp
+++ b/llvm/unittests/CodeGen/InstrRefLDVTest.cpp
@@ -497,15 +497,8 @@ body: |
std::pair<FuncValueTable, FuncValueTable>
allocValueTables(unsigned Blocks, unsigned Locs) {
- FuncValueTable MOutLocs = std::make_unique<ValueTable[]>(Blocks);
- FuncValueTable MInLocs = std::make_unique<ValueTable[]>(Blocks);
-
- for (unsigned int I = 0; I < Blocks; ++I) {
- MOutLocs[I] = std::make_unique<ValueIDNum[]>(Locs);
- MInLocs[I] = std::make_unique<ValueIDNum[]>(Locs);
- }
-
- return std::make_pair(std::move(MOutLocs), std::move(MInLocs));
+ return {FuncValueTable(Blocks, ValueTable(Locs)),
+ FuncValueTable(Blocks, ValueTable(Locs))};
}
};
|
|
| using FuncValueTable = std::unique_ptr<ValueTable[]>; | ||
| using FuncValueTable = SmallVector<ValueTable, 0>; |
There was a problem hiding this comment.
(personally, I find code easier to read when it doesn't obscure these outer templates like unique_ptr or SmallVector (ie: I'd rather avoid/remove these aliases and have code refer to SmallVector<ValueTable, 0> directly) - and the name might be a bit misleading too, if we're going to keep it, FuncValueTable - it isn't a single value table, it's a sequence of them - so like ValueTableVector? (at least that hints at it being a sequence, which is helpful - I guess the old name couldn't do that because unique_ptr<T[]> doesn't have begin/end or even a length, etc))
There was a problem hiding this comment.
I agree.
@jmorse would you be ok if I removed the FuncValueTable typedef and only kept ValueTable?
This would also allow us to pass MutableArrayRefs around instead of SmallVectors
There was a problem hiding this comment.
This seems fine to me and is probably clearer than the array-allocation situation. I've added a question about a potentially functional change / code-smell inline.
(I wrote the unique_ptr stuff in D118774 after having been bitten by asan too many times, in a bid to escape further manual memory allocation).
| SmallPtrSet<const MachineBasicBlock *, 8> EjectedBBs; | ||
| auto EjectBlock = [&](MachineBasicBlock &MBB) -> void { | ||
| if (EjectedBBs.insert(&MBB).second == false) | ||
| return; |
There was a problem hiding this comment.
My recollection of this (which may be faulty) is that blocks should only get ejected once, in the order determined just above this block (it reduces the working-set size during compilation). If the added code fires I'd suggest there's a bug somewhere else, is there a reproducer for that? I'd imagine it'd only reduce performance a bit.
There was a problem hiding this comment.
The rationale for this code is because I removed the check on line 3596 of the "green" side of the diff.
I.e., before:
for (auto *MBB : ArtificialBlocks)
if (MOutLocs[MBB->getNumber()])
EjectBlock(*MBB);
After:
for (auto *MBB : ArtificialBlocks)
EjectBlock(*MBB);
Prior to this change, we were testing whether something had been ejected by querying whether we had called "reset" on the unique pointer. There are no unique pointers anymore, and checking for whether the length of the vector is 0 not the equivalent, so the only solution was to add this set.
So, with this mind, the patch is 100% intended to be NFC. Do you agree with that?
There was a problem hiding this comment.
Ahhh, I see the rationale. I'm struggling to remember where that came from -- I thought it should be the case that the set of ArtificialBlocks and the set of blocks produced by makeDepthFirstEjectionMap shouldn't overlap anyway because the artificial blocks shouldn't be in any lexical scopes? But maybe they can, as LexicalScopes only stores a start/end block-number pair.
Either way, I'm not confident enough to say the is-null test isn't double-ejection prevention so it should be kept. For performance, could you use a SmallDenseMap instead of SmallPtrSet with a call to reserve as we'll know the maximum number of blocks that might get inserted. That limits the worst-case map-growth to a single allocation while providing a local scratchpad for small functions.
There was a problem hiding this comment.
I'm not confident enough to say the is-null test isn't double-ejection prevention so it should be kept
I think there may be some miscommunication going here: the check is kept, it just takes a different form.
I moved it inside the lambda, and instead of checking for a nullptr (pointers don't exist anymore, so we can't check that), I am using an "has been ejected" check.
Does that make sense?
There was a problem hiding this comment.
Makes perfect sense and you're correct; I'm just second-guessing whether the check had a different purpose, or was something I'd gratuitously added in the past without reason.
| using FuncValueTable = std::unique_ptr<ValueTable[]>; | ||
| using FuncValueTable = SmallVector<ValueTable, 0>; |
I'll add the reserve call! |
These are usually difficult to reason about, and they were being used to pass raw pointers around with array semantic (i.e., we were using operator [] on raw pointers). To put it in InstrRef terminology: we were passing a pointer to a ValueTable but using it as if it were a FuncValueTable. These could have easily been SmallVectors, which now allow us to have reference semantics in some places, as well as simpler initialization. In the future, we can use even more pass-by-reference with some extra changes in the code.
So it felt a bit wrong to use a
|
felipepiovezan
force-pushed
the
felipe/remove_unique_ptrs
branch
from
0e36c60 to
b7f8032
Compare
|
Fixed formatter issue |
|
I'll go ahead and merge this, but happy to address the reserve issue afterwards if you disagree with my assessment from above. |
|
Ah, apologies, looks like I intended on writing "SmallSenseSet" but wrote "SmallDenseMap" by mistake -- apologies. Whichever set is probably fine, if there's any serious problems it'll show up in a profiler. |
|
FYI this seems to significantly increase memory usage: http://llvm-compile-time-tracker.com/compare.php?from=0d02ecc6bddc5e8c8bacbae3ab72a10467e177b4&to=1b531d54f6234488dec048fec1c5aca331bf8f3d&stat=max-rss&linkStats=on (look at |
That's very surprising. I'll investigate |
How is the compiler itself built for this? IIUC the configurations refer to how the tests are built, but not how the compiler itself is built. Also, the regressions seem to happen in the link stage? edit: Ah, this is LTO, so probably one huge module at the end of the compilation flow |
|
@nikic could you show me how the memory usage is being measured? The tools I used, namely |
|
max-rss is measured using It's possible that there is some platform-specific difference here (libstdc++ vs libc++), but it seems unlikely that that this causes a 10% regression on Linux and no change on MacOS. |
This is what I tried when repro-ing that experiment. |
Are you sure But then running this under I also tried manually linking all the LTO object files with Without patch: In these traces I see InstrRefBasedImpl.cpp show up quite a bit (unlike the clang ones). Also note how here the peak memory is around 100MB instead of KBs. The difference is quite small between the two. |
|
You need to pass I just tested this myself locally, and I can reproduce the difference. The Here are the massif results for the two runs: https://gist.github.com/nikic/bfdd7686c0d6c1ddc076d527148da851 These show a peak of 72MB vs 84MB. |
Thank you! With |
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.
|
Fixes in #76051 |
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.
These are usually difficult to reason about, and they were being used to pass raw pointers around with array semantic (i.e., we were using operator [] on raw pointers). To put it in InstrRef terminology: we were passing a pointer to a ValueTable but using it as if it were a FuncValueTable.
These could have easily been SmallVectors, which now allow us to have reference semantics in some places, as well as simpler initialization.
In the future, we can use even more pass-by-reference with some extra changes in the code.