xur added a comment. I'll send an updated patch shortly.
================ 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. ---------------- silvas wrote: > 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. OK. I'll use ENUM as you two suggested. ================ Comment at: lib/CodeGen/CodeGenModule.cpp:150 @@ -149,3 +149,3 @@ - if (!CodeGenOpts.InstrProfileInput.empty()) { + if (!CodeGenOpts.ProfileIRInstr && !CodeGenOpts.InstrProfileInput.empty()) { auto ReaderOrErr = ---------------- silvas wrote: > 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. I don't think this is a bug. As I mentioned in the update comments. I still use this option (-fprofile-instrumentor=) in profile-use compilation. Once we have the llvm-profdata patch in, we can detect the profile kind and remove this. ================ Comment at: lib/Frontend/CompilerInvocation.cpp:495 @@ +494,3 @@ + } + } else { + // Default PGO instrumentor is Clang instrumentation. ---------------- silvas wrote: > 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) Only if we changed the driver and pushed in -fprofile-instrumentor to the cc1 argument list, the else part is indeed redundant. But since I did not change the driver options handling, we still need this else branch -- we only set ProfileClangInstr when there is OPT_fprofile_instr_generate. I'll change the -fprofile-instrumentor to -fprofile-instrument and change the driver to push in this argument. I will probably remove OPT_fprfoile_instr_generate as a CC1 option. http://reviews.llvm.org/D15829 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits