This is an archive of the discontinued LLVM Phabricator instance.

[AutoFDO]Merge called target in body samples
ClosedPublic

Authored by mingmingl on Mar 30 2023, 9:55 PM.

Details

Summary

Body samples could have count and call targets, merge them as well.

Diff Detail

Event Timeline

mingmingl created this revision.Mar 30 2023, 9:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 9:55 PM
Herald added a subscriber: wenlei. · View Herald Transcript
mingmingl requested review of this revision.Mar 30 2023, 9:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 9:55 PM
mingmingl edited the summary of this revision. (Show Details)Mar 30 2023, 9:57 PM
mingmingl added a subscriber: davidxl.
wlei added a comment.Mar 30 2023, 11:09 PM

Good catch, thank you! For the test case, perhaps we can reuse the test https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-profdata/Inputs/sample-flatten-profile.proftext#L6 and add some dummy call target to the inlinees. like

   3: bar:10
    1: 10
+   2: 10 baz:10
mingmingl edited the summary of this revision. (Show Details)
mingmingl added a reviewer: wlei.

add test case (and minor change to use 'try_emplace' that does nothing if key presents)

Good catch, thank you! For the test case, perhaps we can reuse the test https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-profdata/Inputs/sample-flatten-profile.proftext#L6 and add some dummy call target to the inlinees. like

   3: bar:10
    1: 10
+   2: 10 baz:10

thanks for confirming! Done.

wlei added inline comments.Mar 31 2023, 12:14 AM
llvm/include/llvm/ProfileData/SampleProf.h
1357–1360

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:)

llvm/test/tools/llvm-profdata/Inputs/sample-flatten-profile.proftext
4 ↗(On Diff #509916)

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.

davidxl added inline comments.Mar 31 2023, 9:54 AM
llvm/include/llvm/ProfileData/SampleProf.h
1357–1360

Perhaps add a TODO comment.

llvm/test/tools/llvm-profdata/Inputs/sample-flatten-profile.proftext
4 ↗(On Diff #509916)

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

mingmingl marked 4 inline comments as done.

added tests and did a small refactor.

llvm/include/llvm/ProfileData/SampleProf.h
1357–1360

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)

llvm/test/tools/llvm-profdata/Inputs/sample-flatten-profile.proftext
4 ↗(On Diff #509916)

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.

wlei accepted this revision.Mar 31 2023, 12:38 PM

LGTM, thanks for the fix!

llvm/include/llvm/ProfileData/SampleProf.h
1357–1360

Good idea to leverage the addSampleRecord, thanks!

This revision is now accepted and ready to land.Mar 31 2023, 12:38 PM
wlei added inline comments.Mar 31 2023, 12:45 PM
llvm/include/llvm/ProfileData/SampleProf.h
781

Nit: for completeness, add the uint64_t Weight = 1 like other addXXX

mingmingl updated this revision to Diff 510115.Mar 31 2023, 1:16 PM
mingmingl marked an inline comment as done.

Add 'Weight' for default parameter consistency.

This revision was landed with ongoing or failed builds.Mar 31 2023, 1:27 PM
This revision was automatically updated to reflect the committed changes.