[PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation
Sean Silva via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 27 17:14:01 PST 2016
silvas added inline comments.
================
Comment at: include/clang/Frontend/CodeGenOptions.def:106
@@ -105,3 +105,3 @@
-CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate
- ///< execution counts to use with PGO.
+CODEGENOPT(ProfileClangInstr, 1, 0) ///< Clang Instrumentation to generate
+ ///< execution counts to use with PGO.
----------------
davidxl wrote:
> For the sake of being consistent with the new cc1 option, making this an enum option is better:
>
> ENUM_CODEGENOPT(ProfileInstr,... ) which takes two legal values ProfInstrClang, ProfInstrLLVM
>
> I'd like to hear Sean's opinion on this.
SGTM. I was going to suggest something similar. This is more consistent with e.g. OPT_debug_info_kind_EQ and the other multi-value options.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:150
@@ -149,3 +149,3 @@
- if (!CodeGenOpts.InstrProfileInput.empty()) {
+ if (!CodeGenOpts.ProfileIRInstr && !CodeGenOpts.InstrProfileInput.empty()) {
auto ReaderOrErr =
----------------
This seems like a latent bug: `!CodeGenOpts.ProfileIRInstr` is true when we are doing no instrumentation. Using an ENUM_CODEGENOPT will fix this and related issues.
================
Comment at: lib/Frontend/CompilerInvocation.cpp:495
@@ +494,3 @@
+ }
+ } else {
+ // Default PGO instrumentor is Clang instrumentation.
----------------
The work being done in this `else` is redundant. `-fprofile-instrumentor={clang,llvm,none}` (with "none" being the default if the argument isn't passed) covers this case as `-fprofile-instrumentor=clang`. The driver can be changed to translate `-fprofile-instr-generate` into `-fprofile-instrumentor=clang`. (and `-fprofile-instrument=` might be a better name)
http://reviews.llvm.org/D15829
More information about the cfe-commits
mailing list