This is an archive of the discontinued LLVM Phabricator instance.

[SampleFDO] Create a separate flag ProfileAccurateForProfSymList for handling profile symbol list
ClosedPublic

Authored by wmi on Sep 25 2019, 1:26 PM.

Details

Summary

Currently many existing users using profile-sample-accurate want to reduce code size as much as possible. Their use cases are different from the scenario profile symbol list tries to handle -- the major motivation of adding profile symbol list is to get the major memory/code size saving without introduce performance regression. So to keep the behavior of profile-sample-accurate unchanged, we think decoupling these two things and use a new flag to control the handling of profile symbol list is better.

Besides the reason above, another benefit of adding a separate flag is we can flip on ProfileAccurateForProfSymList by default, but that flip will only affect targets using profile with symbol list. Targets won't see any impact as long as their profiles don't contain symbol list. It is possible to get the memory saving by just adding profile symbol list in the profile so it is easier for deployment.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Sep 25 2019, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 1:26 PM
davidxl added inline comments.Sep 25 2019, 3:54 PM
lib/Transforms/IPO/SampleProfile.cpp
134

profile-accurate-for-symbols-in-list ?

davidxl added inline comments.Sep 25 2019, 3:56 PM
lib/Transforms/IPO/SampleProfile.cpp
134

It should be true by default.

136

are --> to be.

wmi updated this revision to Diff 221856.Sep 25 2019, 4:22 PM

Address David's comment.

davidxl added inline comments.Sep 27 2019, 9:47 AM
lib/Transforms/IPO/SampleProfile.cpp
1681–1683

check PSL too?

1791

If symbol list is available, it will override the decision of -fprofile-sample-accurate. Is this the intended behavior?

wmi marked 2 inline comments as done.Sep 27 2019, 10:03 AM
wmi added inline comments.
lib/Transforms/IPO/SampleProfile.cpp
1681–1683

Fixed.

1791

Good point. I feel it is safer to keep the profile-sample-accurate behavior the same especially if large scale rollout of PSL is needed, so I will change it.

wmi updated this revision to Diff 222201.Sep 27 2019, 10:45 AM

Address David's comment: profile-accurate-for-symsinlist will be overriden by profile-sample-accurate if both flags present. Add test for it.

wmi updated this revision to Diff 222202.Sep 27 2019, 10:46 AM

Update the flag description of profile-accurate-for-symsinlist.

davidxl added inline comments.Sep 27 2019, 11:31 AM
lib/Transforms/IPO/SampleProfile.cpp
502

Add an assert here that profile-sample-accurate is not on?

1682

Add a comment that while profile-sample-accurate is on, ignore symbol list.

1786

Improve the comment to be: " profile-sample-accurate is a user assertion which has a higher precedence than symbol list. When profile-sample-accurate is on, ignore symbol list.

wmi marked 3 inline comments as done.Sep 27 2019, 1:45 PM
wmi added inline comments.
lib/Transforms/IPO/SampleProfile.cpp
502

I add an assertion in the caller of callsiteIsHot, so we can check the case that profile-sample-accurate is set in function attribute.

wmi updated this revision to Diff 222244.Sep 27 2019, 1:49 PM

Address David's comment.

davidxl accepted this revision.Sep 27 2019, 1:57 PM

lgtm

This revision is now accepted and ready to land.Sep 27 2019, 1:57 PM
This revision was automatically updated to reflect the committed changes.