Page MenuHomePhabricator

Separate the logic when handling indirect calls in SamplePGO ThinLTO compile phase and other phases.
ClosedPublic

Authored by danielcdh on Sep 20 2017, 12:26 PM.

Details

Summary

In SamplePGO ThinLTO compile phase, we will not invoke ICP as it may introduce confusion to the 2nd annotation. This patch extracted that logic and makes it clearer before profile annotation. In the mean time, we need to make function importing process both inlined callsites as well as not promoted indirect callsites.

Event Timeline

danielcdh created this revision.Sep 20 2017, 12:26 PM
tejohnson edited edge metadata.Sep 30 2017, 12:05 PM

Is this patch NFC? It looks like there is some new handling for indirect call targets in findImportedFunctions that makes it non-NFC. Could you split this into an NFC refactoring patch and a separate non-NFC patch with the enhancements if so? I think that would be clearer and easier to review.

include/llvm/ProfileData/SampleProf.h
342

Needs comment.

include/llvm/Transforms/SampleProfile.h
29

How about IsThinLTOPreLink for consistency with Phase? (Here and in other classes)

lib/Transforms/IPO/SampleProfile.cpp
89

Similar, let's use PreLink instead of Compile, to be consistent with terminology introduced for pass pipeline. After looking at how this is used, it does something different than what I originally thought. I first read this as whether to invoke this pass during the ThinLTO compile/pre-link, but it is actually forcing simulation of being in that phase. So maybe better to have something like "SampleProfileForceThinLTOPreLink" or similar.

I see this is used when invoked via opt. Can the phase be forced instead by using "-passes='thinlto-pre-link<O2>" (see test/Other/new-pm-thinlto-defaults.ll)?

danielcdh marked 3 inline comments as done.Sep 30 2017, 2:06 PM

Is this patch NFC? It looks like there is some new handling for indirect call targets in findImportedFunctions that makes it non-NFC. Could you split this into an NFC refactoring patch and a separate non-NFC patch with the enhancements if so? I think that would be clearer and easier to review.

Submitted the NFC change in r314601 and rebased the patch

danielcdh updated this revision to Diff 117262.Sep 30 2017, 2:07 PM

rebase and address Teresa's comments

tejohnson added inline comments.Sep 30 2017, 2:36 PM
include/llvm/ProfileData/SampleProf.h
343

What is "FS"? Also, the sentence is unclear, partly because there is an "if" with no corresponding "then". Is this handling indirect or direct calls (CallTargets is not documented - could you add a doxygen description to it)? From the comments it sounds like this is looking for additional hot targets that we didn't import/inline in the profiled binary, but might want to now with additional profile data (?)

lib/Transforms/IPO/SampleProfile.cpp
770–804

Suggest reversing the sense of this condition and moving the shorter pre link case up here.

danielcdh updated this revision to Diff 117265.Sep 30 2017, 2:51 PM
danielcdh marked an inline comment as done.

update

This revision is now accepted and ready to land.Sep 30 2017, 4:03 PM
danielcdh closed this revision.Sep 30 2017, 10:26 PM