This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] profile feedback: -fprofile-use= and -fprofile-correction, correctness fixes and option semantic changes.
On Sat, Mar 29, 2008 at 5:25 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> so now about the other patch.
> > diff -r 986583ed535e gcc/common.opt
> > --- a/gcc/common.opt Wed Mar 19 14:42:54 2008 -0700
> > +++ b/gcc/common.opt Thu Mar 20 10:01:56 2008 -0700
> > @@ -811,6 +811,10 @@ Common Report Var(profile_arc_flag)
> > Common Report Var(profile_arc_flag)
> > Insert arc-based program profiling code
> >
> > +fprofile-correction
> > +Common Report Var(flag_profile_correction)
> > +Enable correction of flow inconsistent profile data input
>
> You are missing texinfo docs here. I would add few words that this
> option is meant to help ignore race conditions when profiling threaded
> programs. Once Michael's atomic counters is in, we could have the
> options cross referenced.
>
> Also could you please add symmetric option to GCOV implementation?
I'm not sure it makes sense to do this for gcov - I'd think it's
probably better off
to leave gcov output as simply inconsistent.
The reason I made the counters artificially consistent is because
some of transformations that goes haywire when the flow consistency is
not correct.
There were enough of those that I didn't try to find all those places and
make them tolerate inconsistencies (I may very well try to do that later,
if smoothing, even with more sophisticated algorithms, doesn't work out well.
> > +/* Make the block/edge counts of the entire CFG flow-consistent.
> > + This is one super hack. The problem is difficult to solve well
> > + but we rely on compute_all_counts()
> > + to leave no negative number and repeatedly apply the backward propagation
> > + of counts, until everything's consistent. */
>
> We can either have this hack, or we can simply disable the optimization
> and emit counters on each edge as done by some compilers... I guess
> experience will show how much off the result can be and how much we
> care.
We can take that approach, but it at least requires more testing/code inspection
to weed out all places in the compiler (e.g. I found some code, e.g. i386.c,
that breaks if it's fed flow-inconsistent edge counts).
> > @@ -335,6 +338,16 @@ tree_gen_ic_func_profiler (void)
> > cur_func,
> > ic_void_ptr_var);
> > bsi_insert_after (&bsi, stmt1, BSI_NEW_STMT);
> > +
> > + gcc_assert (EDGE_COUNT (bb->succs) == 1);
> > + bb = split_edge (EDGE_I (bb->succs, 0));
> > + bsi = bsi_start (bb);
> > + /* Set __gcov_indirect_call_callee to 0,
> > + so that calls from other modules won't get misattributed
> > + to the last caller of the current callee. */
> > + void0 = build_int_cst (build_pointer_type (void_type_node), 0);
> > + stmt2 = build_gimple_modify_stmt (ic_void_ptr_var, void0);
> > + bsi_insert_after (&bsi, stmt2, BSI_NEW_STMT);
>
> This is unrealted, right? Please put it in as separate patch. It is OK.
Yes. I'll check in as a separate patch.
> > diff -r 986583ed535e gcc/tree-ssa-propagate.c
> > --- a/gcc/tree-ssa-propagate.c Wed Mar 19 14:42:54 2008 -0700
> > +++ b/gcc/tree-ssa-propagate.c Thu Mar 20 10:01:56 2008 -0700
> > @@ -40,6 +40,7 @@
> > #include "langhooks.h"
> > #include "varray.h"
> > #include "vec.h"
> > +#include "value-prof.h"
> >
> > /* This file implements a generic value propagation engine based on
> > the same propagation used by the SSA-CCP algorithm [1].
> > @@ -734,6 +735,7 @@ set_rhs (tree *stmt_p, tree expr)
> > /* Replace the whole statement with EXPR. If EXPR has no side
> > effects, then replace *STMT_P with an empty statement. */
> > ann = stmt_ann (stmt);
> > + gimple_remove_stmt_histograms (cfun, stmt);
>
> What are you shooting for here?
> I think arbitrary replacement is not invalidating the histogram, in most
> propagation algorithms it will be the same value, just computed
> differently. The histograms can be attached to the values in LHS or
> RHS, so we probably would need to do some more cureful work.
Without this change, we can end up with a dangling histogram
(because value histograms are kept in a separate hash table
key'ed off of the statement pointer values themselves).
The buggy case I encountered was when a memmove() call
with a histogram attached is replaced with nop.
The histogram checking later finds the dangling histogram
and leads to ICE.
I can't imagine value histograms being useful
after a wholesale replacement of the entire statement,
but I can simply reattach the histogram, if you think that's more correct.
> Otherwise OK, but please post patch with gcov change too.
>
> Honza
Thanks!
Seongbae