On Fri, Jan 22, 2016 at 1:43 PM, Sean Silva <chisophu...@gmail.com> 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 turned on >> with -fcoverage-mapping option (currently, -fcoverage-mapping can not >> be used alone and must be used together with >> -fprofile-instr-generate). To summarize: >> >> A. Current behavior: >> >> ----------------------- >> >> 1. -fprofile-instr-generate turns on FE based instrumentation >> 2. -fprofile-instr-generate -fcoverage-mapping turns on FE based >> instrumentation and coverage mapping data generation. >> 3. -fprofile-instr-use=<..> assumes profile data from FE instrumentation. >> >> B. Proposed new behavior: >> >> -------------------------------- >> >> 1. -fprofile-instr-generate turns on IR late instrumentation >> 2. -fcoverage-mapping turns on FE instrumentation and coverage-mapping >> 3. -fprofile-instr-generate -fcoverage-mapping result in compiler warning >> 4. -fprofile-instr-use=<> will automatically determine how to use the >> profile data. > > > Very good observation that we can determine FE or IR automatically based on > the input profdata. That simplifies things. > >> B.2) above can be done today for improved usability. > > > I don't see how this improves usability. In fact, it is confusing because it > hijacks an existing option. > > Also B.3 causes existing user builds to emit a warning, which is annoying.
Since it is pointless to specify -fcoverage-mapping option alone, why not do not automatically? Yes it means some behavior change (in a good way) and some annoying warnings initially, but those are trivially fixable by the user. > > I would propose the following modification of B: > > C.: > > 1. -fprofile-instr-generate defaults to IR instrumentation (i.e. behaves > exactly as before, except that it uses IR instrumentation) > 2. -fprofile-instr-generate -fcoverage-mapping turns on frontend > instrumentation and coverage. (i.e. behaves exactly as before) I am fine with this -- as long as the user does not need to bear the burden of understanding where and how the instrumentation is done. > 3. -fprofile-instr-use=<> automatically determines which method to use > > All existing user workflows continue to work, except for workflows that > attempt to `llvm-profdata merge` some old frontend profile data (e.g. they > have checked-in to version control and represents some special workload) with > the profile data from new binaries. > > Concretely, imagine the following workflow: > > clang -fprofile-instr-generate foo.c -o foo > ./foo > llvm-profdata merge default.profraw -o new.profdata > llvm-profdata merge new.profdata > /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata > -o foo.profdata > clang -fprofile-instr-use=foo.profdata foo.c -o foo_pgo > > I think this is a reasonable breakage. We would need to add a note in the > release notes. Unfortunately this is not expected breakage if we claim to > have forward compatibility for profdata (which IIRC Apple requires; > @bogner?). But I think this case will be rare and exceptional enough that we > can tolerate it: > The old profile data has to out live the program source versions which usually change fast. In reality, I don't expect profile data to live too long. > - a simple immediate workaround is to specify `-fcoverage-mapping` (which > also adds some extra stuff, but works around the issue) This is a reasonable workaround > - Presumably > /versioncontrol/some-important-but-expensive-to-reproduce-workload.profdata > is regenerated with some frequency which is more frequent than upgrading the > compiler, and so it is likely reasonable to regenerate it alongside a > compiler upgrade, using the workaround until then. > > > >> B.1) needs a > >> transition period before the IR based instrumentation becomes > >> stablized (and can be flipped to the default). During the transition > >> period, the behavior of 1) does not change, but a cc1 option can be > >> used to turn on IR instrumentation (as proposed by Sean). > > > Just to clarify, users are not allowed to use cc1 options. The cc1 option is > purely for us as compiler developers to do integration and testing, put off > some discussions for later, etc. > yes. thanks, David > > http://reviews.llvm.org/D15829 > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
