xur added inline comments. ================ Comment at: lib/Driver/Tools.cpp:3279 @@ -3278,1 +3278,3 @@ + + Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr); } ---------------- mcrosier wrote: > I don't think AddAllArgs is what you really want. What if the user specifies > the option twice? Do we really want to pass the flag from the driver to the > front-end twice? Also, should we warn if the option is passed twice? > > I also think some of the logic in CompilerInvocation should land here... see > comments below. Thanks for pointing thi out. What about add a guard, like: - Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr); + if (Args.hasArg(options::OPT_fprofile_ir_instr)) + Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr);
But looking at it again, I think i need to remove this stmt in the most recently patch as OPT_profile_ir_instr is now a CC1 option. it will not be seen here, right? ================ Comment at: lib/Frontend/CompilerInvocation.cpp:482 @@ +481,3 @@ + Opts.ProfileIRInstr = Args.hasArg(OPT_fprofile_ir_instr) && + (Args.hasArg(OPT_fprofile_instr_generate) || + Args.hasArg(OPT_fprofile_instr_generate_EQ) || ---------------- mcrosier wrote: > IIRC, the general practice is to put this type of logic in the driver. > Specifically, in Driver.cpp include something like > > if (Args.hasArg(OPT_fprofile_ir_instr) && > (Args.hasArg(OPT_fprofile_instr_generate) || > Args.hasArg(OPT_fprofile_instr_generate_EQ) || > Args.hasArg(OPT_fprofile_instr_use_EQ))) > // Add -fprofile_ir_instr flag.... > > In the most recent patch (Suggested by Sean as a temporary measure to speed up the code review) where -fprofiel-ir-instr becomes a CC1 only option, I have to do it here. Driver won't see the OPT_fprofile_ir_instr. But I'll remember you point when this becomes a Driver option in the next step. Also note that the driver could convert "-fprofile-genearte" to "-fprofiel-instr-generate" and also for "-fprofile-use". So the condition will be longer if we move to driver. http://reviews.llvm.org/D15829 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
