[memprof] Refactor readMemProf (NFC) #149663
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 creates a helper function named handleAllocSite to handle the allocation site. It makes readMemProf a little bit shorter. I'm planning to move the code to handle call sites in a subsequent patch. Doing so in this patch would make this patch a lot longer because we need to move other things like CallSiteEntry and CallSiteEntryHash.
|
@llvm/pr-subscribers-llvm-transforms Author: Kazu Hirata (kazutakahirata) ChangesThis patch creates a helper function named handleAllocSite to handle I'm planning to move the code to handle call sites in a subsequent Full diff: https://github.com/llvm/llvm-project/pull/149663.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
index e5b357fc1bfbf..2a8416d02ffba 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfUse.cpp
@@ -361,6 +361,74 @@ static void addVPMetadata(Module &M, Instruction &I,
}
}
+static void
+handleAllocSite(Instruction &I, CallBase *CI,
+ ArrayRef<uint64_t> InlinedCallStack, LLVMContext &Ctx,
+ OptimizationRemarkEmitter &ORE, uint64_t MaxColdSize,
+ const std::set<const AllocationInfo *> &AllocInfoSet,
+ std::map<std::pair<uint64_t, unsigned>, AllocMatchInfo>
+ &FullStackIdToAllocMatchInfo) {
+ // We may match this instruction's location list to multiple MIB
+ // contexts. Add them to a Trie specialized for trimming the contexts to
+ // the minimal needed to disambiguate contexts with unique behavior.
+ CallStackTrie AllocTrie(&ORE, MaxColdSize);
+ uint64_t TotalSize = 0;
+ uint64_t TotalColdSize = 0;
+ for (auto *AllocInfo : AllocInfoSet) {
+ // Check the full inlined call stack against this one.
+ // If we found and thus matched all frames on the call, include
+ // this MIB.
+ if (stackFrameIncludesInlinedCallStack(AllocInfo->CallStack,
+ InlinedCallStack)) {
+ NumOfMemProfMatchedAllocContexts++;
+ uint64_t FullStackId = 0;
+ if (ClPrintMemProfMatchInfo || recordContextSizeInfoForAnalysis())
+ FullStackId = computeFullStackId(AllocInfo->CallStack);
+ auto AllocType = addCallStack(AllocTrie, AllocInfo, FullStackId);
+ TotalSize += AllocInfo->Info.getTotalSize();
+ if (AllocType == AllocationType::Cold)
+ TotalColdSize += AllocInfo->Info.getTotalSize();
+ // Record information about the allocation if match info printing
+ // was requested.
+ if (ClPrintMemProfMatchInfo) {
+ assert(FullStackId != 0);
+ FullStackIdToAllocMatchInfo[std::make_pair(FullStackId,
+ InlinedCallStack.size())] = {
+ AllocInfo->Info.getTotalSize(), AllocType};
+ }
+ }
+ }
+ // If the threshold for the percent of cold bytes is less than 100%,
+ // and not all bytes are cold, see if we should still hint this
+ // allocation as cold without context sensitivity.
+ if (TotalColdSize < TotalSize && MinMatchedColdBytePercent < 100 &&
+ TotalColdSize * 100 >= MinMatchedColdBytePercent * TotalSize) {
+ AllocTrie.addSingleAllocTypeAttribute(CI, AllocationType::Cold, "dominant");
+ return;
+ }
+
+ // We might not have matched any to the full inlined call stack.
+ // But if we did, create and attach metadata, or a function attribute if
+ // all contexts have identical profiled behavior.
+ if (!AllocTrie.empty()) {
+ NumOfMemProfMatchedAllocs++;
+ // MemprofMDAttached will be false if a function attribute was
+ // attached.
+ bool MemprofMDAttached = AllocTrie.buildAndAttachMIBMetadata(CI);
+ assert(MemprofMDAttached == I.hasMetadata(LLVMContext::MD_memprof));
+ if (MemprofMDAttached) {
+ // Add callsite metadata for the instruction's location list so that
+ // it simpler later on to identify which part of the MIB contexts
+ // are from this particular instruction (including during inlining,
+ // when the callsite metadata will be updated appropriately).
+ // FIXME: can this be changed to strip out the matching stack
+ // context ids from the MIB contexts and not add any callsite
+ // metadata here to save space?
+ addCallsiteMetadata(I, InlinedCallStack, Ctx);
+ }
+ }
+}
+
static void readMemprof(Module &M, Function &F,
IndexedInstrProfReader *MemProfReader,
const TargetLibraryInfo &TLI,
@@ -554,66 +622,8 @@ static void readMemprof(Module &M, Function &F,
if (AllocInfoIter != LocHashToAllocInfo.end() &&
// Only consider allocations which support hinting.
isAllocationWithHotColdVariant(CI->getCalledFunction(), TLI)) {
- // We may match this instruction's location list to multiple MIB
- // contexts. Add them to a Trie specialized for trimming the contexts to
- // the minimal needed to disambiguate contexts with unique behavior.
- CallStackTrie AllocTrie(&ORE, MaxColdSize);
- uint64_t TotalSize = 0;
- uint64_t TotalColdSize = 0;
- for (auto *AllocInfo : AllocInfoIter->second) {
- // Check the full inlined call stack against this one.
- // If we found and thus matched all frames on the call, include
- // this MIB.
- if (stackFrameIncludesInlinedCallStack(AllocInfo->CallStack,
- InlinedCallStack)) {
- NumOfMemProfMatchedAllocContexts++;
- uint64_t FullStackId = 0;
- if (ClPrintMemProfMatchInfo || recordContextSizeInfoForAnalysis())
- FullStackId = computeFullStackId(AllocInfo->CallStack);
- auto AllocType = addCallStack(AllocTrie, AllocInfo, FullStackId);
- TotalSize += AllocInfo->Info.getTotalSize();
- if (AllocType == AllocationType::Cold)
- TotalColdSize += AllocInfo->Info.getTotalSize();
- // Record information about the allocation if match info printing
- // was requested.
- if (ClPrintMemProfMatchInfo) {
- assert(FullStackId != 0);
- FullStackIdToAllocMatchInfo[std::make_pair(
- FullStackId, InlinedCallStack.size())] = {
- AllocInfo->Info.getTotalSize(), AllocType};
- }
- }
- }
- // If the threshold for the percent of cold bytes is less than 100%,
- // and not all bytes are cold, see if we should still hint this
- // allocation as cold without context sensitivity.
- if (TotalColdSize < TotalSize && MinMatchedColdBytePercent < 100 &&
- TotalColdSize * 100 >= MinMatchedColdBytePercent * TotalSize) {
- AllocTrie.addSingleAllocTypeAttribute(CI, AllocationType::Cold,
- "dominant");
- continue;
- }
-
- // We might not have matched any to the full inlined call stack.
- // But if we did, create and attach metadata, or a function attribute if
- // all contexts have identical profiled behavior.
- if (!AllocTrie.empty()) {
- NumOfMemProfMatchedAllocs++;
- // MemprofMDAttached will be false if a function attribute was
- // attached.
- bool MemprofMDAttached = AllocTrie.buildAndAttachMIBMetadata(CI);
- assert(MemprofMDAttached == I.hasMetadata(LLVMContext::MD_memprof));
- if (MemprofMDAttached) {
- // Add callsite metadata for the instruction's location list so that
- // it simpler later on to identify which part of the MIB contexts
- // are from this particular instruction (including during inlining,
- // when the callsite metadata will be updated appropriately).
- // FIXME: can this be changed to strip out the matching stack
- // context ids from the MIB contexts and not add any callsite
- // metadata here to save space?
- addCallsiteMetadata(I, InlinedCallStack, Ctx);
- }
- }
+ handleAllocSite(I, CI, InlinedCallStack, Ctx, ORE, MaxColdSize,
+ AllocInfoIter->second, FullStackIdToAllocMatchInfo);
continue;
}
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/136/builds/4654 Here is the relevant piece of the build log for the reference |
This was referenced Jul 23, 2025
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jul 28, 2025
This patch creates a helper function named handleAllocSite to handle the allocation site. It makes readMemProf a little bit shorter. I'm planning to move the code to handle call sites in a subsequent patch. Doing so in this patch would make this patch a lot longer because we need to move other things like CallSiteEntry and CallSiteEntryHash.
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 creates a helper function named handleAllocSite to handle
the allocation site. It makes readMemProf a little bit shorter.
I'm planning to move the code to handle call sites in a subsequent
patch. Doing so in this patch would make this patch a lot longer
because we need to move other things like CallSiteEntry and
CallSiteEntryHash.