This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Load sample profile in LTO link step.
ClosedPublic

Authored by trentxintong on Nov 14 2018, 6:57 PM.

Details

Summary

Load sample profile in LTO link step.
ThinLTO calls populateModulePassManager to load the profile

Diff Detail

Repository
rL LLVM

Event Timeline

trentxintong created this revision.Nov 14 2018, 6:57 PM

Looks like the same issue exists in the new pass manager. The sample loader is added via buildModuleSimplificationPipeline, which is not called either directly or indirectly from buildLTODefaultPipeline. Can you add and test the fix there too? Thanks!

Add the same thing to the new pass manager as suggested by @tejohnson

davidxl added inline comments.Nov 15 2018, 9:28 AM
lib/Passes/PassBuilder.cpp
1006 ↗(On Diff #174221)

I thought sample profile is already loaded twice -- one in FE and one in backend compilation. Is it not the case?

trentxintong added inline comments.Nov 15 2018, 9:31 AM
lib/Passes/PassBuilder.cpp
1006 ↗(On Diff #174221)

I see thats the case for ThinLTO. but not for LTO.

tejohnson added inline comments.Nov 15 2018, 9:34 AM
lib/Passes/PassBuilder.cpp
1008 ↗(On Diff #174221)

Nit: I think the typical format is to put the parameter name comment before the constant.

test/LTO/Resolution/X86/load-sample-prof-lto.ll
19 ↗(On Diff #174221)

No need to use different check prefixes for the two pass managers since they are checking the same thing.

trentxintong added inline comments.Nov 15 2018, 9:46 AM
lib/Passes/PassBuilder.cpp
1008 ↗(On Diff #174221)

A few lines up, line 976 in this file says different -:).

tejohnson added inline comments.Nov 15 2018, 9:53 AM
lib/Passes/PassBuilder.cpp
1008 ↗(On Diff #174221)

Ok nevermind then, better to be consistent within this file.

Address @tejohnson comment. Thanks!

This revision is now accepted and ready to land.Nov 15 2018, 10:07 AM
This revision was automatically updated to reflect the committed changes.