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
