| Message ID | 20131111161345.GA17167@tsaunders-iceball.corp.tor1.mozilla.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 11, 2013 at 5:13 PM, Trevor Saunders <tsaunders@mozilla.com> wrote: > On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote: >> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote: >> > On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote: >> >> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote: >> >> > From: Trevor Saunders <tsaunders@mozilla.com> >> >> > >> >> > Hi, >> >> > >> >> > This is the result of seeing what it would take to get rid of the has_gate and >> >> > has_execute flags on pass_data. It turns out not much, but I wanted >> >> > confirmation this part is ok before I go deal with all the places that >> >> > initialize the fields. >> >> > >> >> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite >> >> > regressions (ignoring the silk stuff because the full paths in its test names >> >> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok? >> >> >> >> The has_gate flag is easy to remove (without a TODO_ hack), right? >> >> No gate simply means that the gate returns always true. The only >> >> weird thing is >> >> >> >> /* If we have a gate, combine the properties that we could have with >> >> and without the pass being examined. */ >> >> if (pass->has_gate) >> >> properties &= new_properties; >> >> else >> >> properties = new_properties; >> >> >> >> which I don't understand (and you just removed all properties handling there). >> >> >> >> So can you split out removing has_gate? This part is obviously ok. >> >> >> >> Then, for ->execute () I'd have refactored the code to make >> >> ->sub passes explicitely executed by the default ->execute () >> >> implementation only. That is, passes without overriding execute >> >> are containers only. Can you quickly check whether that would >> >> work out? >> > >> > Ok, I've now given this a shot and wasn't terribly successful, if I just >> > change execute_pass_list and execute_ipa_pass_list to handle container >> > passes executing their sub passes I get the following ICE >> > >> > ./gt-passes.h:77:2: internal compiler error: Segmentation fault >> > }; >> > ^ >> > 0xd43d96 crash_signal >> > /src/gcc/gcc/toplev.c:334 >> > 0xd901a9 ssa_default_def(function*, tree_node*) >> > /src/gcc/gcc/tree-dfa.c:318 >> > 0xb56d77 ipa_analyze_params_uses >> > /src/gcc/gcc/ipa-prop.c:2094 >> > 0xb57275 ipa_analyze_node(cgraph_node*) >> > /src/gcc/gcc/ipa-prop.c:2179 >> > 0x13e2b6d ipcp_generate_summary >> > /src/gcc/gcc/ipa-cp.c:3615 >> > 0xc55a2a >> > execute_ipa_summary_passes(ipa_opt_pass_d*) >> > /src/gcc/gcc/passes.c:1991 >> > 0x943341 ipa_passes >> > /src/gcc/gcc/cgraphunit.c:2011 >> > 0x943675 >> > compile() >> > /src/gcc/gcc/cgraphunit.c:2118 >> > >> > now >> > Which is because fn->gimple_df is null. I expect that's because pass >> > ordering changed somehow, but I'm not sure off hand how or ifthat's >> > something worth figuring out right now. >> >> Yeah, some of the walking doesn't seem to work ... probably a >> pass with sub-passes already has an execute member function ;) > > Sounds like a reasonable guess. > >> > Another issue I realized is that doing this will change the order of >> > plugin events from >> > start container pass a >> > end container pass a >> > start contained pass b >> > end contained pass b >> > ... >> > >> > to >> > >> > start container pass a >> > start contained pass b >> > end contained pass b >> > end container pass a >> > >> > Arguably that's an improvement, but I'm not sure if changing that APi >> > like that is acceptable. >> >> Indeed an improvement. Can you attach your patch? > > ok, done It's pass_early_local_passes which has sub-passes and adjusts cgraph state in its execute. The following works for me (not really tested of course). Richard. > thanks > > Trev > >> >> Thanks, >> Richard. >> >> > Trev >> > >> >> >> >> Thanks, >> >> Richard. >> >> >> >> > Trev >> >> > >> >> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com> >> >> > >> >> > * pass_manager.h (pass_manager): Adjust. >> >> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need >> >> > to do anything for this pass. >> >> > (pass_manager::register_dump_files_1): Don't uselessly deal with >> >> > properties of passes. >> >> > (pass_manager::register_dump_files): Adjust. >> >> > (dump_one_pass): Just call pass->gate (). >> >> > (execute_ipa_summary_passes): Likewise. >> >> > (execute_one_pass): Don't check pass->has_execute flag. >> >> > (ipa_write_summaries_2): Don't check pass->has_gate flag. >> >> > (ipa_write_optimization_summaries_1): Likewise. >> >> > (ipa_read_summaries_1): Likewise. >> >> > (ipa_read_optimization_summaries_1): Likewise. >> >> > (execute_ipa_stmt_fixups): Likewise. >> >> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and >> >> > has_execute to useless_has_execute to be sure they're unused. >> >> > (TODO_absolutely_nothing): New constant. >> >> > >> >> > >> >> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h >> >> > index 77d78eb..3bc0a99 100644 >> >> > --- a/gcc/pass_manager.h >> >> > +++ b/gcc/pass_manager.h >> >> > @@ -93,7 +93,7 @@ public: >> >> > >> >> > private: >> >> > void set_pass_for_id (int id, opt_pass *pass); >> >> > - int register_dump_files_1 (struct opt_pass *pass, int properties); >> >> > + void register_dump_files_1 (struct opt_pass *pass); >> >> > void register_dump_files (struct opt_pass *pass, int properties); >> >> > >> >> > private: >> >> > diff --git a/gcc/passes.c b/gcc/passes.c >> >> > index 19e5869..3b28dc9 100644 >> >> > --- a/gcc/passes.c >> >> > +++ b/gcc/passes.c >> >> > @@ -112,7 +112,7 @@ opt_pass::gate () >> >> > unsigned int >> >> > opt_pass::execute () >> >> > { >> >> > - return 0; >> >> > + return TODO_absolutely_nothing; >> >> > } >> >> > >> >> > opt_pass::opt_pass (const pass_data &data, context *ctxt) >> >> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass) >> >> > >> >> > /* Recursive worker function for register_dump_files. */ >> >> > >> >> > -int >> >> > +void >> >> > pass_manager:: >> >> > -register_dump_files_1 (struct opt_pass *pass, int properties) >> >> > +register_dump_files_1 (struct opt_pass *pass) >> >> > { >> >> > do >> >> > { >> >> > - int new_properties = (properties | pass->properties_provided) >> >> > - & ~pass->properties_destroyed; >> >> > - >> >> > if (pass->name && pass->name[0] != '*') >> >> > register_one_dump_file (pass); >> >> > >> >> > if (pass->sub) >> >> > - new_properties = register_dump_files_1 (pass->sub, new_properties); >> >> > - >> >> > - /* If we have a gate, combine the properties that we could have with >> >> > - and without the pass being examined. */ >> >> > - if (pass->has_gate) >> >> > - properties &= new_properties; >> >> > - else >> >> > - properties = new_properties; >> >> > + register_dump_files_1 (pass->sub); >> >> > >> >> > pass = pass->next; >> >> > } >> >> > while (pass); >> >> > - >> >> > - return properties; >> >> > } >> >> > >> >> > /* Register the dump files for the pass_manager starting at PASS. >> >> > @@ -739,7 +727,7 @@ pass_manager:: >> >> > register_dump_files (struct opt_pass *pass,int properties) >> >> > { >> >> > pass->properties_required |= properties; >> >> > - register_dump_files_1 (pass, properties); >> >> > + register_dump_files_1 (pass); >> >> > } >> >> > >> >> > struct pass_registry >> >> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent) >> >> > const char *pn; >> >> > bool is_on, is_really_on; >> >> > >> >> > - is_on = pass->has_gate ? pass->gate () : true; >> >> > + is_on = pass->gate (); >> >> > is_really_on = override_gate_status (pass, current_function_decl, is_on); >> >> > >> >> > if (pass->static_pass_number <= 0) >> >> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass) >> >> > >> >> > /* Execute all of the IPA_PASSes in the list. */ >> >> > if (ipa_pass->type == IPA_PASS >> >> > - && ((!pass->has_gate) || pass->gate ()) >> >> > + && pass->gate () >> >> > && ipa_pass->generate_summary) >> >> > { >> >> > pass_init_dump_file (pass); >> >> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass) >> >> > >> >> > /* Check whether gate check should be avoided. >> >> > User controls the value of the gate through the parameter "gate_status". */ >> >> > - gate_status = pass->has_gate ? pass->gate () : true; >> >> > + gate_status = pass->gate (); >> >> > gate_status = override_gate_status (pass, current_function_decl, gate_status); >> >> > >> >> > /* Override gate with plugin. */ >> >> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass) >> >> > timevar_push (pass->tv_id); >> >> > >> >> > /* Do it! */ >> >> > - if (pass->has_execute) >> >> > - { >> >> > - todo_after = pass->execute (); >> >> > - do_per_function (clear_last_verified, NULL); >> >> > - } >> >> > + todo_after = pass->execute (); >> >> > + if (todo_after != TODO_absolutely_nothing) >> >> > + do_per_function (clear_last_verified, NULL); >> >> > + else >> >> > + todo_after &= ~TODO_absolutely_nothing; >> >> > >> >> > /* Stop timevar. */ >> >> > if (pass->tv_id != TV_NONE) >> >> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state) >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> > if (pass->type == IPA_PASS >> >> > && ipa_pass->write_summary >> >> > - && ((!pass->has_gate) || pass->gate ())) >> >> > + && pass->gate ()) >> >> > { >> >> > /* If a timevar is present, start it. */ >> >> > if (pass->tv_id) >> >> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> > if (pass->type == IPA_PASS >> >> > && ipa_pass->write_optimization_summary >> >> > - && ((!pass->has_gate) || pass->gate ())) >> >> > + && pass->gate ()) >> >> > { >> >> > /* If a timevar is present, start it. */ >> >> > if (pass->tv_id) >> >> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass) >> >> > gcc_assert (!cfun); >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> > >> >> > - if ((!pass->has_gate) || pass->gate ()) >> >> > + if (pass->gate ()) >> >> > { >> >> > if (pass->type == IPA_PASS && ipa_pass->read_summary) >> >> > { >> >> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass) >> >> > gcc_assert (!cfun); >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> > >> >> > - if ((!pass->has_gate) || pass->gate ()) >> >> > + if (pass->gate ()) >> >> > { >> >> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary) >> >> > { >> >> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass, >> >> > { >> >> > /* Execute all of the IPA_PASSes in the list. */ >> >> > if (pass->type == IPA_PASS >> >> > - && ((!pass->has_gate) || pass->gate ())) >> >> > + && pass->gate ()) >> >> > { >> >> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass; >> >> > >> >> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h >> >> > index 9efee1e..bed639e 100644 >> >> > --- a/gcc/tree-pass.h >> >> > +++ b/gcc/tree-pass.h >> >> > @@ -49,11 +49,11 @@ struct pass_data >> >> > >> >> > /* If true, this pass has its own implementation of the opt_pass::gate >> >> > method. */ >> >> > - bool has_gate; >> >> > + bool useless_has_gate; >> >> > >> >> > /* If true, this pass has its own implementation of the opt_pass::execute >> >> > method. */ >> >> > - bool has_execute; >> >> > + bool useless_has_execute; >> >> > >> >> > /* The timevar id associated with this pass. */ >> >> > /* ??? Ideally would be dynamically assigned. */ >> >> > @@ -299,6 +299,10 @@ protected: >> >> > /* Rebuild the callgraph edges. */ >> >> > #define TODO_rebuild_cgraph_edges (1 << 22) >> >> > >> >> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass >> >> > + did absolutely nothing. */ >> >> > +#define TODO_absolutely_nothing 1 << 23 >> >> > + >> >> > /* Internally used in execute_function_todo(). */ >> >> > #define TODO_update_ssa_any \ >> >> > (TODO_update_ssa \ >> >> > -- >> >> > 1.8.4.2 >> >> >
On Fri, Nov 15, 2013 at 12:54:05PM +0100, Richard Biener wrote: > On Mon, Nov 11, 2013 at 5:13 PM, Trevor Saunders <tsaunders@mozilla.com> wrote: > > On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote: > >> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote: > >> > On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote: > >> >> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote: > >> >> > From: Trevor Saunders <tsaunders@mozilla.com> > >> >> > > >> >> > Hi, > >> >> > > >> >> > This is the result of seeing what it would take to get rid of the has_gate and > >> >> > has_execute flags on pass_data. It turns out not much, but I wanted > >> >> > confirmation this part is ok before I go deal with all the places that > >> >> > initialize the fields. > >> >> > > >> >> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite > >> >> > regressions (ignoring the silk stuff because the full paths in its test names > >> >> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok? > >> >> > >> >> The has_gate flag is easy to remove (without a TODO_ hack), right? > >> >> No gate simply means that the gate returns always true. The only > >> >> weird thing is > >> >> > >> >> /* If we have a gate, combine the properties that we could have with > >> >> and without the pass being examined. */ > >> >> if (pass->has_gate) > >> >> properties &= new_properties; > >> >> else > >> >> properties = new_properties; > >> >> > >> >> which I don't understand (and you just removed all properties handling there). > >> >> > >> >> So can you split out removing has_gate? This part is obviously ok. > >> >> > >> >> Then, for ->execute () I'd have refactored the code to make > >> >> ->sub passes explicitely executed by the default ->execute () > >> >> implementation only. That is, passes without overriding execute > >> >> are containers only. Can you quickly check whether that would > >> >> work out? > >> > > >> > Ok, I've now given this a shot and wasn't terribly successful, if I just > >> > change execute_pass_list and execute_ipa_pass_list to handle container > >> > passes executing their sub passes I get the following ICE > >> > > >> > ./gt-passes.h:77:2: internal compiler error: Segmentation fault > >> > }; > >> > ^ > >> > 0xd43d96 crash_signal > >> > /src/gcc/gcc/toplev.c:334 > >> > 0xd901a9 ssa_default_def(function*, tree_node*) > >> > /src/gcc/gcc/tree-dfa.c:318 > >> > 0xb56d77 ipa_analyze_params_uses > >> > /src/gcc/gcc/ipa-prop.c:2094 > >> > 0xb57275 ipa_analyze_node(cgraph_node*) > >> > /src/gcc/gcc/ipa-prop.c:2179 > >> > 0x13e2b6d ipcp_generate_summary > >> > /src/gcc/gcc/ipa-cp.c:3615 > >> > 0xc55a2a > >> > execute_ipa_summary_passes(ipa_opt_pass_d*) > >> > /src/gcc/gcc/passes.c:1991 > >> > 0x943341 ipa_passes > >> > /src/gcc/gcc/cgraphunit.c:2011 > >> > 0x943675 > >> > compile() > >> > /src/gcc/gcc/cgraphunit.c:2118 > >> > > >> > now > >> > Which is because fn->gimple_df is null. I expect that's because pass > >> > ordering changed somehow, but I'm not sure off hand how or ifthat's > >> > something worth figuring out right now. > >> > >> Yeah, some of the walking doesn't seem to work ... probably a > >> pass with sub-passes already has an execute member function ;) > > > > Sounds like a reasonable guess. > > > >> > Another issue I realized is that doing this will change the order of > >> > plugin events from > >> > start container pass a > >> > end container pass a > >> > start contained pass b > >> > end contained pass b > >> > ... > >> > > >> > to > >> > > >> > start container pass a > >> > start contained pass b > >> > end contained pass b > >> > end container pass a > >> > > >> > Arguably that's an improvement, but I'm not sure if changing that APi > >> > like that is acceptable. > >> > >> Indeed an improvement. Can you attach your patch? > > > > ok, done > > It's pass_early_local_passes which has sub-passes and adjusts > cgraph state in its execute. > > The following works for me (not really tested of course). ok, thanks a lot for looking, I'll test with the removal of the flags and repost the full patch. Trev > > Richard. > > > thanks > > > > Trev > > > >> > >> Thanks, > >> Richard. > >> > >> > Trev > >> > > >> >> > >> >> Thanks, > >> >> Richard. > >> >> > >> >> > Trev > >> >> > > >> >> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com> > >> >> > > >> >> > * pass_manager.h (pass_manager): Adjust. > >> >> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need > >> >> > to do anything for this pass. > >> >> > (pass_manager::register_dump_files_1): Don't uselessly deal with > >> >> > properties of passes. > >> >> > (pass_manager::register_dump_files): Adjust. > >> >> > (dump_one_pass): Just call pass->gate (). > >> >> > (execute_ipa_summary_passes): Likewise. > >> >> > (execute_one_pass): Don't check pass->has_execute flag. > >> >> > (ipa_write_summaries_2): Don't check pass->has_gate flag. > >> >> > (ipa_write_optimization_summaries_1): Likewise. > >> >> > (ipa_read_summaries_1): Likewise. > >> >> > (ipa_read_optimization_summaries_1): Likewise. > >> >> > (execute_ipa_stmt_fixups): Likewise. > >> >> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and > >> >> > has_execute to useless_has_execute to be sure they're unused. > >> >> > (TODO_absolutely_nothing): New constant. > >> >> > > >> >> > > >> >> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h > >> >> > index 77d78eb..3bc0a99 100644 > >> >> > --- a/gcc/pass_manager.h > >> >> > +++ b/gcc/pass_manager.h > >> >> > @@ -93,7 +93,7 @@ public: > >> >> > > >> >> > private: > >> >> > void set_pass_for_id (int id, opt_pass *pass); > >> >> > - int register_dump_files_1 (struct opt_pass *pass, int properties); > >> >> > + void register_dump_files_1 (struct opt_pass *pass); > >> >> > void register_dump_files (struct opt_pass *pass, int properties); > >> >> > > >> >> > private: > >> >> > diff --git a/gcc/passes.c b/gcc/passes.c > >> >> > index 19e5869..3b28dc9 100644 > >> >> > --- a/gcc/passes.c > >> >> > +++ b/gcc/passes.c > >> >> > @@ -112,7 +112,7 @@ opt_pass::gate () > >> >> > unsigned int > >> >> > opt_pass::execute () > >> >> > { > >> >> > - return 0; > >> >> > + return TODO_absolutely_nothing; > >> >> > } > >> >> > > >> >> > opt_pass::opt_pass (const pass_data &data, context *ctxt) > >> >> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass) > >> >> > > >> >> > /* Recursive worker function for register_dump_files. */ > >> >> > > >> >> > -int > >> >> > +void > >> >> > pass_manager:: > >> >> > -register_dump_files_1 (struct opt_pass *pass, int properties) > >> >> > +register_dump_files_1 (struct opt_pass *pass) > >> >> > { > >> >> > do > >> >> > { > >> >> > - int new_properties = (properties | pass->properties_provided) > >> >> > - & ~pass->properties_destroyed; > >> >> > - > >> >> > if (pass->name && pass->name[0] != '*') > >> >> > register_one_dump_file (pass); > >> >> > > >> >> > if (pass->sub) > >> >> > - new_properties = register_dump_files_1 (pass->sub, new_properties); > >> >> > - > >> >> > - /* If we have a gate, combine the properties that we could have with > >> >> > - and without the pass being examined. */ > >> >> > - if (pass->has_gate) > >> >> > - properties &= new_properties; > >> >> > - else > >> >> > - properties = new_properties; > >> >> > + register_dump_files_1 (pass->sub); > >> >> > > >> >> > pass = pass->next; > >> >> > } > >> >> > while (pass); > >> >> > - > >> >> > - return properties; > >> >> > } > >> >> > > >> >> > /* Register the dump files for the pass_manager starting at PASS. > >> >> > @@ -739,7 +727,7 @@ pass_manager:: > >> >> > register_dump_files (struct opt_pass *pass,int properties) > >> >> > { > >> >> > pass->properties_required |= properties; > >> >> > - register_dump_files_1 (pass, properties); > >> >> > + register_dump_files_1 (pass); > >> >> > } > >> >> > > >> >> > struct pass_registry > >> >> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent) > >> >> > const char *pn; > >> >> > bool is_on, is_really_on; > >> >> > > >> >> > - is_on = pass->has_gate ? pass->gate () : true; > >> >> > + is_on = pass->gate (); > >> >> > is_really_on = override_gate_status (pass, current_function_decl, is_on); > >> >> > > >> >> > if (pass->static_pass_number <= 0) > >> >> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass) > >> >> > > >> >> > /* Execute all of the IPA_PASSes in the list. */ > >> >> > if (ipa_pass->type == IPA_PASS > >> >> > - && ((!pass->has_gate) || pass->gate ()) > >> >> > + && pass->gate () > >> >> > && ipa_pass->generate_summary) > >> >> > { > >> >> > pass_init_dump_file (pass); > >> >> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass) > >> >> > > >> >> > /* Check whether gate check should be avoided. > >> >> > User controls the value of the gate through the parameter "gate_status". */ > >> >> > - gate_status = pass->has_gate ? pass->gate () : true; > >> >> > + gate_status = pass->gate (); > >> >> > gate_status = override_gate_status (pass, current_function_decl, gate_status); > >> >> > > >> >> > /* Override gate with plugin. */ > >> >> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass) > >> >> > timevar_push (pass->tv_id); > >> >> > > >> >> > /* Do it! */ > >> >> > - if (pass->has_execute) > >> >> > - { > >> >> > - todo_after = pass->execute (); > >> >> > - do_per_function (clear_last_verified, NULL); > >> >> > - } > >> >> > + todo_after = pass->execute (); > >> >> > + if (todo_after != TODO_absolutely_nothing) > >> >> > + do_per_function (clear_last_verified, NULL); > >> >> > + else > >> >> > + todo_after &= ~TODO_absolutely_nothing; > >> >> > > >> >> > /* Stop timevar. */ > >> >> > if (pass->tv_id != TV_NONE) > >> >> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state) > >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); > >> >> > if (pass->type == IPA_PASS > >> >> > && ipa_pass->write_summary > >> >> > - && ((!pass->has_gate) || pass->gate ())) > >> >> > + && pass->gate ()) > >> >> > { > >> >> > /* If a timevar is present, start it. */ > >> >> > if (pass->tv_id) > >> >> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s > >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); > >> >> > if (pass->type == IPA_PASS > >> >> > && ipa_pass->write_optimization_summary > >> >> > - && ((!pass->has_gate) || pass->gate ())) > >> >> > + && pass->gate ()) > >> >> > { > >> >> > /* If a timevar is present, start it. */ > >> >> > if (pass->tv_id) > >> >> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass) > >> >> > gcc_assert (!cfun); > >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); > >> >> > > >> >> > - if ((!pass->has_gate) || pass->gate ()) > >> >> > + if (pass->gate ()) > >> >> > { > >> >> > if (pass->type == IPA_PASS && ipa_pass->read_summary) > >> >> > { > >> >> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass) > >> >> > gcc_assert (!cfun); > >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); > >> >> > > >> >> > - if ((!pass->has_gate) || pass->gate ()) > >> >> > + if (pass->gate ()) > >> >> > { > >> >> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary) > >> >> > { > >> >> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass, > >> >> > { > >> >> > /* Execute all of the IPA_PASSes in the list. */ > >> >> > if (pass->type == IPA_PASS > >> >> > - && ((!pass->has_gate) || pass->gate ())) > >> >> > + && pass->gate ()) > >> >> > { > >> >> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass; > >> >> > > >> >> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h > >> >> > index 9efee1e..bed639e 100644 > >> >> > --- a/gcc/tree-pass.h > >> >> > +++ b/gcc/tree-pass.h > >> >> > @@ -49,11 +49,11 @@ struct pass_data > >> >> > > >> >> > /* If true, this pass has its own implementation of the opt_pass::gate > >> >> > method. */ > >> >> > - bool has_gate; > >> >> > + bool useless_has_gate; > >> >> > > >> >> > /* If true, this pass has its own implementation of the opt_pass::execute > >> >> > method. */ > >> >> > - bool has_execute; > >> >> > + bool useless_has_execute; > >> >> > > >> >> > /* The timevar id associated with this pass. */ > >> >> > /* ??? Ideally would be dynamically assigned. */ > >> >> > @@ -299,6 +299,10 @@ protected: > >> >> > /* Rebuild the callgraph edges. */ > >> >> > #define TODO_rebuild_cgraph_edges (1 << 22) > >> >> > > >> >> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass > >> >> > + did absolutely nothing. */ > >> >> > +#define TODO_absolutely_nothing 1 << 23 > >> >> > + > >> >> > /* Internally used in execute_function_todo(). */ > >> >> > #define TODO_update_ssa_any \ > >> >> > (TODO_update_ssa \ > >> >> > -- > >> >> > 1.8.4.2 > >> >> >
On Fri, Nov 15, 2013 at 1:11 PM, Trevor Saunders <tsaunders@mozilla.com> wrote: > On Fri, Nov 15, 2013 at 12:54:05PM +0100, Richard Biener wrote: >> On Mon, Nov 11, 2013 at 5:13 PM, Trevor Saunders <tsaunders@mozilla.com> wrote: >> > On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote: >> >> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote: >> >> > On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote: >> >> >> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote: >> >> >> > From: Trevor Saunders <tsaunders@mozilla.com> >> >> >> > >> >> >> > Hi, >> >> >> > >> >> >> > This is the result of seeing what it would take to get rid of the has_gate and >> >> >> > has_execute flags on pass_data. It turns out not much, but I wanted >> >> >> > confirmation this part is ok before I go deal with all the places that >> >> >> > initialize the fields. >> >> >> > >> >> >> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite >> >> >> > regressions (ignoring the silk stuff because the full paths in its test names >> >> >> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok? >> >> >> >> >> >> The has_gate flag is easy to remove (without a TODO_ hack), right? >> >> >> No gate simply means that the gate returns always true. The only >> >> >> weird thing is >> >> >> >> >> >> /* If we have a gate, combine the properties that we could have with >> >> >> and without the pass being examined. */ >> >> >> if (pass->has_gate) >> >> >> properties &= new_properties; >> >> >> else >> >> >> properties = new_properties; >> >> >> >> >> >> which I don't understand (and you just removed all properties handling there). >> >> >> >> >> >> So can you split out removing has_gate? This part is obviously ok. >> >> >> >> >> >> Then, for ->execute () I'd have refactored the code to make >> >> >> ->sub passes explicitely executed by the default ->execute () >> >> >> implementation only. That is, passes without overriding execute >> >> >> are containers only. Can you quickly check whether that would >> >> >> work out? >> >> > >> >> > Ok, I've now given this a shot and wasn't terribly successful, if I just >> >> > change execute_pass_list and execute_ipa_pass_list to handle container >> >> > passes executing their sub passes I get the following ICE >> >> > >> >> > ./gt-passes.h:77:2: internal compiler error: Segmentation fault >> >> > }; >> >> > ^ >> >> > 0xd43d96 crash_signal >> >> > /src/gcc/gcc/toplev.c:334 >> >> > 0xd901a9 ssa_default_def(function*, tree_node*) >> >> > /src/gcc/gcc/tree-dfa.c:318 >> >> > 0xb56d77 ipa_analyze_params_uses >> >> > /src/gcc/gcc/ipa-prop.c:2094 >> >> > 0xb57275 ipa_analyze_node(cgraph_node*) >> >> > /src/gcc/gcc/ipa-prop.c:2179 >> >> > 0x13e2b6d ipcp_generate_summary >> >> > /src/gcc/gcc/ipa-cp.c:3615 >> >> > 0xc55a2a >> >> > execute_ipa_summary_passes(ipa_opt_pass_d*) >> >> > /src/gcc/gcc/passes.c:1991 >> >> > 0x943341 ipa_passes >> >> > /src/gcc/gcc/cgraphunit.c:2011 >> >> > 0x943675 >> >> > compile() >> >> > /src/gcc/gcc/cgraphunit.c:2118 >> >> > >> >> > now >> >> > Which is because fn->gimple_df is null. I expect that's because pass >> >> > ordering changed somehow, but I'm not sure off hand how or ifthat's >> >> > something worth figuring out right now. >> >> >> >> Yeah, some of the walking doesn't seem to work ... probably a >> >> pass with sub-passes already has an execute member function ;) >> > >> > Sounds like a reasonable guess. >> > >> >> > Another issue I realized is that doing this will change the order of >> >> > plugin events from >> >> > start container pass a >> >> > end container pass a >> >> > start contained pass b >> >> > end contained pass b >> >> > ... >> >> > >> >> > to >> >> > >> >> > start container pass a >> >> > start contained pass b >> >> > end contained pass b >> >> > end container pass a >> >> > >> >> > Arguably that's an improvement, but I'm not sure if changing that APi >> >> > like that is acceptable. >> >> >> >> Indeed an improvement. Can you attach your patch? >> > >> > ok, done >> >> It's pass_early_local_passes which has sub-passes and adjusts >> cgraph state in its execute. >> >> The following works for me (not really tested of course). > > ok, thanks a lot for looking, I'll test with the removal of the flags > and repost the full patch. There seem to be similar issues elsewhere (from just running tree-ssa.exp). Richard. > Trev > >> >> Richard. >> >> > thanks >> > >> > Trev >> > >> >> >> >> Thanks, >> >> Richard. >> >> >> >> > Trev >> >> > >> >> >> >> >> >> Thanks, >> >> >> Richard. >> >> >> >> >> >> > Trev >> >> >> > >> >> >> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com> >> >> >> > >> >> >> > * pass_manager.h (pass_manager): Adjust. >> >> >> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need >> >> >> > to do anything for this pass. >> >> >> > (pass_manager::register_dump_files_1): Don't uselessly deal with >> >> >> > properties of passes. >> >> >> > (pass_manager::register_dump_files): Adjust. >> >> >> > (dump_one_pass): Just call pass->gate (). >> >> >> > (execute_ipa_summary_passes): Likewise. >> >> >> > (execute_one_pass): Don't check pass->has_execute flag. >> >> >> > (ipa_write_summaries_2): Don't check pass->has_gate flag. >> >> >> > (ipa_write_optimization_summaries_1): Likewise. >> >> >> > (ipa_read_summaries_1): Likewise. >> >> >> > (ipa_read_optimization_summaries_1): Likewise. >> >> >> > (execute_ipa_stmt_fixups): Likewise. >> >> >> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and >> >> >> > has_execute to useless_has_execute to be sure they're unused. >> >> >> > (TODO_absolutely_nothing): New constant. >> >> >> > >> >> >> > >> >> >> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h >> >> >> > index 77d78eb..3bc0a99 100644 >> >> >> > --- a/gcc/pass_manager.h >> >> >> > +++ b/gcc/pass_manager.h >> >> >> > @@ -93,7 +93,7 @@ public: >> >> >> > >> >> >> > private: >> >> >> > void set_pass_for_id (int id, opt_pass *pass); >> >> >> > - int register_dump_files_1 (struct opt_pass *pass, int properties); >> >> >> > + void register_dump_files_1 (struct opt_pass *pass); >> >> >> > void register_dump_files (struct opt_pass *pass, int properties); >> >> >> > >> >> >> > private: >> >> >> > diff --git a/gcc/passes.c b/gcc/passes.c >> >> >> > index 19e5869..3b28dc9 100644 >> >> >> > --- a/gcc/passes.c >> >> >> > +++ b/gcc/passes.c >> >> >> > @@ -112,7 +112,7 @@ opt_pass::gate () >> >> >> > unsigned int >> >> >> > opt_pass::execute () >> >> >> > { >> >> >> > - return 0; >> >> >> > + return TODO_absolutely_nothing; >> >> >> > } >> >> >> > >> >> >> > opt_pass::opt_pass (const pass_data &data, context *ctxt) >> >> >> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass) >> >> >> > >> >> >> > /* Recursive worker function for register_dump_files. */ >> >> >> > >> >> >> > -int >> >> >> > +void >> >> >> > pass_manager:: >> >> >> > -register_dump_files_1 (struct opt_pass *pass, int properties) >> >> >> > +register_dump_files_1 (struct opt_pass *pass) >> >> >> > { >> >> >> > do >> >> >> > { >> >> >> > - int new_properties = (properties | pass->properties_provided) >> >> >> > - & ~pass->properties_destroyed; >> >> >> > - >> >> >> > if (pass->name && pass->name[0] != '*') >> >> >> > register_one_dump_file (pass); >> >> >> > >> >> >> > if (pass->sub) >> >> >> > - new_properties = register_dump_files_1 (pass->sub, new_properties); >> >> >> > - >> >> >> > - /* If we have a gate, combine the properties that we could have with >> >> >> > - and without the pass being examined. */ >> >> >> > - if (pass->has_gate) >> >> >> > - properties &= new_properties; >> >> >> > - else >> >> >> > - properties = new_properties; >> >> >> > + register_dump_files_1 (pass->sub); >> >> >> > >> >> >> > pass = pass->next; >> >> >> > } >> >> >> > while (pass); >> >> >> > - >> >> >> > - return properties; >> >> >> > } >> >> >> > >> >> >> > /* Register the dump files for the pass_manager starting at PASS. >> >> >> > @@ -739,7 +727,7 @@ pass_manager:: >> >> >> > register_dump_files (struct opt_pass *pass,int properties) >> >> >> > { >> >> >> > pass->properties_required |= properties; >> >> >> > - register_dump_files_1 (pass, properties); >> >> >> > + register_dump_files_1 (pass); >> >> >> > } >> >> >> > >> >> >> > struct pass_registry >> >> >> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent) >> >> >> > const char *pn; >> >> >> > bool is_on, is_really_on; >> >> >> > >> >> >> > - is_on = pass->has_gate ? pass->gate () : true; >> >> >> > + is_on = pass->gate (); >> >> >> > is_really_on = override_gate_status (pass, current_function_decl, is_on); >> >> >> > >> >> >> > if (pass->static_pass_number <= 0) >> >> >> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass) >> >> >> > >> >> >> > /* Execute all of the IPA_PASSes in the list. */ >> >> >> > if (ipa_pass->type == IPA_PASS >> >> >> > - && ((!pass->has_gate) || pass->gate ()) >> >> >> > + && pass->gate () >> >> >> > && ipa_pass->generate_summary) >> >> >> > { >> >> >> > pass_init_dump_file (pass); >> >> >> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass) >> >> >> > >> >> >> > /* Check whether gate check should be avoided. >> >> >> > User controls the value of the gate through the parameter "gate_status". */ >> >> >> > - gate_status = pass->has_gate ? pass->gate () : true; >> >> >> > + gate_status = pass->gate (); >> >> >> > gate_status = override_gate_status (pass, current_function_decl, gate_status); >> >> >> > >> >> >> > /* Override gate with plugin. */ >> >> >> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass) >> >> >> > timevar_push (pass->tv_id); >> >> >> > >> >> >> > /* Do it! */ >> >> >> > - if (pass->has_execute) >> >> >> > - { >> >> >> > - todo_after = pass->execute (); >> >> >> > - do_per_function (clear_last_verified, NULL); >> >> >> > - } >> >> >> > + todo_after = pass->execute (); >> >> >> > + if (todo_after != TODO_absolutely_nothing) >> >> >> > + do_per_function (clear_last_verified, NULL); >> >> >> > + else >> >> >> > + todo_after &= ~TODO_absolutely_nothing; >> >> >> > >> >> >> > /* Stop timevar. */ >> >> >> > if (pass->tv_id != TV_NONE) >> >> >> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state) >> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> >> > if (pass->type == IPA_PASS >> >> >> > && ipa_pass->write_summary >> >> >> > - && ((!pass->has_gate) || pass->gate ())) >> >> >> > + && pass->gate ()) >> >> >> > { >> >> >> > /* If a timevar is present, start it. */ >> >> >> > if (pass->tv_id) >> >> >> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s >> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> >> > if (pass->type == IPA_PASS >> >> >> > && ipa_pass->write_optimization_summary >> >> >> > - && ((!pass->has_gate) || pass->gate ())) >> >> >> > + && pass->gate ()) >> >> >> > { >> >> >> > /* If a timevar is present, start it. */ >> >> >> > if (pass->tv_id) >> >> >> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass) >> >> >> > gcc_assert (!cfun); >> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> >> > >> >> >> > - if ((!pass->has_gate) || pass->gate ()) >> >> >> > + if (pass->gate ()) >> >> >> > { >> >> >> > if (pass->type == IPA_PASS && ipa_pass->read_summary) >> >> >> > { >> >> >> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass) >> >> >> > gcc_assert (!cfun); >> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); >> >> >> > >> >> >> > - if ((!pass->has_gate) || pass->gate ()) >> >> >> > + if (pass->gate ()) >> >> >> > { >> >> >> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary) >> >> >> > { >> >> >> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass, >> >> >> > { >> >> >> > /* Execute all of the IPA_PASSes in the list. */ >> >> >> > if (pass->type == IPA_PASS >> >> >> > - && ((!pass->has_gate) || pass->gate ())) >> >> >> > + && pass->gate ()) >> >> >> > { >> >> >> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass; >> >> >> > >> >> >> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h >> >> >> > index 9efee1e..bed639e 100644 >> >> >> > --- a/gcc/tree-pass.h >> >> >> > +++ b/gcc/tree-pass.h >> >> >> > @@ -49,11 +49,11 @@ struct pass_data >> >> >> > >> >> >> > /* If true, this pass has its own implementation of the opt_pass::gate >> >> >> > method. */ >> >> >> > - bool has_gate; >> >> >> > + bool useless_has_gate; >> >> >> > >> >> >> > /* If true, this pass has its own implementation of the opt_pass::execute >> >> >> > method. */ >> >> >> > - bool has_execute; >> >> >> > + bool useless_has_execute; >> >> >> > >> >> >> > /* The timevar id associated with this pass. */ >> >> >> > /* ??? Ideally would be dynamically assigned. */ >> >> >> > @@ -299,6 +299,10 @@ protected: >> >> >> > /* Rebuild the callgraph edges. */ >> >> >> > #define TODO_rebuild_cgraph_edges (1 << 22) >> >> >> > >> >> >> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass >> >> >> > + did absolutely nothing. */ >> >> >> > +#define TODO_absolutely_nothing 1 << 23 >> >> >> > + >> >> >> > /* Internally used in execute_function_todo(). */ >> >> >> > #define TODO_update_ssa_any \ >> >> >> > (TODO_update_ssa \ >> >> >> > -- >> >> >> > 1.8.4.2 >> >> >> > > >
diff --git a/gcc/passes.c b/gcc/passes.c index c008a7b..038bca7 100644 --- a/gcc/passes.c +++ b/gcc/passes.c @@ -112,6 +112,9 @@ opt_pass::gate () unsigned int opt_pass::execute () { + if (sub) + execute_pass_list (sub); + return TODO_absolutely_nothing; } @@ -2240,8 +2243,7 @@ execute_pass_list (struct opt_pass *pass) { gcc_assert (pass->type == GIMPLE_PASS || pass->type == RTL_PASS); - if (execute_one_pass (pass) && pass->sub) - execute_pass_list (pass->sub); + execute_one_pass (pass); pass = pass->next; } while (pass); @@ -2552,21 +2554,7 @@ execute_ipa_pass_list (struct opt_pass *pass) gcc_assert (!current_function_decl); gcc_assert (!cfun); gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS); - if (execute_one_pass (pass) && pass->sub) - { - if (pass->sub->type == GIMPLE_PASS) - { - invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_START, NULL); - do_per_function_toporder ((void (*)(void *))execute_pass_list, - pass->sub); - invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_END, NULL); - } - else if (pass->sub->type == SIMPLE_IPA_PASS - || pass->sub->type == IPA_PASS) - execute_ipa_pass_list (pass->sub); - else - gcc_unreachable (); - } + execute_one_pass (pass); gcc_assert (!current_function_decl); cgraph_process_new_functions (); pass = pass->next;