[PATCH] D15829: [PGO] Clang Option that enables IR level PGO instrumentation

[Rong Xu via cfe-commits]

xur added inline comments.

================
Comment at: lib/CodeGen/BackendUtil.cpp:440
@@ +439,3 @@
+  if (CodeGenOpts.ProfileIRInstr) {
+    // Should not have ProfileInstrGenerate set -- it is for clang
+    // instrumentation only.
----------------
silvas wrote:
> Then change the existing references in clang. Please try to cleanly integrate the new functionality, not "sneak it in" with the smallest number of lines changed (and creating technical debt for future maintainers).
The integration is actually clean to me: For CodeGenOpt: ProfileInstrGenerate is for Clang instrumentation and ProfileIRInstr is for IR instrumentation. They need to mutually exclusive. 

Maybe I need to modify the name and description for  ProfileInstrGenerate to reflect the change.

================
Comment at: lib/Frontend/CompilerInvocation.cpp:482
@@ +481,3 @@
+  Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr) &&
+                        (Args.hasArg(OPT_fprofile_instr_generate) ||
+                         Args.hasArg(OPT_fprofile_instr_generate_EQ) ||
----------------
silvas wrote:
> xur wrote:
> > mcrosier wrote:
> > > IIRC, the general practice is to put this type of logic in the driver.  Specifically, in Driver.cpp include something like
> > > 
> > > if (Args.hasArg(OPT_fprofile_ir_instr) &&
> > >     (Args.hasArg(OPT_fprofile_instr_generate) ||
> > >      Args.hasArg(OPT_fprofile_instr_generate_EQ) ||
> > >      Args.hasArg(OPT_fprofile_instr_use_EQ)))
> > >   // Add -fprofile_ir_instr flag....
> > > 
> > > 
> > In the most recent patch (Suggested by Sean as a temporary measure to speed up the code review)  where -fprofiel-ir-instr becomes a CC1 only option, I have to do it here. Driver won't see the OPT_fprofile_ir_instr. 
> > 
> > But I'll remember you point when this becomes a Driver option in the next step. Also note that the driver could convert "-fprofile-genearte" to "-fprofiel-instr-generate" and also for "-fprofile-use".  So the condition will be longer if we move to driver.
> Chad makes a good point though. This should be just `Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr);` because we can assume that for cc11 this option is not used except in conjunction with `-fprofile-instr-{generate,use}`
If we can have the assumption. Yes I can simplify the stmt. If the user breaks the assumption. Current patch will not invoke the IR PGO pass, while the proposed way will cause error. Of course, you can say this is a usage error.

I have no problem to simplify this.

================
Comment at: lib/Frontend/CompilerInvocation.cpp:487
@@ +486,3 @@
+  // Opts.ProfileInstrGenerate will be used for Clang instrumentation only.
+  if (!Opts.ProfileIRInstr)
+    Opts.ProfileInstrGenerate = Args.hasArg(OPT_fprofile_instr_generate) ||
----------------
silvas wrote:
> I don't like this. It breaks the 1:1 mapping of flags to codegen options. Like Chad said, this sort of complexity should be kept in the driver.
> 
> Some refactoring may be needed to cleanly integrate the new IR instrumentation.
hmm. I don't think there is 1:1 mapping b/w flags and codegen options, because -fprofile-instr-generate is shared by IR and FE instrumentation.


http://reviews.llvm.org/D15829