Skip to content

Commit 2fa0591

Browse files
[MemProf] Use remove_if to erase MapVector elements in bulk (#94269)
A cycle profile showed that we were spending a lot of time invoking MapVector::erase. According to https://llvm.org/docs/ProgrammersManual.html#llvm-adt-mapvector-h, erasing elements one at a time is very inefficient for MapVector and it is better to use remove_if. This change resulted in around 7% time reduction on a large thin link. While here remove an unused function that also invokes erase on MapVectors.
1 parent d48d108 commit 2fa0591

File tree

1 file changed

+8
-18
lines changed

1 file changed

+8
-18
lines changed

llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp

+8-18
Original file line numberDiff line numberDiff line change
@@ -505,9 +505,6 @@ class CallsiteContextGraph {
505505
ContextNode *getNodeForAlloc(const CallInfo &C);
506506
ContextNode *getNodeForStackId(uint64_t StackId);
507507

508-
/// Removes the node information recorded for the given call.
509-
void unsetNodeForInst(const CallInfo &C);
510-
511508
/// Computes the alloc type corresponding to the given context ids, by
512509
/// unioning their recorded alloc types.
513510
uint8_t computeAllocType(DenseSet<uint32_t> &ContextIds);
@@ -813,15 +810,6 @@ CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::getNodeForStackId(
813810
return nullptr;
814811
}
815812

816-
template <typename DerivedCCG, typename FuncTy, typename CallTy>
817-
void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::unsetNodeForInst(
818-
const CallInfo &C) {
819-
AllocationCallToContextNodeMap.erase(C) ||
820-
NonAllocationCallToContextNodeMap.erase(C);
821-
assert(!AllocationCallToContextNodeMap.count(C) &&
822-
!NonAllocationCallToContextNodeMap.count(C));
823-
}
824-
825813
template <typename DerivedCCG, typename FuncTy, typename CallTy>
826814
void CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::ContextNode::
827815
addOrUpdateCallerEdge(ContextNode *Caller, AllocationType AllocType,
@@ -1694,11 +1682,10 @@ void CallsiteContextGraph<DerivedCCG, FuncTy,
16941682
MapVector<CallInfo, ContextNode *> TailCallToContextNodeMap;
16951683

16961684
for (auto Entry = NonAllocationCallToContextNodeMap.begin();
1697-
Entry != NonAllocationCallToContextNodeMap.end();) {
1685+
Entry != NonAllocationCallToContextNodeMap.end(); Entry++) {
16981686
auto *Node = Entry->second;
16991687
assert(Node->Clones.empty());
17001688
// Check all node callees and see if in the same function.
1701-
bool Removed = false;
17021689
auto Call = Node->Call.call();
17031690
for (auto EI = Node->CalleeEdges.begin(); EI != Node->CalleeEdges.end();) {
17041691
auto Edge = *EI;
@@ -1714,15 +1701,18 @@ void CallsiteContextGraph<DerivedCCG, FuncTy,
17141701
// Work around by setting Node to have a null call, so it gets
17151702
// skipped during cloning. Otherwise assignFunctions will assert
17161703
// because its data structures are not designed to handle this case.
1717-
Entry = NonAllocationCallToContextNodeMap.erase(Entry);
17181704
Node->setCall(CallInfo());
1719-
Removed = true;
17201705
break;
17211706
}
1722-
if (!Removed)
1723-
Entry++;
17241707
}
17251708

1709+
// Remove all mismatched nodes identified in the above loop from the node map
1710+
// (checking whether they have a null call which is set above). For a
1711+
// MapVector like NonAllocationCallToContextNodeMap it is much more efficient
1712+
// to do the removal via remove_if than by individually erasing entries above.
1713+
NonAllocationCallToContextNodeMap.remove_if(
1714+
[](const auto &it) { return !it.second->hasCall(); });
1715+
17261716
// Add the new nodes after the above loop so that the iteration is not
17271717
// invalidated.
17281718
for (auto &[Call, Node] : TailCallToContextNodeMap)

0 commit comments

Comments
 (0)