On Wed, 2017-12-20 at 20:24 +0100, Jakub Jelinek wrote:
> Hi!
>
> Prathamesh's r254140 moved the
> if (!flag_wpa)
> ipa_free_fn_summary ();
> from the ipa-inline pass to the following ipa-pure-const pass.
> That doesn't work well though, because ipa-inline pass has
> unconditional
> gate, so is done always when real IPA passes are performed, but
> ipa-pure-const is gated on some flag_* options, so if those options
> are disabled, we keep fn summary computed until end of compilation,
> and
> e.g. new function insertion hooks then attempt to update the
> fnsummary
> even when with RTL hooks registered.
>
> Calling ipa_free_fn_summary () in ipa-inline pass if the gate of
> the following pass will not be run looks too ugly to me, and while
> there
> is another unconditional ipa pass, moving it there doesn't look too
> clean to
> me either.
>
> We already have a separate pass ipa-free-fn-summary, just use it so
> far only
> during small IPA passes. I think the cleanest is to add another ipa
> pass
> that will just free it instead of sticking it into unrelated passes
> which
> just happen to be unconditional ATM.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Out of interest, did you test this with "jit" enabled? I mention it
because we ran into issues with ipa fn summary cleanup with jit before,
which required r254458 to fix (PR jit/82826).
Thanks
Dave
> 2017-12-20 Jakub Jelinek <ja...@redhat.com>
>
> PR ipa/83506
> * ipa-fnsummary.c (pass_data_ipa_free_fn_summary): Use 0 for
> todo_flags_finish.
> (pass_ipa_free_fn_summary): Add small_p private data member,
> initialize to false in the ctor.
> (pass_ipa_free_fn_summary::clone,
> pass_ipa_free_fn_summary::set_pass_param,
> pass_ipa_free_fn_summary::gate): New methods.
> (pass_ipa_free_fn_summary::execute): Return
> TODO_remove_functions
> | TODO_dump_symtab if small_p.
> * passes.def: Add true parm for the existing
> pass_ipa_free_fn_summary
> entry and add another instance of the pass with false parm
> after
> ipa-pure-const.
> * ipa-pure-const.c (pass_ipa_pure_const): Don't call
> ipa_free_fn_summary here.
>
> * gcc.dg/pr83506.c: New test.
> * gcc.dg/ipa/ctor-empty-1.c: Use -fdump-ipa-free-fnsummary1
> instead
> of -fdump-ipa-free-fnsummary and scan in free-fnsummary1
> instead of
> free-fnsummary dump.
>
> --- gcc/ipa-fnsummary.c.jj 2017-12-20 11:01:13.000000000 +0100
> +++ gcc/ipa-fnsummary.c 2017-12-20 11:43:40.462288141 +0100
> @@ -3538,26 +3538,36 @@ const pass_data pass_data_ipa_free_fn_su
> 0, /* properties_provided */
> 0, /* properties_destroyed */
> 0, /* todo_flags_start */
> - /* Early optimizations may make function unreachable. We can not
> - remove unreachable functions as part of the ealry opts pass
> because
> - TODOs are run before subpasses. Do it here. */
> - ( TODO_remove_functions | TODO_dump_symtab ), /* todo_flags_finish
> */
> + 0, /* todo_flags_finish */
> };
>
> class pass_ipa_free_fn_summary : public simple_ipa_opt_pass
> {
> public:
> pass_ipa_free_fn_summary (gcc::context *ctxt)
> - : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt)
> + : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt),
> + small_p (false)
> {}
>
> /* opt_pass methods: */
> + opt_pass *clone () { return new pass_ipa_free_fn_summary (m_ctxt);
> }
> + void set_pass_param (unsigned int n, bool param)
> + {
> + gcc_assert (n == 0);
> + small_p = param;
> + }
> + virtual bool gate (function *) { return small_p || !flag_wpa; }
> virtual unsigned int execute (function *)
> {
> ipa_free_fn_summary ();
> - return 0;
> + /* Early optimizations may make function unreachable. We can
> not
> + remove unreachable functions as part of the early opts pass
> because
> + TODOs are run before subpasses. Do it here. */
> + return small_p ? TODO_remove_functions | TODO_dump_symtab : 0;
> }
>
> +private:
> + bool small_p;
> }; // class pass_ipa_free_fn_summary
>
> } // anon namespace
> --- gcc/passes.def.jj 2017-12-18 14:57:20.000000000 +0100
> +++ gcc/passes.def 2017-12-20 11:31:32.402117369 +0100
> @@ -144,7 +144,7 @@ along with GCC; see the file COPYING3.
> PUSH_INSERT_PASSES_WITHIN (pass_ipa_tree_profile)
> NEXT_PASS (pass_feedback_split_functions);
> POP_INSERT_PASSES ()
> - NEXT_PASS (pass_ipa_free_fn_summary);
> + NEXT_PASS (pass_ipa_free_fn_summary, true /* small_p */);
> NEXT_PASS (pass_ipa_increase_alignment);
> NEXT_PASS (pass_ipa_tm);
> NEXT_PASS (pass_ipa_lower_emutls);
> @@ -161,6 +161,7 @@ along with GCC; see the file COPYING3.
> NEXT_PASS (pass_ipa_fn_summary);
> NEXT_PASS (pass_ipa_inline);
> NEXT_PASS (pass_ipa_pure_const);
> + NEXT_PASS (pass_ipa_free_fn_summary, false /* small_p */);
> NEXT_PASS (pass_ipa_reference);
> /* This pass needs to be scheduled after any IP code
> duplication. */
> NEXT_PASS (pass_ipa_single_use);
> --- gcc/ipa-pure-const.c.jj 2017-12-04 22:29:11.000000000
> +0100
> +++ gcc/ipa-pure-const.c 2017-12-20 11:33:11.905956408 +0100
> @@ -2013,10 +2013,6 @@ execute (function *)
> if (has_function_state (node))
> free (get_function_state (node));
> funct_state_vec.release ();
> -
> - /* In WPA we use inline summaries for partitioning process. */
> - if (!flag_wpa)
> - ipa_free_fn_summary ();
> return remove_p ? TODO_remove_functions : 0;
> }
>
> --- gcc/testsuite/gcc.dg/pr83506.c.jj 2017-12-20
> 11:39:48.405212526 +0100
> +++ gcc/testsuite/gcc.dg/pr83506.c 2017-12-20
> 11:39:18.000000000 +0100
> @@ -0,0 +1,14 @@
> +/* PR ipa/83506 */
> +/* { dg-do compile { target pthread } } */
> +/* { dg-options "-O1 -ftree-parallelize-loops=2 -fno-ipa-pure-const"
> } */
> +
> +unsigned int
> +foo (unsigned int x, int y)
> +{
> + while (y < 1)
> + {
> + x *= 3;
> + ++y;
> + }
> + return x;
> +}
> --- gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c.jj 2017-05-24
> 11:59:01.000000000 +0200
> +++ gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c 2017-12-20
> 20:15:27.144761517 +0100
> @@ -1,7 +1,7 @@
> /* { dg-do compile } */
> -/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary" } */
> +/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary1" } */
> static __attribute__((constructor))
> void empty_constructor()
> {
> }
> -/* { dg-final { scan-ipa-dump "Reclaiming functions:
> empty_constructor" "free-fnsummary" } } */
> +/* { dg-final { scan-ipa-dump "Reclaiming functions:
> empty_constructor" "free-fnsummary1" } } */
>
> Jakub
