This is an archive of the discontinued LLVM Phabricator instance.

[SampleFDO] Skip profile reading when flatten profile is used in ThinLTO postlink phase
ClosedPublic

Authored by wmi on Nov 21 2018, 4:14 PM.

Details

Summary

If the sample profile has no inlining hierachy information included, we call the sample profile is flatten. For flatten profile, in ThinLTO postlink phase, SampleProfileLoader's hot function inlining and profile annotation will do nothing, so it is better to save the effort to read in the profile (pass running will be skipped too because no profile is available). It is helpful for reducing compile time when the flatten profile is huge.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi created this revision.Nov 21 2018, 4:14 PM

I just stumbled on this patch - sorry for the late review! I thought this was already in. A few comments and a question below. Also, presumably this should be done for the old PM as well?

lib/Passes/PassBuilder.cpp
612 ↗(On Diff #174994)

Rather than passing this down and skipping the initialization in the pass, can we just skip adding the pass completely?

lib/Transforms/IPO/SampleProfile.cpp
126 ↗(On Diff #174994)

s/flatten/flattened/ here and in name/description below (and in rest of patch)

310 ↗(On Diff #174994)

Nit, add empty line above

wmi marked 3 inline comments as done.Jan 11 2019, 6:43 PM

Yes, the support is also helpful for old pass manager. Add some code to make it easier to enable sampleprofileloader pass in the pipeline of old pass manager through opt, mainly for testing purpose.

lib/Passes/PassBuilder.cpp
612 ↗(On Diff #174994)

Done.

lib/Transforms/IPO/SampleProfile.cpp
126 ↗(On Diff #174994)

Fixed.

wmi updated this revision to Diff 181414.Jan 11 2019, 6:46 PM

Address Teresa's comments.

tejohnson added inline comments.Jan 15 2019, 11:08 AM
lib/Passes/PassBuilder.cpp
619 ↗(On Diff #181414)

Minor nit: to me this would be more understandable if written as:

if (!(FlattenedProfileUsed && Phase == ThinLTOPhase::PostLink))

(ditto for the old PM check, although I have another note down there that is more fundamental).

lib/Transforms/IPO/PassManagerBuilder.cpp
438 ↗(On Diff #181414)

By guarding this with PrepareForThinLTO, I think that means that with a flattened profile and a non-thinlto build we would not get the sample PGO loaded. Should PrepareForThinLTO instead be !PerformThinLTO (which is equivalent to your new PM check)?

tools/opt/opt.cpp
391 ↗(On Diff #181414)

How have we been testing the PGO pipelines in the old PM without this change?

wmi marked 3 inline comments as done.Jan 15 2019, 2:51 PM
wmi added inline comments.
lib/Transforms/IPO/PassManagerBuilder.cpp
438 ↗(On Diff #181414)

Good catch. Thanks. A new option to control PerformThinLTO is added for testing purpose.

tools/opt/opt.cpp
391 ↗(On Diff #181414)

There are some opt options defined in PassManagerBuilder.cpp for PGO pipeline testing. I feel like it is better to use the mechanism in opt driver similar as what new pass manager does. I will send out a clean up patch first.

wmi updated this revision to Diff 182319.Jan 17 2019, 9:31 AM

Address Teresa's comments.

This revision is now accepted and ready to land.Jan 17 2019, 11:13 AM
This revision was automatically updated to reflect the committed changes.