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

Create a new review here: http://reviews.llvm.org/D17622 Thanks, -Rong On Wed, Feb 24, 2016 at 9:22 PM, Sean Silva wrote: > silvas added a comment. > > In http://reviews.llvm.org/D15829#360006, @xur wrote: > >> Here is the new patch that removes the auto detection of

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

xur added a comment. Create a new review here: http://reviews.llvm.org/D17622 Thanks, -Rong http://reviews.llvm.org/D15829 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

silvas added a comment. In http://reviews.llvm.org/D15829#360006, @xur wrote: > Here is the new patch that removes the auto detection of profile kind. > > In this patch, I replace the CC1 option of -fprofile-instr-use=<> with > -fprofile-instrument={llvm-use|clang-use}. For the use compilation,

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

davidxl added a comment. Looks good to me -- and it makes the profile-gen and profile-use's cc1 option handling consistent. Please check with Sean or Justin just in case before proceeding. http://reviews.llvm.org/D15829 ___ cfe-commits mailing

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

xur updated this revision to Diff 48859. xur marked 2 inline comments as done. xur added a comment. Integrated with David's comments. Thanks, -Rong http://reviews.llvm.org/D15829 Files: include/clang/Driver/CC1Options.td include/clang/Driver/Options.td

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

xur marked 4 inline comments as done. Comment at: lib/CodeGen/CodeGenModule.cpp:151 @@ -151,1 +150,3 @@ + if (CodeGenOpts.hasProfileClangUse() && + !CodeGenOpts.InstrProfileInput.empty()) { auto ReaderOrErr = davidxl wrote: > Is the second condition

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

xur updated this revision to Diff 48846. xur marked an inline comment as done. xur added a comment. Here is the new patch that removes the auto detection of profile kind. In this patch, I replace the CC1 option of -fprofile-instr-use=<> with -fprofile-instrument={llvm-use|clang-use}. For the

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

xur marked an inline comment as done. xur added a comment. I'll send a revised patch soon. Comment at: lib/CodeGen/CodeGenModule.cpp:150 @@ -149,2 +149,3 @@ - if (!CodeGenOpts.InstrProfileInput.empty()) { + if (!CodeGenOpts.hasProfileIRInstr() && +

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

silvas added a comment. Apologies for the delay. At this point, I think that this patch has evolved enough that it is best to start a new patch. I think the steps forward are: - Have cc1 accept -fprofile-instrument=llvm (requires no driver changes, but is enough for developers to test by being

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

davidxl added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:150 @@ -149,2 +149,3 @@ - if (!CodeGenOpts.InstrProfileInput.empty()) { + if (!CodeGenOpts.hasProfileIRInstr() && + !CodeGenOpts.InstrProfileInput.empty()) { Better to use if

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

xur updated this revision to Diff 47331. xur added a comment. Ping (also added the test that missed from last upload). Thanks, -Rong http://reviews.llvm.org/D15829 Files: include/clang/Driver/CC1Options.td include/clang/Frontend/CodeGenOptions.h lib/CodeGen/BackendUtil.cpp

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

xur updated this revision to Diff 47251. xur marked an inline comment as done. xur added a comment. fixed the typo in comments http://reviews.llvm.org/D15829 Files: include/clang/Driver/CC1Options.td include/clang/Frontend/CodeGenOptions.h lib/CodeGen/BackendUtil.cpp

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

davidxl added inline comments. Comment at: include/clang/Frontend/CodeGenOptions.h:234 @@ +233,3 @@ + + /// \brief Check if IR profile instrumenation is on. + bool hasProfileIRInstr() const { typo: instrumentation http://reviews.llvm.org/D15829

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

xur updated this revision to Diff 46960. xur added a comment. Now http://reviews.llvm.org/D16730 has been committed (as r259811) Here is the patch that adds cc1 option -fprofile-instrument=llvm to enable IR level PGO generate and use. The pgo use part of the patch depends

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

On Wed, Feb 3, 2016 at 12:40 PM, Bob Wilson wrote: > > On Feb 3, 2016, at 12:23 PM, Xinliang David Li wrote: > > On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson wrote: > > > On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits >

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

On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson wrote: > >> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits >> wrote: >> >> silvas added a comment. >> >> In http://reviews.llvm.org/D15829#333902, @davidxl wrote: >> >>> For the longer term,

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

> On Feb 3, 2016, at 12:23 PM, Xinliang David Li wrote: > > On Tue, Feb 2, 2016 at 1:31 PM, Bob Wilson > wrote: >> >>> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits >>> wrote: >>>

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

> On Jan 22, 2016, at 1:43 PM, Sean Silva via cfe-commits > wrote: > > silvas added a comment. > > In http://reviews.llvm.org/D15829#333902, @davidxl wrote: > >> For the longer term, one possible solution is to make FE based >> instrumentation only used for

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

xur updated this revision to Diff 46389. xur added a comment. This new patch adds the change the clang test cases (cc1 option -fprofile-instr-generate to -fprofile-instrument=Clang). It also replaces cc1 option -fprofile-instr-generate= to -fprofile-instrument-path=, as suggested by David.

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

xur added a comment. To make the review easier, I split the cc1 names change part into a new NFC patch here. http://reviews.llvm.org/D16730 http://reviews.llvm.org/D15829 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

davidxl added inline comments. Comment at: lib/Driver/Tools.cpp:3232 @@ -3231,2 +3231,3 @@ CmdArgs.push_back( Args.MakeArgString(Twine("-fprofile-instr-generate=") + Path)); +} I also suggest changing CC1 option -fprofile-instr-generate= to

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

The previous patch was not good as it failed 58 tests that use -fprofile-instr-generate as a cc1 option. I can see two methods to solve this: (1) we change all the 58 tests to use -fprofile-instrument=Clang option. (2) Fall back to my previous patch: keep -fprofile-instr-generate as the CC1

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

On Thu, Jan 28, 2016 at 2:34 PM, Rong Xu wrote: > The previous patch was not good as it failed 58 tests that use > -fprofile-instr-generate as a cc1 option. > > I can see two methods to solve this: > (1) we change all the 58 tests to use -fprofile-instrument=Clang option. My

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

xur added a comment. The previous patch was not good as it failed 58 tests that use -fprofile-instr-generate as a cc1 option. I can see two methods to solve this: (1) we change all the 58 tests to use -fprofile-instrument=Clang option. (2) Fall back to my previous patch: keep

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

xur updated this revision to Diff 46303. xur added a comment. Integrated most recently comments/suggestions from David and Sean. Thanks, -Rong http://reviews.llvm.org/D15829 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/CC1Options.td

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

xur added a comment. I'll send an updated patch shortly. Comment at: include/clang/Frontend/CodeGenOptions.def:106 @@ -105,3 +105,3 @@ -CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate -///< execution counts to

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

davidxl added inline comments. Comment at: include/clang/Frontend/CodeGenOptions.def:106 @@ -105,3 +105,3 @@ -CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate -///< execution counts to use with PGO.

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

silvas added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:483 @@ +482,3 @@ +Opts.ProfileIRInstr = true; + else +// Opts.ClangProfInstrGen will be used for Clang instrumentation only. davidxl wrote: > silvas wrote: > > This still isn't

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

xur updated this revision to Diff 46199. xur added a comment. This new version implemented the usage model David proposed. We now have a cc1 option -fprofile-instrumentor={llvm | clang} to choose which instrumentation to use. The slight difference from David's proposal is that we can use this

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

silvas added inline comments. Comment at: include/clang/Frontend/CodeGenOptions.def:106 @@ -105,3 +105,3 @@ -CODEGENOPT(ProfileInstrGenerate , 1, 0) ///< Instrument code to generate -///< execution counts to use with PGO.

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

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

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

Sean: Adding a new CC1 option ProfileClangInstr will make things cleaner. But this won't solve the problem. The root of all the mess is there is no driver level option for IR instrumentation. I need to either "hijack" the -Xclang option, or move the logic to CompilerInvocation.cpp, which both you

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

On Wed, Jan 27, 2016 at 11:31 AM, David Li wrote: > davidxl added inline comments. > > > Comment at: lib/Driver/Tools.cpp:5520 > @@ +5519,3 @@ > +A->claim(); > +if ((StringRef(A->getValue(0)) == "-fprofile-ir-instr") && > +

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

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[=] 2) To turn on clang

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

silvas 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) || This is definitely not the right

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

On Fri, Jan 22, 2016 at 1:43 PM, Sean Silva wrote: > silvas added a comment. > > In http://reviews.llvm.org/D15829#333902, @davidxl wrote: > >> For the longer term, one possible solution is to make FE based >> instrumentation only used for coverage testing which can be

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

xur added a subscriber: xur. xur added a comment. OK. Now I understand what you meant. I was trying to avoid driver changes. It seems you are saying I can change the driver to support cc1 options more cleanly. This can be done. Thanks, Rong http://reviews.llvm.org/D15829

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

OK. Now I understand what you meant. I was trying to avoid driver changes. It seems you are saying I can change the driver to support cc1 options more cleanly. This can be done. Thanks, Rong On Jan 25, 2016 12:43, "Sean Silva" wrote: > silvas added inline comments. > >

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

silvas added inline comments. 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) ||

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

On Mon, Jan 25, 2016 at 12:52 PM, Rong Xu wrote: > OK. Now I understand what you meant. I was trying to avoid driver changes. > It seems you are saying I can change the driver to support cc1 options more > cleanly. This can be done. > Great! Sorry for the confusion. -- Sean

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

On Mon, Jan 25, 2016 at 11:00 AM, Xinliang David Li wrote: > On Fri, Jan 22, 2016 at 1:43 PM, Sean Silva wrote: > > silvas added a comment. > > > > In http://reviews.llvm.org/D15829#333902, @davidxl wrote: > > > >> For the longer term, one possible

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

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

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

xur updated this revision to Diff 45929. xur marked an inline comment as done. xur added a comment. Here is the new patch. Changes from previous patch are: (1) rename codegen option ProfileInstrGenerate to ClangProfInstrGen to avoid the name confusion (Davidxl). This option is for clang

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

silvas added inline comments. Comment at: lib/Driver/Tools.cpp:3279 @@ -3278,1 +3278,3 @@ + + Args.AddAllArgs(CmdArgs, options::OPT_fprofile_ir_instr); } xur wrote: > mcrosier wrote: > > I don't think AddAllArgs is what you really want. What if the user > >

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

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

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

silvas added a comment. In http://reviews.llvm.org/D15829#333902, @davidxl wrote: > For the longer term, one possible solution is to make FE based > instrumentation only used for coverage testing which can be turned on > with -fcoverage-mapping option (currently, -fcoverage-mapping can not >

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

davidxl added a comment. For the longer term, one possible solution is to make FE based instrumentation only used for coverage testing which can be turned on with -fcoverage-mapping option (currently, -fcoverage-mapping can not be used alone and must be used together with

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

xur updated this revision to Diff 45708. xur added a comment. This new patches integrates Sean review comments: (1) make -fprofile-ir-instr a cc1 option instead of driver option. (2) add one cc1 option test, and test the pass is indeed invoked. (3) remove the driver test (runtime library). (4)

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

mcrosier added a subscriber: mcrosier. mcrosier added a comment. Would it make sense to include an additional test (in test/Driver) that shows the -fprofile-ir-instr option being passed from the driver to the frontend? Such a test case would land in clang_f_opt.c, which has many examples.

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

On Thu, Jan 21, 2016 at 7:32 PM, Sean Silva wrote: > silvas added a comment. > > @slingn and I had a discussion offline about the potential names and came up > with some ideas, but none is a clear winner. > > Overall, my feeling is that from a user's perspective, the

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

silvas 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. Then change the existing references in

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

xur added a comment. Ping. Passmanagerbuilder change has been committed. Could you take a look at this command line option patch? Thanks, -Rong http://reviews.llvm.org/D15829 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

silvas added a comment. This needs tests showing that the IR gen/use passes get run. Maybe use -debug-pass=Structure like test/CodeGen/thinlto_backend.c? My biggest concern is the naming and user visible parts. I can't come up with anything better than `-fprofile-ir-instr` TBH. Overall, from a

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

silvas added a comment. @slingn and I had a discussion offline about the potential names and came up with some ideas, but none is a clear winner. Overall, my feeling is that from a user's perspective, the frontend stuff is probably best referred to as "coverage-based". It's not as clear for

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

silvas added inline comments. Comment at: include/clang/Frontend/CodeGenOptions.def:108 @@ -107,2 +107,3 @@ ///< execution counts to use with PGO. +CODEGENOPT(ProfileIRInstr, 1, 0) ///< IR level code PGO instrumentation and use.

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

silvas added a comment. @bogner btw, did you say that at Apple you guys have a requirement of supporting existing profdata? I.e. users can pass older profdata to a newer compiler? Realistically, it would be nice if our PGO offering defaulted to the IR stuff (since it seems like it is going to

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

davidxl added a comment. Should add a test case in test/Driver/instrprof-ld.c. Comment at: lib/CodeGen/BackendUtil.cpp:435 @@ +434,3 @@ + if (CodeGenOpts.ProfileIRInstr) { +assert (!CodeGenOpts.ProfileInstrGenerate); +if (!CodeGenOpts.InstrProfileOutput.empty())

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

xur created this revision. xur added reviewers: davidxl, silvas, bogner. xur added a subscriber: cfe-commits. This patch introduce a new toggle option -fprofile-ir-instr that enables IR level instrumentation. It needs to be used in combination with current PGO options. Without an existing PGO