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

[David Li via cfe-commits]

davidxl added inline comments.

================
Comment at: lib/Driver/Tools.cpp:5520
@@ +5519,3 @@
+    A->claim();
+    if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") &&
+        (Args.hasArg(options::OPT_fprofile_instr_generate) ||
----------------
silvas wrote:
> This is definitely not the right thing to do. Don't hijack -Xclang (which is a completely generic thing).
Why do we need special handling here ? will the existing behavior (simple forwarding) work?

Note that intention of the cc1 option is to hide it from the user but make it used by the developers -- so we can make assumption that this option is used only when -fprofile-instr-generate[=...] is specified. You can add diagnostics later during cc1 option processing if it is not the case.

Also think about making it easier to flip the default behavior in the future, it might be better to make the cc1 option look like this:

-fprofile-instrumentor=[clang|LLVM]

if the option is missing, the current default will be 'clang'. In the future just switch it to 'llvm'.

================
Comment at: lib/Frontend/CompilerInvocation.cpp:483
@@ +482,3 @@
+    Opts.ProfileIRInstr = true;
+  else
+    // Opts.ClangProfInstrGen will be used for Clang instrumentation only.
----------------
silvas wrote:
> This still isn't factored right. I think at this point the meaning of the driver-level options is not really useful at CC1 level (too convoluted) for it to be useful to pass them through.
> 
> The right thing to do here is probably more like:
> - refactor so that we have 4 individual CC1 options for InstrProfileOutput, InstrProfileInput, ProfileIRInstr, ClangProfInstrGen (although probably rename ClangProfInstrGen and ProfileIRInstr to be consistent with each other, e.g. "ProfileIRInstr" and "ProfileClangInstr").
> - add logic in Driver to convert from the driver-level options to the CC1 options.
It is already pretty close to your proposal -- the only missing thing is cc1 option for ClangProfInstrGen. However given that IR and Clang InstrGen are mutually exclusive, is an additional CC1 option needed?


http://reviews.llvm.org/D15829