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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl 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,
----------------
davidxl wrote:
> 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.
Thanks  for the pointer. Did a little refactor in the new code path (may slightly prefer a separate refactor if the effort is larger or may affect more users)


================
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
----------------
davidxl wrote:
> 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.
> Did you check if test can pass without this patch? 

I see your point. The test would fail without this patch. Now with test edited this is cleaerer.

> Either this or add another small test case on merging of VP profile.

Good point. Edit this test to show profiles with the same line location are merged, and those with different line location are not.


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

https://reviews.llvm.org/D147297