From 9d327649ef879dc9aafa88e4232362d06a982796 Mon Sep 17 00:00:00 2001 From: Kazu Hirata Date: Fri, 13 Dec 2024 12:24:47 -0800 Subject: [PATCH 1/3] [memprof] Simplify readMemprof (NFC) This patch essentially replaces: std::pair *, unsigned> with: ArrayRef This way, we can store and pass ArrayRef, conceptually one item, instead of the pointer and index. The only problem is that std::set> doesn't work because ArrayRef doesn't come with operator<. This patch works around the problem by providing CallStackRef, a thin wrapper around ArrayRef. --- .../Instrumentation/MemProfiler.cpp | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp index ab7d3c7002991..03c9c64e8a1db 100644 --- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp +++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp @@ -763,9 +763,8 @@ static AllocationType addCallStack(CallStackTrie &AllocTrie, // non-zero. static bool stackFrameIncludesInlinedCallStack(ArrayRef ProfileCallStack, - ArrayRef InlinedCallStack, - unsigned StartIndex = 0) { - auto StackFrame = ProfileCallStack.begin() + StartIndex; + ArrayRef InlinedCallStack) { + auto StackFrame = ProfileCallStack.begin(); auto InlCallStackIter = InlinedCallStack.begin(); for (; StackFrame != ProfileCallStack.end() && InlCallStackIter != InlinedCallStack.end(); @@ -969,10 +968,20 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, // Build maps of the location hash to all profile data with that leaf location // (allocation info and the callsites). std::map> LocHashToAllocInfo; - // For the callsites we need to record the index of the associated frame in - // the frame array (see comments below where the map entries are added). - std::map *, unsigned>>> - LocHashToCallSites; + // A thin wrapper around ArrayRef to facilitate std::set. + struct CallStackRef : public ArrayRef { + CallStackRef(ArrayRef CS, unsigned Pos) + : ArrayRef(CS.drop_front(Pos)) {} + // std::set requires std::less. + bool operator<(const CallStackRef &R) const { + const CallStackRef &L = *this; + return std::make_pair(L.data(), L.size()) < + std::make_pair(R.data(), R.size()); + } + }; + // For the callsites we need to record slices of the frame array (see comments + // below where the map entries are added). + std::map> LocHashToCallSites; for (auto &AI : MemProfRec->AllocSites) { NumOfMemProfAllocContextProfiles++; // Associate the allocation info with the leaf frame. The later matching @@ -989,7 +998,7 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, unsigned Idx = 0; for (auto &StackFrame : CS) { uint64_t StackId = computeStackId(StackFrame); - LocHashToCallSites[StackId].insert(std::make_pair(&CS, Idx++)); + LocHashToCallSites[StackId].emplace(CS, Idx++); ProfileHasColumns |= StackFrame.Column; // Once we find this function, we can stop recording. if (StackFrame.Function == FuncGUID) @@ -1029,8 +1038,7 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, // and another callsite). std::map>::iterator AllocInfoIter; - std::map *, - unsigned>>>::iterator CallSitesIter; + std::map>::iterator CallSitesIter; for (const DILocation *DIL = I.getDebugLoc(); DIL != nullptr; DIL = DIL->getInlinedAt()) { // Use C++ linkage name if possible. Need to compile with @@ -1121,8 +1129,8 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, for (auto CallStackIdx : CallSitesIter->second) { // If we found and thus matched all frames on the call, create and // attach call stack metadata. - if (stackFrameIncludesInlinedCallStack( - *CallStackIdx.first, InlinedCallStack, CallStackIdx.second)) { + if (stackFrameIncludesInlinedCallStack(CallStackIdx, + InlinedCallStack)) { NumOfMemProfMatchedCallSites++; addCallsiteMetadata(I, InlinedCallStack, Ctx); // Only need to find one with a matching call stack and add a single From 4dba1363c3dd2c43c4a7ff395b82d4455a4606f4 Mon Sep 17 00:00:00 2001 From: Kazu Hirata Date: Fri, 13 Dec 2024 16:05:46 -0800 Subject: [PATCH 2/3] Address comments. --- .../Instrumentation/MemProfiler.cpp | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp index 03c9c64e8a1db..0b310d9c8a01b 100644 --- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp +++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp @@ -725,8 +725,7 @@ static uint64_t computeStackId(const memprof::Frame &Frame) { // Helper to generate a single hash id for a given callstack, used for emitting // matching statistics and useful for uniquing such statistics across modules. -static uint64_t -computeFullStackId(const std::vector &CallStack) { +static uint64_t computeFullStackId(ArrayRef CallStack) { llvm::HashBuilder, llvm::endianness::little> HashBuilder; for (auto &F : CallStack) @@ -968,20 +967,16 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, // Build maps of the location hash to all profile data with that leaf location // (allocation info and the callsites). std::map> LocHashToAllocInfo; - // A thin wrapper around ArrayRef to facilitate std::set. - struct CallStackRef : public ArrayRef { - CallStackRef(ArrayRef CS, unsigned Pos) - : ArrayRef(CS.drop_front(Pos)) {} - // std::set requires std::less. - bool operator<(const CallStackRef &R) const { - const CallStackRef &L = *this; - return std::make_pair(L.data(), L.size()) < - std::make_pair(R.data(), R.size()); + // A hash function for std::unordered_set> to work. + struct CallStackHash { + size_t operator()(ArrayRef CS) const { + return computeFullStackId(CS); } }; // For the callsites we need to record slices of the frame array (see comments // below where the map entries are added). - std::map> LocHashToCallSites; + std::map, CallStackHash>> + LocHashToCallSites; for (auto &AI : MemProfRec->AllocSites) { NumOfMemProfAllocContextProfiles++; // Associate the allocation info with the leaf frame. The later matching @@ -998,7 +993,8 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, unsigned Idx = 0; for (auto &StackFrame : CS) { uint64_t StackId = computeStackId(StackFrame); - LocHashToCallSites[StackId].emplace(CS, Idx++); + LocHashToCallSites[StackId].insert( + ArrayRef(CS).drop_front(Idx++)); ProfileHasColumns |= StackFrame.Column; // Once we find this function, we can stop recording. if (StackFrame.Function == FuncGUID) @@ -1038,7 +1034,7 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, // and another callsite). std::map>::iterator AllocInfoIter; - std::map>::iterator CallSitesIter; + decltype(LocHashToCallSites)::iterator CallSitesIter; for (const DILocation *DIL = I.getDebugLoc(); DIL != nullptr; DIL = DIL->getInlinedAt()) { // Use C++ linkage name if possible. Need to compile with From a790e98d514a7f585dc0f237f5e956b0d6407a58 Mon Sep 17 00:00:00 2001 From: Kazu Hirata Date: Fri, 13 Dec 2024 21:36:45 -0800 Subject: [PATCH 3/3] Address comments. --- llvm/lib/Transforms/Instrumentation/MemProfiler.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp index 0b310d9c8a01b..c980869a1c0d8 100644 --- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp +++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp @@ -993,8 +993,7 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader, unsigned Idx = 0; for (auto &StackFrame : CS) { uint64_t StackId = computeStackId(StackFrame); - LocHashToCallSites[StackId].insert( - ArrayRef(CS).drop_front(Idx++)); + LocHashToCallSites[StackId].insert(ArrayRef(CS).drop_front(Idx++)); ProfileHasColumns |= StackFrame.Column; // Once we find this function, we can stop recording. if (StackFrame.Function == FuncGUID)