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

[Sean Silva via cfe-commits]

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

================
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) ||
----------------
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}`

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

================
Comment at: test/CodeGen/pgo-instrumentation.c:9
@@ +8,2 @@
+// RUN: %clang -O2 -c -Xclang -fprofile-ir-instr %s -mllvm -debug-pass=Structure 2>&1 | FileCheck %s -check-prefix=CHECK-PASS2
+// CHECK-PASS2-NOT: PGOInstrumentationGenPass
----------------
Please test both Gen and Use phases.

Also, please use more descriptive CHECK names.


http://reviews.llvm.org/D15829