Page MenuHomePhabricator

[llvm-profgen] Update callsite body samples by summing up all call target samples.
ClosedPublic

Authored by hoy on Mar 28 2022, 12:05 PM.

Details

Summary

Current profile generation caculcates callsite body samples and call target samples separately. The former is done based on LBR range samples while the latter is done based on branch samples. Note that there's a subtle difference. LBR ranges is formed from two consecutive branch samples. Therefore the last entry in a LBR record will not be counted towards body samples while there's still a chance for it to be counted towards call targets if it is a function call. I'm making sense of the call body samples by updating it to the aggregation of call targets.

Diff Detail

Event Timeline

hoy created this revision.Mar 28 2022, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 12:05 PM
Herald added subscribers: modimo, wenlei. · View Herald Transcript
hoy requested review of this revision.Mar 28 2022, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 12:05 PM
wenlei added inline comments.May 12 2022, 5:16 PM
llvm/include/llvm/ProfileData/SampleProf.h
793

LBR ranges is formed from two consecutive branch samples. Therefore the last entry in a LBR record will not be counted towards body samples while there's still a chance for it to be counted towards call targets if it is a function call

Based on this theory, is it actually possible for I.second.getSamples() to be larger than TargetSamples?

llvm/tools/llvm-profgen/ProfileGenerator.cpp
94

I'm not sure if we need a switch here. It feels to me that we should always try to make the counts consistent. But ofc, that means more testing is needed.

llvm/tools/llvm-profgen/ProfileGenerator.h
104

nit: updateFunctionSamples?

hoy added inline comments.May 12 2022, 11:42 PM
llvm/include/llvm/ProfileData/SampleProf.h
793

Actually it is possible. If some call targets are external targets, they won't be counted, but the body sample count which is from the lbr ranges can include them.

llvm/tools/llvm-profgen/ProfileGenerator.cpp
94

Makes sense to make it unconditional.

Regarding testing, it should have minimal impact on CS probe profile. I'll test for regular non-CS profile.

llvm/tools/llvm-profgen/ProfileGenerator.h
104

done.

hoy updated this revision to Diff 429149.May 12 2022, 11:43 PM

Updating D122609: [llvm-profgen] An option to update callsite body samples by summing up all call target samples.

hoy retitled this revision from [llvm-profgen] An option to update callsite body samples by summing up all call target samples. to [llvm-profgen] Update callsite body samples by summing up all call target samples..May 12 2022, 11:43 PM
hoy edited the summary of this revision. (Show Details)
wenlei added inline comments.May 12 2022, 11:45 PM
llvm/include/llvm/ProfileData/SampleProf.h
28

what triggered this change?

793

Ok, add a comment?

hoy updated this revision to Diff 429150.May 12 2022, 11:46 PM
hoy edited the summary of this revision. (Show Details)

Updating D122609: [llvm-profgen] Update callsite body samples by summing up all call target samples.

hoy updated this revision to Diff 429151.May 12 2022, 11:51 PM

Updating D122609: [llvm-profgen] Update callsite body samples by summing up all call target samples.

llvm/include/llvm/ProfileData/SampleProf.h
793

done.

wenlei accepted this revision.May 12 2022, 11:52 PM

lgtm, thanks.

This revision is now accepted and ready to land.May 12 2022, 11:52 PM
hoy added a comment.May 16 2022, 9:04 AM

Performance testing came back neutral, with a 99.962% profile similarity.

This revision was landed with ongoing or failed builds.May 16 2022, 9:13 AM
This revision was automatically updated to reflect the committed changes.