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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits