Re: [PATCH] profile feedback: -fprofile-use= and -fprofile-correction, correctness fixes and option semantic changes.

["Seongbae Park (박성배, 朴成培)" <seongbae dot park at gmail dot com>]

On Wed, Mar 26, 2008 at 5:21 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi profile feedback maintainer(s?),
>  Hi,
>
> > The first new option -fprofile-use= allows profile feedback data files
>  > (gcda files) to be picked up from different directories than where
>  > object files are.
>  > This makes it more convenient to use FDO
>  > for programs that are not run on the same system as the build system
>  > and also makes it easier to maintain feedback datafiles separately
>  > from the object files (which are usually placed in some scratch area).
>
>  I am not quite sure I like -fprofile-use= scheme. I see it is quite
>  convenient for users, but the problem is that -fprofile-use is really a
>  shortcut for several profiling options and optimization flags (pretty
>  much as -O2 is).  It would probably make more sense to have independent
>  parameter for that liek -fprofile-dir.  We can maintain
>  -fprofile-use=<dir> as a shortcut for -fprofile-use -fprofile-dir=<dir>
>  but having it independently is IMO quite important at least for
>  development.

>  > The other new option -fprofile-datafile-relative-path prevents
>  > gcc from adding getpwd() to the baked gcda file path in each object file.
>  > This has two benefits: one is that the content of the object files are no more
>  > dependent on the absolute path of the object files - hence
>  > we have one less file location dependent contents in the object
>  > making the compiler more "purely functional" - i.e. the compiler output
>  > don't change unless any input files change.
>  > Another benefit is that you don't need to use path dependent
>  > GCOV_PREFIX/GCOV_PREFIX_STRIP options at the profile collection run time
>  > to redirect the profile data files somewhere other than the original
>  > object paths, and thus makes it easier to share the same profile
>  > collected from object built from one particular path to be shared
>  > by builds running on different paths.
>  > Combined with the new -fprofile-use=,
>  > this option makes it easier to switch between pre-collected feedback data files.
>
>  Hmm, perhaps if we go for -fpeofile-dir, we can use it on generation
>  stage too and -fprofile-data-relative-path would translate to something
>  like -fprofile-dir=""?

Ok. The current behavior is essentially -fprofile-dir=`pwd`.
I think this makes it easier to explain to the user.
So I'll make -fprofile-dir= and make -fprofile-use= a shortcut for both.

>  > The patch also changes the behavior regarding when we read gcda files.
>  > Currently, we *always* read gcda file if it exists where we expect it to be.
>  > The patch makes it so that we'll read gcda files
>  > if and only if -fprofile-use option is explicitly used.
>  > This avoids the compiler from trying to access another file that
>  > usually does not exist,
>  > and even when exists, users may NOT want to use it.
>
>  I defnitly like this.  I always wondered why the gcda files was always
>  opened, but since it was not my invention I never took tome to figure
>  out why it is done so.  Definitly when not asked to use the profile,
>  we should not look for it.
>
>  > Lastly, the patch disables writing of .gcno files unless
>  > -ftest-coverage option is used.
>
>  This is OK too. Separating these two changes from the patch would be
>  pre-approved.  Lets discuss more the command line interface for the first
>  part of the patch.
>
>  > The second patch (profile-correction) adds a new option -fprofile-correction
>  > which will essentially smooth out any small inconsistencies caused by
>  > missed counter updates in multithreaded profile collection.
>  > Using atomic updates for counter increment adds significant overhead,
>  > especially as the core count grows.
>
>  I will need to play with this more tomorrow.  I would like to have the
>  atomic operation being the default solution for coverage (I tought
>  Zdenek had a patch for this, but it is not in mainline, I will need to
>  dig out the history here).  Allowing error in profiles kills last way to
>  do some consistency checking and also since we optimize away most of
>  counters the errors can propagate to quite important mispredictions...

I actually had my own patch of atomic increments (almost 2 years ago, already)
but I didn't send it out because someone else said they were about to submit it
(or maybe even submitted, don't remember now).
But using atomic increment on highly multithreaded programs on high
core count systems
incur significant performance overhead.
So I'd rather like to see TLS counters than atomic increments.
Regardless, this option (-fprofile-correction) is completely
orthogonal to either atomic increment or TLS.

> > Making counters TLS adds more memory overhead
>  > and more complicated interaction with threading system.
>  > This ad-hoc correction algorithm is an interim solution
>  > until we have more proper algorithm implemented
>  > (based on variants of minimum cost network flow algorithms),
>
>  Do you have some reference here?

http://www.springerlink.com/content/j444437052584j16/

>  Thanks and sorry for the delay,
>  Honza

Thanks for the review.

Seongbae