This is an archive of the discontinued LLVM Phabricator instance.

Use the first instruction's count to estimate the funciton's entry frequency.
ClosedPublic

Authored by danielcdh on Oct 2 2017, 1:52 PM.

Details

Summary

In the current implementation, we only have accurate profile count for standalone symbols. For inlined functions, we do not have entry count data because it's not available in LBR. In this patch, we use the first instruction's frequency to estimiate the function's entry count, especially for inlined functions. This may be inaccurate due to debug info in optimized code. However, this is a better estimate than the static 80/20 estimation we have in the current implementation.

Event Timeline

danielcdh created this revision.Oct 2 2017, 1:52 PM
tejohnson edited edge metadata.Oct 3 2017, 11:32 AM

Started looking at, but have a few initial questions.

include/llvm/ProfileData/SampleProf.h
303

s/standalong/standalone/

304

Should this be used somewhere in this change since it is preferred?

311

What is this line checking?

316

What do these samples correspond to?

danielcdh marked an inline comment as done.Oct 3 2017, 12:48 PM
danielcdh added inline comments.
include/llvm/ProfileData/SampleProf.h
304

This was used by standalone symbols before this change. The 2 newly added use of getEntryCount was used in this patch for inlined functions.

311

Added comment to make it clear.

316

Updated comment to make it clear.

danielcdh updated this revision to Diff 117565.Oct 3 2017, 12:48 PM

update comment

tejohnson added inline comments.Oct 9 2017, 8:05 AM
include/llvm/ProfileData/SampleProf.h
317

"An indirect callsite may be..."?

lib/Transforms/IPO/SampleProfile.cpp
1268

Is this annotated the value profile before or after promotion/inlining of the indirect calls recorded as inlined in the profile? If after, not sure why we want to include the counts of the promoted/inlined targets in Sum (which AFAICT is what will happen by computing Sum via findIndirectCallFunctionSamples).

danielcdh updated this revision to Diff 118219.Oct 9 2017, 9:26 AM

update comment

lib/Transforms/IPO/SampleProfile.cpp
1268

It's after. The reason we need to have all promoted counts included in SUM is because we do not want to promote too many targets. E.g. if an indirect call site has already been promoted to 3 targets that covers 99% of the case. The rest 1%, even if it's 100% to a 4th target. we do not want to promote it.

tejohnson added inline comments.Oct 10 2017, 9:41 AM
lib/Transforms/IPO/SampleProfile.cpp
1268

The reason I ask is that this is different than what is done by the main ICP pass. E.g. see ICallPromotionFunc::processFunction and ICallPromotionFunc::tryToPromote, which update the total count before re-generating a new VP annotation. It also makes this patch have a larger effect than what is described in the summary. Suggest splitting this change out and evaluating separately.

danielcdh marked an inline comment as done.

extract the change that update the SUM of VP.

This revision is now accepted and ready to land.Oct 10 2017, 1:19 PM
danielcdh closed this revision.Oct 10 2017, 2:13 PM