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

[David Li via cfe-commits]

davidxl 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.
----------------
xur wrote:
> 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.
yes, you can change ProfileInstrGenerate to something like ClangProfInstrGen as a NFC first.

================
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) ||
----------------
xur wrote:
> 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.
If you rename the FE PGO CodeGen opt name as proposed, it will be clearer and less confusing.


http://reviews.llvm.org/D15829