Patchwork PR middle-end/53321: [4.8 Regression] LTO bootstrap failed with bootstrap-profiled

login
register
mail settings
Submitter H.J. Lu
Date July 31, 2012, 11:21 p.m.
Message ID <CAMe9rOomcm3rt-pTxZ_B_1NrQgp2kf4yfRL2A21mFpP-6ULDfA@mail.gmail.com>
Download mbox | patch
Permalink /patch/174351/
State New
Headers show

Comments

H.J. Lu - July 31, 2012, 11:21 p.m.
On Tue, Jul 31, 2012 at 5:39 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Hi,
>>
>> 2012-07-06  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>       PR middle-end/53321
>>       PR middle-end/53865
>>       * Makefile.in (tree-profile.o): Depend on ipa-inline.h.
>>
>>       * ipa.c (symtab_remove_unreachable_nodes): Restore
>>       cgraph_propagate_frequency call when something was changed.
>>
>>       * tree-profile.c: Include "ipa-inline.h".
>>       (gimple_gen_ic_func_profiler): Return bool.
>>       (tree_profiling): Call inline_free_summary to clear stale inline
>>       summary if gimple_gen_ic_func_profiler returns true.
>>
>>       * value-prof.h (gimple_gen_ic_func_profiler): Change return
>>       type to bool.
>
> Hi,
> My local tree is profedbootstrapping since I had local patch to
> prevent recomputation it before ipa_profile pass, but I agree it is better
> to not have stale inline summaries around.
>
>> +  /* Clear stale inline summary.  */
>> +  if (gen_ic_func_profiler)
>> +    inline_free_summary ();
>
> The inline summary is completely dead here. It is computed for purposes of
> early inlining and the real inliner will trash it and recompute soon.
> (because the one computer by real inliner is more detailed).
>
> matrix_reorg, emutls and tm passes will also suffer from the problem of
> invalidating the inling summary. It is better to free it early this.
>
> Can you simply add a micro pass just after pass_early_local_passes calliing
> inline_free_summary unconditionally?
>
> Path is preapproved (or I will make it tomorrow unless you beat me).
>

This patch works passed profiledbootstrap with LTO as well as LTO -O3
on 176.gcc in SPEC CPU 2000.  I have to add 2 inline_edge_summary_vec
checks to avoid ICE.  OK to install?

Thanks.
Jan Hubicka - Aug. 2, 2012, 10:23 a.m.
> 
> This patch works passed profiledbootstrap with LTO as well as LTO -O3
> on 176.gcc in SPEC CPU 2000.  I have to add 2 inline_edge_summary_vec
> checks to avoid ICE.  OK to install?

Thanks, it looks good. I am just concerned about...
> diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
> index 33cf7d2..7a8844f 100644
> --- a/gcc/ipa-split.c
> +++ b/gcc/ipa-split.c
> @@ -1415,7 +1415,7 @@ execute_split_functions (void)
>      }
>    /* This can be relaxed; function might become inlinable after splitting
>       away the uninlinable part.  */
> -  if (!inline_summary (node)->inlinable)
> +  if (inline_edge_summary_vec && !inline_summary (node)->inlinable)

.. this one. spliting is executed before free_inline_summary and thus should
not be affected. Or is it because of it gets called from process_new_functions
because some IPA pass adds a new function?
If so, I think we need to make sure that process_new_function do not make inline summary
allocated when it was previously free (and thus hunk would be OK).

Honza
H.J. Lu - Aug. 2, 2012, 1:35 p.m.
On Thu, Aug 2, 2012 at 3:23 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> This patch works passed profiledbootstrap with LTO as well as LTO -O3
>> on 176.gcc in SPEC CPU 2000.  I have to add 2 inline_edge_summary_vec
>> checks to avoid ICE.  OK to install?
>
> Thanks, it looks good. I am just concerned about...
>> diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
>> index 33cf7d2..7a8844f 100644
>> --- a/gcc/ipa-split.c
>> +++ b/gcc/ipa-split.c
>> @@ -1415,7 +1415,7 @@ execute_split_functions (void)
>>      }
>>    /* This can be relaxed; function might become inlinable after splitting
>>       away the uninlinable part.  */
>> -  if (!inline_summary (node)->inlinable)
>> +  if (inline_edge_summary_vec && !inline_summary (node)->inlinable)
>
> .. this one. spliting is executed before free_inline_summary and thus should
> not be affected. Or is it because of it gets called from process_new_functions
> because some IPA pass adds a new function?

It is called from pass_feedback_split_functions:

  NEXT_PASS (pass_all_early_optimizations);
    {
       ...
    }
  NEXT_PASS (pass_ipa_free_inline_summary);
  NEXT_PASS (pass_ipa_tree_profile);
    {
      struct opt_pass **p = &pass_ipa_tree_profile.pass.sub;
      NEXT_PASS (pass_feedback_split_functions);
    }

OK to install?

Thanks.
Jan Hubicka - Aug. 2, 2012, 4:40 p.m.
> On Thu, Aug 2, 2012 at 3:23 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >>
> >> This patch works passed profiledbootstrap with LTO as well as LTO -O3
> >> on 176.gcc in SPEC CPU 2000.  I have to add 2 inline_edge_summary_vec
> >> checks to avoid ICE.  OK to install?
> >
> > Thanks, it looks good. I am just concerned about...
> >> diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
> >> index 33cf7d2..7a8844f 100644
> >> --- a/gcc/ipa-split.c
> >> +++ b/gcc/ipa-split.c
> >> @@ -1415,7 +1415,7 @@ execute_split_functions (void)
> >>      }
> >>    /* This can be relaxed; function might become inlinable after splitting
> >>       away the uninlinable part.  */
> >> -  if (!inline_summary (node)->inlinable)
> >> +  if (inline_edge_summary_vec && !inline_summary (node)->inlinable)
> >
> > .. this one. spliting is executed before free_inline_summary and thus should
> > not be affected. Or is it because of it gets called from process_new_functions
> > because some IPA pass adds a new function?
> 
> It is called from pass_feedback_split_functions:
> 
>   NEXT_PASS (pass_all_early_optimizations);
>     {
>        ...
>     }
>   NEXT_PASS (pass_ipa_free_inline_summary);
>   NEXT_PASS (pass_ipa_tree_profile);
>     {
>       struct opt_pass **p = &pass_ipa_tree_profile.pass.sub;
>       NEXT_PASS (pass_feedback_split_functions);
>     }
> 
> OK to install?

Hmm, this is an ordering issue, but minnor one.  OK.
As comment says, this should be relaxed anyway and in fact I have path for that
in queue.

Honza
> 
> Thanks.
> 
> -- 
> H.J.

Patch

diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
index ae32131..970be1e 100644
--- a/gcc/ipa-inline-analysis.c
+++ b/gcc/ipa-inline-analysis.c
@@ -3242,6 +3242,8 @@  void
 inline_free_summary (void)
 {
   struct cgraph_node *node;
+  if (inline_edge_summary_vec == NULL)
+    return;
   FOR_EACH_DEFINED_FUNCTION (node)
     reset_inline_summary (node);
   if (function_insertion_hook_holder)
diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c
index 33cf7d2..7a8844f 100644
--- a/gcc/ipa-split.c
+++ b/gcc/ipa-split.c
@@ -1415,7 +1415,7 @@  execute_split_functions (void)
     }
   /* This can be relaxed; function might become inlinable after splitting
      away the uninlinable part.  */
-  if (!inline_summary (node)->inlinable)
+  if (inline_edge_summary_vec && !inline_summary (node)->inlinable)
     {
       if (dump_file)
 	fprintf (dump_file, "Not splitting: not inlinable.\n");
diff --git a/gcc/ipa.c b/gcc/ipa.c
index 9329d9b..e270591 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -448,6 +448,11 @@  symtab_remove_unreachable_nodes (bool
before_inlining_p, FILE *file)
   verify_symtab ();
 #endif

+  /* If we removed something, perhaps profile could be improved.  */
+  if (changed && optimize && inline_edge_summary_vec)
+    FOR_EACH_DEFINED_FUNCTION (node)
+      cgraph_propagate_frequency (node);
+
   return changed;
 }

@@ -960,6 +965,34 @@  struct simple_ipa_opt_pass
pass_ipa_function_and_variable_visibility =
  }
 };

+/* Free inline summary.  */
+
+static unsigned
+free_inline_summary (void)
+{
+  inline_free_summary ();
+  return 0;
+}
+
+struct simple_ipa_opt_pass pass_ipa_free_inline_summary =
+{
+ {
+  SIMPLE_IPA_PASS,
+  "*free_inline_summary",		/* name */
+  NULL,					/* gate */
+  free_inline_summary,			/* execute */
+  NULL,					/* sub */
+  NULL,					/* next */
+  0,					/* static_pass_number */
+  TV_IPA_FREE_INLINE_SUMMARY,		/* tv_id */
+  0,	                                /* properties_required */
+  0,					/* properties_provided */
+  0,					/* properties_destroyed */
+  0,					/* todo_flags_start */
+  TODO_ggc_collect			/* todo_flags_finish */
+ }
+};
+
 /* Do not re-run on ltrans stage.  */

 static bool
diff --git a/gcc/passes.c b/gcc/passes.c
index 7709671..fc4dbc5 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1324,6 +1324,7 @@  init_optimization_passes (void)
       NEXT_PASS (pass_rebuild_cgraph_edges);
       NEXT_PASS (pass_inline_parameters);
     }
+  NEXT_PASS (pass_ipa_free_inline_summary);
   NEXT_PASS (pass_ipa_tree_profile);
     {
       struct opt_pass **p = &pass_ipa_tree_profile.pass.sub;
diff --git a/gcc/timevar.def b/gcc/timevar.def
index 18d419d..8fa1a2b 100644
--- a/gcc/timevar.def
+++ b/gcc/timevar.def
@@ -89,6 +89,7 @@  DEFTIMEVAR (TV_IPA_PURE_CONST        , "ipa pure const")
 DEFTIMEVAR (TV_IPA_PTA               , "ipa points-to")
 DEFTIMEVAR (TV_IPA_SRA               , "ipa SRA")
 DEFTIMEVAR (TV_IPA_FREE_LANG_DATA    , "ipa free lang data")
+DEFTIMEVAR (TV_IPA_FREE_INLINE_SUMMARY, "ipa free inline summary")
 /* Time spent by constructing CFG.  */
 DEFTIMEVAR (TV_CFG                   , "cfg construction")
 /* Time spent by cleaning up CFG.  */
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 081e48c..5037c29 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -377,6 +377,7 @@  extern struct simple_ipa_opt_pass
pass_ipa_increase_alignment;
 extern struct simple_ipa_opt_pass pass_ipa_matrix_reorg;
 extern struct ipa_opt_pass_d pass_ipa_inline;
 extern struct simple_ipa_opt_pass pass_ipa_free_lang_data;
+extern struct simple_ipa_opt_pass pass_ipa_free_inline_summary;
 extern struct ipa_opt_pass_d pass_ipa_cp;
 extern struct ipa_opt_pass_d pass_ipa_reference;