Add new options for context-sensitive PGO.
Refer to https://reviews.llvm.org/D54175
davidxl | |
vsk | |
tejohnson |
Add new options for context-sensitive PGO.
Refer to 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.
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:
The change to lib/CodeGen/CodeGenModule.cpp is for llvm profile summary change.
include/clang/Frontend/CodeGenOptions.h | ||
---|---|---|
330 | The comment is not precise. | |
lib/CodeGen/BackendUtil.cpp | ||
690 | what is going to happen if user uses a merged data with CS profile in profile-use and also specifies CS prof-gen? | |
948 | Can you explain the change at this line? | |
954 | Explain the change at this line. | |
967 | should emit error message instead. | |
1244 | please add +tejohnson for reviewing thinlto related pieces. | |
lib/Frontend/CompilerInvocation.cpp | ||
504 | CSIRInstr and IRInstr are not exclusive to each other, so why use if-then-else here? |
include/clang/Frontend/CodeGenOptions.h | ||
---|---|---|
330 | Fixed | |
lib/CodeGen/BackendUtil.cpp | ||
690 | Good question. As of this patch, the behavior is different for the legacy pass manager and the new pass manager. The behavior for legacy pass manager is wrong. I will fix. | |
948 | My earlier version will have two new booleans (bool RunCSProfileGen and bool RunCSProfileUse). bool RunProfileGen; bool RunCSProfileGen; bool RunCSProfileUse; into an enum. There three are mutually exclusive. | |
lib/Frontend/CompilerInvocation.cpp | ||
504 | For CodeGenOptions kind, CSIRInstr and IRinstr are exclusive -- there is just one enum for the kind. |
Integrated David's review comments. Also add a test to test the error when both CSInstrumentGen and CSInstrumentUse on.
lib/CodeGen/BackendUtil.cpp | ||
---|---|---|
1244 | 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. |
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..)
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 | ||
---|---|---|
967 | 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. |
Integrated Teresa's suggestion to change the err() to assert.
Added documentation change suggested by David.
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). |