We first need to nail the use model of the option, and then talk about implementation. To clarify, I think the following should be the way:
(assuming the current clang instrumetnation is the default): 1) To turn on clang based instrumentation: -fprofile-instr-generate[=<path>] 2) To turn on clang based instrumentation explicitly: -fprofile-instr-generate[=<path>] -Xclang -fprofile-instrumentor=clang 3) To turn on IR based instrumentation: -fprofile-instr-generate[=<path>] -Xclang -fprofile-instrumentor=llvm There is no need to change driver handling -- just let it forward. In CompilerInvocation.cpp, diagnostics can be emitted if user does -Xclang -fprofile-instrumentor=xxx without specfiying -fprofile-instr-generate Handling of -fprofile-instr-use=<> is different which does not depend on -fprofile-instrumentor, so it should be done in a different patch. David On Wed, Jan 27, 2016 at 11:38 AM, Rong Xu <x...@google.com> wrote: > On Wed, Jan 27, 2016 at 11:31 AM, David Li <davi...@google.com> wrote: >> 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? > > The original patch does the simple forwarding. In that case, we handle > the cc1 options (choosing b/w IR or clang instrumentation) in > CompilerInvocation.cpp. Sean and Chad think this logic should be > driver. > >> >> 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 >> >> >> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
