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

[Rong Xu via cfe-commits]

xur added inline comments.

================
Comment at: lib/Driver/Tools.cpp:3279
@@ -3278,1 +3278,3 @@
+
+  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
 }
----------------
mcrosier wrote:
> I don't think AddAllArgs is what you really want.  What if the user specifies the option twice?  Do we really want to pass the flag from the driver to the front-end twice?  Also, should we warn if the option is passed twice?
> 
> I also think some of the logic in CompilerInvocation should land here...  see comments below.
Thanks for pointing thi out. What about add a guard, like:
-  Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
+  if (Args.hasArg(options::OPT_fprofile_ir_instr))
+    Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);

But looking at it again, I think i need to remove this stmt in the most recently patch as OPT_profile_ir_instr is now a CC1 option. it will not be seen here, right?



================
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) ||
----------------
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.


http://reviews.llvm.org/D15829