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

Reply via email to