| Message ID | 20171220192404.GL2353@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Fix ICE with -ftree-parallelize-loops=2 -fno-ipa-pure-const (PR ipa/83506) | expand |
On December 20, 2017 8:24:04 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> 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? OK. Richard. >2017-12-20 Jakub Jelinek <jakub@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
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 <jakub@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
On Wed, Dec 20, 2017 at 02:38:13PM -0500, David Malcolm wrote: > > 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). No, I'm testing jit only when building distro rpms, not in normal bootstraps (because it requires --enable-host-shared). Jakub
--- 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" } } */
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? 2017-12-20 Jakub Jelinek <jakub@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. Jakub