On Mon, Jan 25, 2016 at 11:00 AM, Xinliang David Li <davi...@google.com> wrote:
> 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. > "trivially fixable" needs to be weighed against the number of times it needs to be fixed across the user base. Anyway, emitting a warning for this is clearly a separate discussion (separate even from the discussion of whether to accept just `-fcoverage-mapping` without `-fprofile-instr-generate`), so let's not get hung up on it. > > > > > > 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. > Agreed. > > > 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. > I don't either, but it's not unthinkable. E.g. a user may have the sources of a dependency checked-in, and they only update the profdata for those sources when they update the dependency. But overall, I think that release notes mentioning the workaround for this situation is sufficient. -- Sean Silva > > > - 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
