Page MenuHomePhabricator

[PGO] clang part of change for context-sensitive PGO.
ClosedPublic

Authored by xur on Nov 6 2018, 12:20 PM.

Diff Detail

Event Timeline

xur created this revision.Nov 6 2018, 12:20 PM
xur updated this revision to Diff 175926.Nov 29 2018, 12:09 PM

update the patch to work with new version in
https://reviews.llvm.org/D54175

Can you add a summary of this change, i.e., the driver option processing flow for csPGO.

Test cases are also missing.

xur added a comment.Nov 30 2018, 10:51 AM

Here is the summary of CSPGO options workflow:

(1) CSPGO instrumentation.
We add new driver options of fcs-profile-generate fcs-profile-generate=. They are typically used with -fprofile-use (but not required).
They will not work together with -fprofile-generate (They could, but not do not make sense. There is a hard error for this).

We also add a new instrument kind (csllvm) for cc1 option fprofile-instrument. This is corresponding to a new CodeGen option: CodeGenOptions::ProfileCSIRInstr.

The driver option is parsed and translated to cc1 option in addPGOAndCoverageFlags().

cc1 option is converted to CodeGenOpts in setPGOInstrumentor() (CompilerInvocation.cpp).

CodeGenOpts is passed into llvm pass manager in BackendUtil.cpp:
(1) PMBuilder options for legacy passmanager in EmitAssemblyHelper::CreatePasses().
(2) PGOOptions struct for new passmanager in EmitAssemblyHelper::EmitAssemblyWithNewPassManager().

(2) PGO use
We will set CodeGenOptions::ProfileCSIRInstr in setPGOUseInstrumentor() (CompilerInvocation.cpp) when processing -fprofile-use option.

CodeGenOpts is handled similarly as in CSPGO instrumentation.

(3) lto/thinlto
We need to pass CSPGO instrumentation and PGO profile to the back-end compilation. This is through some new options to -plugin-opt (in tools::AddGoldPlugin()). Note:

  • For CSPGO instrumentation, it is guarded by CSPGOGenerateArg.
  • For PGO use, I unconditionally add -plugin-opt=cs-profile-path=<> because I don't want to open profile. I could read the profile and check if there is CS counters. But that needs to open the profile file.

The change to lib/CodeGen/CodeGenModule.cpp is for llvm profile summary change.

xur updated this revision to Diff 176207.Nov 30 2018, 2:52 PM

Add a testcase to test the options

davidxl added inline comments.Nov 30 2018, 3:27 PM
include/clang/Frontend/CodeGenOptions.h
330 ↗(On Diff #175926)

The comment is not precise.

lib/CodeGen/BackendUtil.cpp
694

what is going to happen if user uses a merged data with CS profile in profile-use and also specifies CS prof-gen?

964–965

Explain the change at this line.

966

Can you explain the change at this line?

982

should emit error message instead.

1273

please add +tejohnson for reviewing thinlto related pieces.

lib/Frontend/CompilerInvocation.cpp
616

CSIRInstr and IRInstr are not exclusive to each other, so why use if-then-else here?

xur marked 7 inline comments as done.Dec 3 2018, 9:40 AM
xur added inline comments.
include/clang/Frontend/CodeGenOptions.h
330 ↗(On Diff #175926)

Fixed

lib/CodeGen/BackendUtil.cpp
694

Good question. As of this patch, the behavior is different for the legacy pass manager and the new pass manager.
For the legacy pass manager (the code here), we will call normal PGOUsePass, CSPGOGenPass, and CSPGOUsePass.
For the new pass manager, we will report error of
Assertion `!CodeGenOpts.hasProfileCSIRUse() && "Cannot have both CSProfileUse and CSProfileGen at the same time"' failed.

The behavior for legacy pass manager is wrong. I will fix.

966

My earlier version will have two new booleans (bool RunCSProfileGen and bool RunCSProfileUse).
Here I combined the following 3 field in PGOOptions struct

bool RunProfileGen;
bool RunCSProfileGen;
bool RunCSProfileUse;

into an enum. There three are mutually exclusive.

lib/Frontend/CompilerInvocation.cpp
616

For CodeGenOptions kind, CSIRInstr and IRinstr are exclusive -- there is just one enum for the kind.

xur updated this revision to Diff 176428.Dec 3 2018, 10:33 AM
xur marked an inline comment as done.

Integrated David's review comments. Also add a test to test the error when both CSInstrumentGen and CSInstrumentUse on.

tejohnson added inline comments.Dec 3 2018, 11:50 AM
lib/CodeGen/BackendUtil.cpp
1273

This part looks fine (haven't examined the rest in detail). Please add a test. There are some existing ones for the ThinLTO backend via clang in test/CodeGen/thinlto-* that you can look at for examples.

xur updated this revision to Diff 177974.Dec 12 2018, 4:33 PM

Added a thinlto test case and fixed a few issues.

xur updated this revision to Diff 178484.Dec 17 2018, 9:38 AM

Updated the patch to sync with the updates in https://reviews.llvm.org/D54175

lebedev.ri set the repository for this revision to rC Clang.Dec 17 2018, 9:39 AM
lebedev.ri added a project: Restricted Project.
lebedev.ri added a subscriber: cfe-commits.
lebedev.ri added a subscriber: lebedev.ri.

Clang reviews should mainly go to cfe-commits, not llvm-commits. (all that will happen automagically if one sets the correct repo for the differential..)

xur updated this revision to Diff 181587.Jan 14 2019, 10:25 AM

Update the patch to sync with https://reviews.llvm.org/D54175

xur updated this revision to Diff 186023.Feb 8 2019, 11:54 AM

Updated the patch to sync with D54175

LGTM except for place noted below where I disagree with a change made earlier. Will let @davidxl chime in if he disagrees with me or has any other comments.

lib/CodeGen/BackendUtil.cpp
982

I disagree with David's comment here. All the error checking should (and is) being done in Clang.cpp where the options are being processed. These and the places above for the old PM should just be asserts.

Please add changes to option documentation.

xur updated this revision to Diff 188955.Mar 1 2019, 12:07 PM

Integrated Teresa's suggestion to change the err() to assert.
Added documentation change suggested by David.

davidxl added inline comments.Mar 4 2019, 8:33 AM
docs/UsersManual.rst
1792 ↗(On Diff #188955)

Please add a use example to show the typical flow with this option (both gen and use phases -- including llvm-profdata step).

xur updated this revision to Diff 189167.Mar 4 2019, 10:49 AM

Added usage example to UserManual.rst suggested by David.

davidxl accepted this revision.Mar 4 2019, 11:06 AM

lgtm

This revision is now accepted and ready to land.Mar 4 2019, 11:06 AM
This revision was automatically updated to reflect the committed changes.