[PATCH] D121179: [memprof] Store callsite metadata with memprof records.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:259
+  // context. We store this as a list of locations, each with its list of
+  // inline locations.
+  llvm::SmallVector<llvm::SmallVector<Frame>> CallSites;
----------------
Specify ordering of inline frames (i.e. leaf to root).


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:294
+Error RawMemProfReader::mapRawProfileToRecords() {
+  // Hold a mapping from function to each callsite location we encounter within
+  // it that is part of some dynamic allocation context. The location is stored
----------------
maybe replace "to each callsite location" with "to the set of callsite locations"


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:328
+      // Record the function this inline frame belongs to.
+      PerFunctionCallSites[Frames.back().Function].insert(&Frames);
+
----------------
I think we decided we need to do the same thing for the callsites that you are doing below for the memprof data on allocations, and put the callsites metadata on the inline functions as well, since the inlining at the point of matching may be less than the inlining in the final instrumented binary. Although in that case in the inline functions we only need the frames in Frames up to and including that function.

e.g.

void qux() { ... }
void foo() { qux(); }
void bar() { foo(); }

If instrumented binary inlined foo -> bar (but not qux->foo), Frames for the address in bar would include:
foo:0:13 (IsInlineFrame = true)
bar:0:13 (IsInlineFrame = false)

and we would want:

foo:
   CallSites:
      [ foo:0:13 ]

bar:
   CallSites:
      [ foo:0:13, bar:0:13 ]


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:340
+
+      // We attach the memprof record to each function bottom-up including the
+      // first non-inline frame.
----------------
It would be clearer I think to have this comment just above the start of this loop, to explain what the loop is doing.


================
Comment at: llvm/unittests/ProfileData/MemProfTest.cpp:172
+  for (const auto &Pair : Reader) {
+    llvm::errs() << "Id: " << Pair.first << "\n";
+    Records.insert({Pair.first, Pair.second});
----------------
Is this printing meant to be here?


================
Comment at: llvm/unittests/ProfileData/MemProfTest.cpp:179
+  //                              AllocSite       CallSite
+  // inline foo() { new(); }         Y               N
+  // bar() { foo(); }                Y               Y
----------------
This is similar to the case I described in my comment in mapRawProfileToRecords(), where I think we should get CallSite metadata in foo() and in xyz() below. Of course, with the "inline" keyword it is likely to be early inlined before we do matching. But in the general case the symbolized stack frame may contain inline frames from a later inlining pass, which would not have been inlined at the point of matching yet. And we can't distinguish these cases when doing the profile symbolization / generation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121179/new/

https://reviews.llvm.org/D121179