This is an archive of the discontinued LLVM Phabricator instance.

ProfileSummaryInfo improvements
ClosedPublic

Authored by eraman on Jan 11 2017, 3:24 PM.

Details

Summary
  • Add is{Hot|Cold}CallSite methods
  • Fix a bug in isHotBB where it was looking for MD_prof on a return instruction
  • Use MD_prof data only if sample profiling was used to collect profiles.
  • Add an unit test to ProfileSummaryInfo

Diff Detail

Event Timeline

eraman updated this revision to Diff 84040.Jan 11 2017, 3:24 PM
eraman retitled this revision from to Add is{Hot|Cold}CallSite and an unit test to ProfileSummaryInfo.
eraman updated this object.
eraman added reviewers: danielcdh, davidxl.
eraman added a subscriber: llvm-commits.
davidxl added inline comments.Jan 11 2017, 3:38 PM
lib/Analysis/ProfileSummaryInfo.cpp
139

is this a separate fix?

203

should the order of check swapped? BFI check first and then meta data ?

eraman added inline comments.Jan 11 2017, 3:49 PM
lib/Analysis/ProfileSummaryInfo.cpp
139

yes, that part was never exercised before.

203

I discussed this with Dehao and he said it is possible that block's hotness got messed up in the upward direction (higher than what it should be) in sample profiling. If there is annotation, it is better to treat it as the ground truth.

davidxl added inline comments.Jan 11 2017, 3:54 PM
lib/Analysis/ProfileSummaryInfo.cpp
203

This is not good for instrumentation based PGO.

  1. meta data is not updated during cloning so it can get wrong for instrumentation based pgo
  2. it is slightly slower for instrumentation based PGO.

In fact, we should probably never need to check weight data for instPGO.

eraman added inline comments.Jan 11 2017, 3:57 PM
lib/Analysis/ProfileSummaryInfo.cpp
203

Agreed that we should check which mode we're in, but it is not a problem now because the annotations happen only in Sample PGO.

davidxl added inline comments.Jan 11 2017, 4:01 PM
lib/Analysis/ProfileSummaryInfo.cpp
203

ok. A comment there is good for clarification.

eraman updated this revision to Diff 84149.Jan 12 2017, 11:19 AM

Check MD_prof metadata on calls and branches to determine hotness only when it is sample profile and fix the unittest.

eraman added inline comments.Jan 12 2017, 11:20 AM
lib/Analysis/ProfileSummaryInfo.cpp
203

I have added the check for profile type instead.

davidxl edited edge metadata.Jan 12 2017, 11:43 AM

looks fine to me. Dehao, do you have any comments?

danielcdh edited edge metadata.Jan 12 2017, 2:35 PM

looks fine to me. Dehao, do you have any comments?

The current changes LGTM.

Thanks,
Dehao

eraman retitled this revision from Add is{Hot|Cold}CallSite and an unit test to ProfileSummaryInfo to ProfileSummaryInfo improvements.Jan 12 2017, 3:01 PM
eraman updated this object.
eraman edited edge metadata.
davidxl accepted this revision.Jan 12 2017, 5:06 PM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jan 12 2017, 5:06 PM
This revision was automatically updated to reflect the committed changes.