[PATCH] D147297: [AutoFDO]Merge called target in body samples

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1354
     } else {
-      for (const auto &Line : FS.getBodySamples()) {
-        Profile.addBodySamples(Line.first.LineOffset, Line.first.Discriminator,
-                               Line.second.getSamples());
+      for (const auto &[DI, SampleRecord] : FS.getBodySamples()) {
+        Profile.addBodySamples(DI.LineOffset, DI.Discriminator,
----------------
wlei wrote:
> Nit: Looks like there is an existing function https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/SampleProf.cpp#L119 doing the similar thing, but I guess that requires some changes of it, I'm good for this version, note here just in case people like to refactor it in the future:) 
Perhaps add a TODO comment.


================
Comment at: llvm/test/tools/llvm-profdata/Inputs/sample-flatten-profile.proftext:4
  3: 20
+ 4: 21 qux:5 quux:6 corge:10
  5: foo:30
----------------
wlei wrote:
> Thanks for adding the test.
> Did you check if test can pass without this patch? Here the `baz` is the top-level profile, it's possible that it's just copied from the input profile ( see the `if (Ret.second)` true branch). Or I'm thinking if we can add another line to the `baz` in line 28 so that it works for the either merging order.
Either this or add another small test case on merging of VP profile.


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

https://reviews.llvm.org/D147297