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 Thu, Apr 3, 2008 at 9:58 AM, Seongbae Park (박성배, 朴成培)
<seongbae.park@gmail.com> wrote:
>
> On Wed, Apr 2, 2008 at 11:39 PM, Jan Hubicka <jh@suse.cz> wrote:
>  > >
>  >  > Attached is the separate patch that preserves the value histogram
>  >  > when replacing a whole statement in set_rhs.
>  >  > I'm trying to reduce the testcase that causes the ICE
>  >  > but no luck so far - I'll add the testcase as soon as I can.
>  >  > Bootstrapped and regtested with other patches.
>  >  > Ok for mainline ?
>  >  >
>  >  > Seongbae
>  >  >
>  >  > 2008-04-02  Seongbae Park  <seongbae.park@gmail.com>
>  >  >
>  >  >         * tree-ssa-propagate.c (set_rhs): Preserve the histogram.
>  >  >         * value-prof.c (gimple_move_stmt_histograms): New function.
>  >  >         * value-prof.h (gimple_move_stmt_histograms): New function declaration.
>  >
>  >  The patch looks fine from histograms perspective, but it seems to me
>  >  that you also need something like
>  >
>  >   /* Preserve EH region information from the original statement, if
>  >      requested by the caller.  */
>  >   eh_region = lookup_stmt_eh_region (orig_stmt);
>  >   if (eh_region >= 0)
>  >     {
>  >       remove_stmt_from_eh_region (orig_stmt);
>  >       add_stmt_to_eh_region (stmt, eh_region);
>  >     }
>  >
>  >  To move the EH info around, or do we already take care of that or is
>  >  there some simple reason why it can't be around I've missed?
>  >
>  >  Honza
>
>  I don't think this is taken care of anywhere, and I can't convince myself
>  this won't happen either (I thought I had a good reason to believe
>  it won't happen, but I can't remember now :P ).
>  Here's the updated patch.
>  Bootstrapped on i686. Regtest still in progress.
>
>  Seongbae
>
>
>  2008-04-03  Seongbae Park  <seongbae.park@gmail.com>
>
>
>         * tree-ssa-propagate.c (set_rhs): Preserve the histogram
>         and the eh region information.
>
>
>         * value-prof.c (gimple_move_stmt_histograms): New function.
>         * value-prof.h (gimple_move_stmt_histograms): New function declaration.
>

Regtest found one regression.
It turns out we replace a call to __builtin_strcpy to __builtin_memcpy
but __builtin_strcpy is not marked with ECF_NOTHROW but memcpy is.
So the above code does the wrong thing - since it puts builtin_memcpy
which is ECF_NOTHROW, into eh region.

Of course, probably the right thing to do is mark builtin strcpy as no throw,
but that's not the point of this patch.
Attached is the updated patch. Restarted bootstrap & regtest.
The ChangeLog remains the same.

Seongbae

diff -r d366cc0ab0fe gcc/tree-ssa-propagate.c
--- a/gcc/tree-ssa-propagate.c	Tue Apr 01 15:29:24 2008 -0700
+++ b/gcc/tree-ssa-propagate.c	Thu Apr 03 16:29:16 2008 -0700
@@ -681,9 +681,10 @@ set_rhs (tree *stmt_p, tree expr)
 set_rhs (tree *stmt_p, tree expr)
 {
   tree stmt = *stmt_p, op;
-  stmt_ann_t ann;
+  tree new_stmt;
   tree var;
   ssa_op_iter iter;
+  int eh_region;
 
   if (!valid_gimple_expression_p (expr))
     return false;
@@ -734,10 +735,22 @@ set_rhs (tree *stmt_p, tree expr)
     default:
       /* 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);
-      *stmt_p = TREE_SIDE_EFFECTS (expr) ? expr : build_empty_stmt ();
-      (*stmt_p)->base.ann = (tree_ann_t) ann;
+      new_stmt = TREE_SIDE_EFFECTS (expr) ? expr : build_empty_stmt ();
+      *stmt_p = new_stmt;
+
+      /* Preserve the annotation, the histograms and the EH region information
+         associated with the original statement. The EH information
+	 needs to be preserved only if the new statement still can throw.  */
+      new_stmt->base.ann = (tree_ann_t) stmt_ann (stmt);
+      gimple_move_stmt_histograms (cfun, new_stmt, stmt);
+      if (tree_could_throw_p (new_stmt))
+	{
+	  eh_region = lookup_stmt_eh_region (stmt);
+	  /* We couldn't possibly turn a nothrow into a throw statement.  */
+	  gcc_assert (eh_region >= 0);
+	  remove_stmt_from_eh_region (stmt);
+	  add_stmt_to_eh_region (new_stmt, eh_region);
+	}
 
       if (gimple_in_ssa_p (cfun)
 	  && TREE_SIDE_EFFECTS (expr))
diff -r d366cc0ab0fe gcc/value-prof.c
--- a/gcc/value-prof.c	Tue Apr 01 15:29:24 2008 -0700
+++ b/gcc/value-prof.c	Thu Apr 03 16:29:16 2008 -0700
@@ -333,6 +333,25 @@ gimple_duplicate_stmt_histograms (struct
       new->hvalue.counters = xmalloc (sizeof (*new->hvalue.counters) * new->n_counters);
       memcpy (new->hvalue.counters, val->hvalue.counters, sizeof (*new->hvalue.counters) * new->n_counters);
       gimple_add_histogram_value (fun, stmt, new);
+    }
+}
+
+
+/* Move all histograms associated with OSTMT to STMT.  */
+
+void
+gimple_move_stmt_histograms (struct function *fun, tree stmt, tree ostmt)
+{
+  histogram_value val = gimple_histogram_value (fun, ostmt);
+  if (val)
+    {
+      /* The following three statements can't be reordered,
+         because histogram hashtab relies on stmt field value
+	 for finding the exact slot. */
+      set_histogram_value (fun, ostmt, NULL);
+      for (; val != NULL; val = val->hvalue.next)
+	val->hvalue.stmt = stmt;
+      set_histogram_value (fun, stmt, val);
     }
 }
 
diff -r d366cc0ab0fe gcc/value-prof.h
--- a/gcc/value-prof.h	Tue Apr 01 15:29:24 2008 -0700
+++ b/gcc/value-prof.h	Thu Apr 03 16:29:16 2008 -0700
@@ -116,6 +116,7 @@ void gimple_remove_histogram_value (stru
 void gimple_remove_histogram_value (struct function *, tree, histogram_value);
 void gimple_remove_stmt_histograms (struct function *, tree);
 void gimple_duplicate_stmt_histograms (struct function *, tree, struct function *, tree);
+void gimple_move_stmt_histograms (struct function *, tree, tree);
 void verify_histograms (void);
 void free_histograms (void);
 void stringop_block_profile (tree, unsigned int *, HOST_WIDE_INT *);