[memprof] Dump call site matching information #125130

kazutakahirata · 2025-01-30T22:58:49Z

MemProfiler.cpp annotates the IR with the memory profile so that we
can later duplicate context. This patch dumps the entire inline call stack
for each call site match.

MemProfiler.cpp annotates the IR with the memory profile so that we can later duplicate context. Dumping the call site matching information here allows us to analyze how well we manage to annotate the IR. Specifically, this patch dumps: - the full stack ID (to identify the profile call stack) - the index within the profile call stack where we start matching - the size of InlinedCallStack This way, we get to see what part of profile call stack we are matching, not just one frame somewhere in the profile call stack. Now, obtaining the full stack ID requires a little bit of refactoring. This patch modifies the value type of LocHashToCallSites so that it contains the full stack as well as the starting index of a match. Essentially, this patch partially reverts: commit 7c294eb Author: Kazu Hirata <[email protected]> Date: Sat Dec 14 00:03:27 2024 -0800

llvmbot · 2025-01-30T22:59:28Z

@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: Kazu Hirata (kazutakahirata)

Changes

MemProfiler.cpp annotates the IR with the memory profile so that we
can later duplicate context. Dumping the call site matching
information here allows us to analyze how well we manage to annotate
the IR. Specifically, this patch dumps:

the full stack ID (to identify the profile call stack)
the index within the profile call stack where we start matching
the size of InlinedCallStack

This way, we get to see what part of profile call stack we are
matching, not just one frame somewhere in the profile call stack.

Now, obtaining the full stack ID requires a little bit of refactoring.
This patch modifies the value type of LocHashToCallSites so that it
contains the full stack as well as the starting index of a match.
Essentially, this patch partially reverts:

commit 7c294eb
Author: Kazu Hirata <[email protected]>
Date: Sat Dec 14 00:03:27 2024 -0800

Full diff: https://github.com/llvm/llvm-project/pull/125130.diff

1 Files Affected:

(modified) llvm/lib/Transforms/Instrumentation/MemProfiler.cpp (+15-6)

diff --git a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
index 91c48338d032089..66d7466a92c3b39 100644
--- a/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp
@@ -1034,13 +1034,15 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader,
   std::map<uint64_t, std::set<const AllocationInfo *>> LocHashToAllocInfo;
   // A hash function for std::unordered_set<ArrayRef<Frame>> to work.
   struct CallStackHash {
-    size_t operator()(ArrayRef<Frame> CS) const {
-      return computeFullStackId(CS);
+    size_t operator()(const std::pair<ArrayRef<Frame>, unsigned> &CS) const {
+      auto &[CallStack, Idx] = CS;
+      return computeFullStackId(ArrayRef<Frame>(CallStack).drop_front(Idx));
     }
   };
   // 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::unordered_set<ArrayRef<Frame>, CallStackHash>>
+  std::map<uint64_t, std::unordered_set<std::pair<ArrayRef<Frame>, unsigned>,
+                                        CallStackHash>>
       LocHashToCallSites;
   for (auto &AI : MemProfRec->AllocSites) {
     NumOfMemProfAllocContextProfiles++;
@@ -1058,7 +1060,7 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader,
     unsigned Idx = 0;
     for (auto &StackFrame : CS) {
       uint64_t StackId = computeStackId(StackFrame);
-      LocHashToCallSites[StackId].insert(ArrayRef<Frame>(CS).drop_front(Idx++));
+      LocHashToCallSites[StackId].emplace(CS, Idx++);
       ProfileHasColumns |= StackFrame.Column;
       // Once we find this function, we can stop recording.
       if (StackFrame.Function == FuncGUID)
@@ -1201,15 +1203,22 @@ readMemprof(Module &M, Function &F, IndexedInstrProfReader *MemProfReader,
       // instruction's leaf location in the callsites map and not the allocation
       // map.
       assert(CallSitesIter != LocHashToCallSites.end());
-      for (auto CallStackIdx : CallSitesIter->second) {
+      for (auto &[ProfileCallStack, Idx] : CallSitesIter->second) {
         // If we found and thus matched all frames on the call, create and
         // attach call stack metadata.
-        if (stackFrameIncludesInlinedCallStack(CallStackIdx,
+        if (stackFrameIncludesInlinedCallStack(ProfileCallStack.drop_front(Idx),
                                                InlinedCallStack)) {
           NumOfMemProfMatchedCallSites++;
           addCallsiteMetadata(I, InlinedCallStack, Ctx);
           // Only need to find one with a matching call stack and add a single
           // callsite metadata.
+
+          // Dump call site matching information upon request.
+          if (ClPrintMemProfMatchInfo) {
+            uint64_t FullStackId = computeFullStackId(ProfileCallStack);
+            errs() << "MemProf callsite " << FullStackId << " " << Idx << " "
+                   << InlinedCallStack.size() << "\n";
+          }
           break;
         }
       }

teresajohnson

You should probably clarify in the description that this will just be the full profiled context of frames on the interior callsite (i.e. including all possible inlined frames). Unlike in the allocation case where we have the whole allocation context.

Also, can you add a test?

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

kazutakahirata · 2025-02-05T21:09:07Z

You should probably clarify in the description that this will just be the full profiled context of frames on the interior callsite (i.e. including all possible inlined frames). Unlike in the allocation case where we have the whole allocation context.

Thank you for pointing this out! I've switched to printing the entire inline call stack per line.

Also, can you add a test?

Done in the latest revision.

github-actions · 2025-02-05T21:11:03Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ellishg · 2025-02-05T21:37:19Z

llvm/test/Transforms/PGOProfile/memprof-dump-matched-call-sites.ll

@@ -0,0 +1,114 @@
+; Tests that the compiler dumps call site matches upon request.
+;
+; The test case is generated from:


Can we use Elaborated Tests to generate the yaml and the IR?
https://llvm.org/docs/TestingGuide.html#elaborated-tests

Thanks for the pointer! I wasn't aware of the gen feature, but the ability to regenerate the whole thing is very appealing. That said, I like cleaned-up IR in this particular case because I care about debugging information being human readable. I'll definitely keep "Elaborated Tested" in my tool box. Thanks again!

teresajohnson · 2025-02-05T22:18:58Z

llvm/test/Transforms/PGOProfile/memprof-dump-matched-call-sites.ll

+...
+;--- memprof-dump-matched-call-site.ll
+; CHECK: MemProf notcold context with id 3894143216621363392 has total profiled size 4 is matched
+; CHECK: MemProf callsite match for inline call stack 4745611964195289084 10616861955219347331


These will be impossible to compare against the allocation match messages which use the full context hash - should we (optionally?) emit the constituent stack ids of the allocations as well?

Right, we cannot compare these stack IDs against anything else dumped by the compiler. I am thinking about an external tool to dump the full call stack along its constituent stack IDs.

If we are interested in assessing how well we are matching call sites, we need an external tool to know the universe (i.e. the list of all full call stacks) so that we can compute the set complement -- "universe" - "all call site matches" (roughly speaking). We could ask one invocation of the compiler to dump the entire profile contents, including constituent stack IDs, but picking one compiler invocation feels a bit weird. It's cleaner to have a separate tool to dump the profile IMHO.

The other alternative is to have the compiler optionally dump out this correspondence for each matched allocation, however, there is already a need to deduplicate these across the compilations and that would increase the volume quite a bit. The upside is that we wouldn't need another tool and tool invocation. The other upside is that the compiler trims unneeded parts of the context during matching, which would reduce the volume and the need for redoing this analysis externally. But that can be considered for a follow on change if needed.

@teresajohnson can you elaborate on this -- compiler trims unneeded parts of the context during matching? Are you referring to this FIXME?

No I meant the trimming done in CallStackTrie::buildAndAttachMIBMetadata called here:

llvm-project/llvm/lib/Transforms/Instrumentation/MemProfiler.cpp

Line 1184 in 11c3f52

bool MemprofMDAttached = AllocTrie.buildAndAttachMIBMetadata(CI);

Chatted with @teresajohnson about this offline -- the fact that it trims unneeded contexts is orthogonal to how we want to evaluate the matching quality so we can continue the current approach. We can evaluate whether more precision is required if simple heuristics for prioritization (e.g. look at N frames from the leaf) don't yield good results.

teresajohnson

lgtm

teresajohnson · 2025-02-06T15:17:29Z

llvm/test/Transforms/PGOProfile/memprof-dump-matched-call-sites.ll

+...
+;--- memprof-dump-matched-call-site.ll
+; CHECK: MemProf notcold context with id 3894143216621363392 has total profiled size 4 is matched
+; CHECK: MemProf callsite match for inline call stack 4745611964195289084 10616861955219347331


The other alternative is to have the compiler optionally dump out this correspondence for each matched allocation, however, there is already a need to deduplicate these across the compilations and that would increase the volume quite a bit. The upside is that we wouldn't need another tool and tool invocation. The other upside is that the compiler trims unneeded parts of the context during matching, which would reduce the volume and the need for redoing this analysis externally. But that can be considered for a follow on change if needed.

MemProfiler.cpp annotates the IR with the memory profile so that we can later duplicate context. This patch dumps the entire inline call stack for each call site match.

kazutakahirata requested review from snehasish and teresajohnson January 30, 2025 22:58

llvmbot added the llvm:transforms label Jan 30, 2025

teresajohnson reviewed Jan 31, 2025

View reviewed changes

llvm/lib/Transforms/Instrumentation/MemProfiler.cpp Outdated Show resolved Hide resolved

Address comments.

ec5c64f

llvmbot added the PGO Profile Guided Optimizations label Feb 5, 2025

clang-format MemProfiler.cpp.

05b2f60

ellishg reviewed Feb 5, 2025

View reviewed changes

teresajohnson reviewed Feb 5, 2025

View reviewed changes

teresajohnson approved these changes Feb 6, 2025

View reviewed changes

snehasish approved these changes Feb 6, 2025

View reviewed changes

kazutakahirata merged commit b7feccb into llvm:main Feb 7, 2025
8 checks passed

kazutakahirata deleted the memprof_dump_callsite branch February 7, 2025 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[memprof] Dump call site matching information #125130

[memprof] Dump call site matching information #125130

Uh oh!

kazutakahirata commented Jan 30, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jan 30, 2025 •

edited

Loading

Uh oh!

teresajohnson left a comment

Uh oh!

Uh oh!

kazutakahirata commented Feb 5, 2025

Uh oh!

github-actions bot commented Feb 5, 2025 •

edited

Loading

Uh oh!

ellishg Feb 5, 2025

Uh oh!

kazutakahirata Feb 5, 2025

Uh oh!

teresajohnson Feb 5, 2025

Uh oh!

kazutakahirata Feb 5, 2025 •

edited

Loading

Uh oh!

teresajohnson Feb 6, 2025

Uh oh!

snehasish Feb 6, 2025

Uh oh!

teresajohnson Feb 6, 2025

Uh oh!

snehasish Feb 6, 2025

Uh oh!

teresajohnson left a comment

Uh oh!

teresajohnson Feb 6, 2025

Uh oh!

Uh oh!

Uh oh!

[memprof] Dump call site matching information #125130

[memprof] Dump call site matching information #125130

Uh oh!

Conversation

kazutakahirata commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kazutakahirata commented Feb 5, 2025

Uh oh!

github-actions bot commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazutakahirata Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kazutakahirata commented Jan 30, 2025 •

edited

Loading

llvmbot commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025 •

edited

Loading

kazutakahirata Feb 5, 2025 •

edited

Loading