This is an archive of the discontinued LLVM Phabricator instance.

Tune isHotFunction/isColdFunction
ClosedPublic

Authored by danielcdh on Sep 28 2016, 2:57 PM.

Details

Summary

This patch sets function as hot if function's entry count is hot/cold.

Diff Detail

Event Timeline

danielcdh updated this revision to Diff 72906.Sep 28 2016, 2:57 PM
danielcdh retitled this revision from to Tune isHotFunction/isColdFunction.
danielcdh updated this object.
danielcdh added reviewers: eraman, davidxl.
danielcdh added a subscriber: llvm-commits.
eraman edited edge metadata.Sep 28 2016, 7:59 PM

Long term we shouldn't be using the current logic (>= 0.3 * MaxFuncCount). Could you just use only isHotCount/isColdCount and run Spec? If the performance doesn't degrade, then we can just kill the 30% or 1% of MaxFunctionCount logic. If there is a regression, we should either live with the current state of things (ie discard this patch), or move the 30%/1% logic into the inliner (and not call isHotFunction/isColdFunction.

davidxl added inline comments.Sep 28 2016, 10:08 PM
lib/Analysis/ProfileSummaryInfo.cpp
80

It is very likely that isHotCount(...) is a superset of the old predicate which means the old predicate is likely to be redundant here.

102

It is likely that isColdCount(..) predicate is a subset of the old predicate -- thus this change likely does not make any behavior difference. To make the new check effective, Either && should be used or remove the old predicate.

danielcdh updated this revision to Diff 73305.Oct 3 2016, 11:01 AM
danielcdh edited edge metadata.

Rename functions.

davidxl added inline comments.Oct 10 2016, 11:39 AM
lib/Analysis/ProfileSummaryInfo.cpp
78–80

Can you remove this change and make the first patch NFC?

164

s/:hot/:hot entry/

166

cold entry

LGTM (after addressing David's comment on separating into two patches)

lib/Analysis/ProfileSummaryInfo.cpp
154

Remove this FIXME since isFunctionEntryHot calls isHotCount.

davidxl accepted this revision.Oct 10 2016, 3:38 PM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.Oct 10 2016, 3:38 PM
danielcdh closed this revision.Oct 10 2016, 10:28 PM