[MemProf] Fix the propagation of context/size info after inlining #164872
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
In certain cases the context/size info we use for reporting of hinted bytes in the LTO link was being dropped when we re-constructed context tries and memprof metadata after inlining. This only affected cases where we were using the -memprof-min-percent-max-cold-size option to only keep that information for the largest cold contexts, and where the pre-LTO compile did *not* specify -memprof-report-hinted-sizes. The issue is that we don't have a MaxSize, which is only available during the profile matching step. Use an existing bool indicating that we are redoing this from existing metadata to always propagate any context size metadata in that case.
llvmbot
added
the
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Teresa Johnson (teresajohnson) ChangesIn certain cases the context/size info we use for reporting of hinted The issue is that we don't have a MaxSize, which is only available Full diff: https://github.com/llvm/llvm-project/pull/164872.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/MemoryProfileInfo.cpp b/llvm/lib/Analysis/MemoryProfileInfo.cpp
index 92a5b6f378aab..b09f4ed78ca7e 100644
--- a/llvm/lib/Analysis/MemoryProfileInfo.cpp
+++ b/llvm/lib/Analysis/MemoryProfileInfo.cpp
@@ -241,9 +241,13 @@ static MDNode *createMIBNode(LLVMContext &Ctx, ArrayRef<uint64_t> MIBCallStack,
ColdBytes += TotalSize;
// If we have the max cold context size from summary information and have
// requested identification of contexts above a percentage of the max, see
- // if this context qualifies.
- if (MaxColdSize > 0 && MinPercentMaxColdSize < 100 &&
- TotalSize * 100 >= MaxColdSize * MinPercentMaxColdSize)
+ // if this context qualifies. We should assume this is large if we rebuilt
+ // the trie from existing metadata (i.e. to update after inlining), in
+ // which case we don't have a MaxSize from the profile - we assume any
+ // context size info in existence on the metadata should be propagated.
+ if (BuiltFromExistingMetadata ||
+ (MaxColdSize > 0 && MinPercentMaxColdSize < 100 &&
+ TotalSize * 100 >= MaxColdSize * MinPercentMaxColdSize))
LargeColdContext = true;
}
// Only add the context size info as metadata if we need it in the thin
diff --git a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
index d1c0f643f5cd7..113b05299252b 100644
--- a/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
+++ b/llvm/unittests/Analysis/MemoryProfileInfoTest.cpp
@@ -638,7 +638,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
!0 = !{!1, !3, !5, !7, !9, !11}
!1 = !{!2, !"cold"}
!2 = !{i64 1, i64 2, i64 3}
-!3 = !{!4, !"cold"}
+!3 = !{!4, !"cold", !13}
!4 = !{i64 1, i64 2, i64 4}
!5 = !{!6, !"notcold"}
!6 = !{i64 1, i64 5, i64 6}
@@ -648,6 +648,7 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
!10 = !{i64 1, i64 8, i64 9}
!11 = !{!12, !"hot"}
!12 = !{i64 1, i64 8, i64 10}
+!13 = !{i64 123, i64 456}
)IR");
Function *Func = M->getFunction("test");
@@ -683,10 +684,25 @@ declare dso_local noalias noundef i8* @malloc(i64 noundef)
auto *StackId = mdconst::dyn_extract<ConstantInt>(StackMD->getOperand(0));
EXPECT_EQ(StackId->getZExtValue(), 1u);
StackId = mdconst::dyn_extract<ConstantInt>(StackMD->getOperand(1));
- if (StackId->getZExtValue() == 2u)
+ if (StackId->getZExtValue() == 2u) {
EXPECT_EQ(getMIBAllocType(MIB), AllocationType::Cold);
- else if (StackId->getZExtValue() == 5u)
+ // We should propagate the single context size info from the second cold
+ // context above onto the new merged/trimmed context.
+ ASSERT_EQ(MIB->getNumOperands(), 3u);
+ MDNode *ContextSizePair = dyn_cast<MDNode>(MIB->getOperand(2));
+ assert(ContextSizePair->getNumOperands() == 2);
+ EXPECT_EQ(
+ mdconst::dyn_extract<ConstantInt>(ContextSizePair->getOperand(0))
+ ->getZExtValue(),
+ 123u);
+ EXPECT_EQ(
+ mdconst::dyn_extract<ConstantInt>(ContextSizePair->getOperand(1))
+ ->getZExtValue(),
+ 456u);
+ } else if (StackId->getZExtValue() == 5u) {
EXPECT_EQ(getMIBAllocType(MIB), AllocationType::NotCold);
+ ASSERT_EQ(MIB->getNumOperands(), 2u);
+ }
}
}
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/20109 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/52/builds/12218 Here is the relevant piece of the build log for the reference |
dvbuka
pushed a commit
to dvbuka/llvm-project
that referenced
this pull request
Oct 27, 2025
…vm#164872) In certain cases the context/size info we use for reporting of hinted bytes in the LTO link was being dropped when we re-constructed context tries and memprof metadata after inlining. This only affected cases where we were using the -memprof-min-percent-max-cold-size option to only keep that information for the largest cold contexts, and where the pre-LTO compile did *not* specify -memprof-report-hinted-sizes. The issue is that we don't have a MaxSize, which is only available during the profile matching step. Use an existing bool indicating that we are redoing this from existing metadata to always propagate any context size metadata in that case.
Lukacma
pushed a commit
to Lukacma/llvm-project
that referenced
this pull request
Oct 29, 2025
…vm#164872) In certain cases the context/size info we use for reporting of hinted bytes in the LTO link was being dropped when we re-constructed context tries and memprof metadata after inlining. This only affected cases where we were using the -memprof-min-percent-max-cold-size option to only keep that information for the largest cold contexts, and where the pre-LTO compile did *not* specify -memprof-report-hinted-sizes. The issue is that we don't have a MaxSize, which is only available during the profile matching step. Use an existing bool indicating that we are redoing this from existing metadata to always propagate any context size metadata in that case.
aokblast
pushed a commit
to aokblast/llvm-project
that referenced
this pull request
Oct 30, 2025
…vm#164872) In certain cases the context/size info we use for reporting of hinted bytes in the LTO link was being dropped when we re-constructed context tries and memprof metadata after inlining. This only affected cases where we were using the -memprof-min-percent-max-cold-size option to only keep that information for the largest cold contexts, and where the pre-LTO compile did *not* specify -memprof-report-hinted-sizes. The issue is that we don't have a MaxSize, which is only available during the profile matching step. Use an existing bool indicating that we are redoing this from existing metadata to always propagate any context size metadata in that case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
In certain cases the context/size info we use for reporting of hinted
bytes in the LTO link was being dropped when we re-constructed context
tries and memprof metadata after inlining. This only affected cases
where we were using the -memprof-min-percent-max-cold-size option to
only keep that information for the largest cold contexts, and where the
pre-LTO compile did not specify -memprof-report-hinted-sizes.
The issue is that we don't have a MaxSize, which is only available
during the profile matching step. Use an existing bool indicating that
we are redoing this from existing metadata to always propagate any
context size metadata in that case.