[memprof] Simplify readMemprof (NFC) #119930
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
This patch essentially replaces: std::pair<const std::vector<Frame> *, unsigned> with: ArrayRef<Frame> This way, we can store and pass ArrayRef<Frame>, conceptually one item, instead of the pointer and index. The only problem is that std::set<ArrayRef<Frame>> doesn't work because ArrayRef doesn't come with operator<. This patch works around the problem by providing CallStackRef, a thin wrapper around ArrayRef<Frame>.
|
@llvm/pr-subscribers-llvm-transforms Author: Kazu Hirata (kazutakahirata) ChangesThis patch essentially replaces: std::pair<const std::vector<Frame> *, unsigned> with: ArrayRef<Frame> This way, we can store and pass ArrayRef<Frame>, conceptually one The only problem is that std::set<ArrayRef<Frame>> doesn't work Full diff: https://github.com/llvm/llvm-project/pull/119930.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index ab7d3c70029910..03c9c64e8a1db6 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<Frame> ProfileCallStack,
- ArrayRef<uint64_t> InlinedCallStack,
- unsigned StartIndex = 0) {
- auto StackFrame = ProfileCallStack.begin() + StartIndex;
+ ArrayRef<uint64_t> 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<uint64_t, std::set<const AllocationInfo *>> 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<uint64_t, std::set<std::pair<const std::vector<Frame> *, unsigned>>>
- LocHashToCallSites;
+ // A thin wrapper around ArrayRef<Frame> to facilitate std::set<CallStackRef>.
+ struct CallStackRef : public ArrayRef<Frame> {
+ CallStackRef(ArrayRef<Frame> CS, unsigned Pos)
+ : ArrayRef<Frame>(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<uint64_t, std::set<CallStackRef>> 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<uint64_t, std::set<const AllocationInfo *>>::iterator
AllocInfoIter;
- std::map<uint64_t, std::set<std::pair<const std::vector<Frame> *,
- unsigned>>>::iterator CallSitesIter;
+ std::map<uint64_t, std::set<CallStackRef>>::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
|
snehasish
reviewed
Dec 13, 2024
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
snehasish
reviewed
Dec 14, 2024
snehasish
approved these changes
Dec 14, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/11247 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/10319 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/7665 Here is the relevant piece of the build log for the reference |
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.
This patch essentially replaces:
std::pair<const std::vector *, 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 we don't have an existing hash function for
ArrayRef>, so we provide a custom one, namely
CallStackHash.