| Message ID | 1383840050-26620-1-git-send-email-tsaunders@mozilla.com |
|---|---|
| State | New |
| Headers | show |
On 11/07/13 09:00, 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. Most definitely, the has_gate/has_execute stuff is definitely a wart. > > 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? I don't see anything glaringly wrong. Any reason why the register_dump_files bits were mucking with properties. That just seems wrong. > + todo_after = pass->execute (); > + if (todo_after != TODO_absolutely_nothing) > + do_per_function (clear_last_verified, NULL); > + else > + todo_after &= ~TODO_absolutely_nothing; Isn't the last assignment there just todo_after = 0; ? Jeff
On Thu, Nov 07, 2013 at 10:30:16AM -0700, Jeff Law wrote: > On 11/07/13 09:00, 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. > Most definitely, the has_gate/has_execute stuff is definitely a wart. > > > > >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? > I don't see anything glaringly wrong. great, I'll work on ripping them out then :) > Any reason why the register_dump_files bits were mucking with > properties. That just seems wrong. no, it looks like its pretty old, the most recent change I can see is r110742 in 2006, but I agree it seems crazy, but atleast its not actually doing anything now. > >+ todo_after = pass->execute (); > >+ if (todo_after != TODO_absolutely_nothing) > >+ do_per_function (clear_last_verified, NULL); > >+ else > >+ todo_after &= ~TODO_absolutely_nothing; > > Isn't the last assignment there just > todo_after = 0; yes Trev > > ? > > Jeff
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? 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 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? yes > 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). yes, because it was pretty obviously doing nothing useful, but I'm not sure what it was trying to do. > So can you split out removing has_gate? This part is obviously ok. ugh, already wrote / sent the patch that got rid of setting both flags. > 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? It seems nice and I'll try it. A quick look makes me worry a little about what execute_ipa_pass_list is doing with passes->sub, but I suspect that's the wrong place for whatever it is, and maybe it isn't actually an issue. 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 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. 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. 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 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 ;) > 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? 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 >> >
Il 08/11/2013 10:37, Richard Biener ha scritto: > /* 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). The properties argument to register_dump_files_1 is indeed dead code; it is never used except to compute new_properties which is also dead (because it is simply the argument in a recursive call). I seem to recall it was needed back when pass->type didn't exist; it used PROP_rtl to find whether a pass was tree or gimple, or something like that. The above test and "&=" were needed for this to work at all optimization levels, but I don't recall the details and as said above it's all dead anyway. The argument to register_dump_files is used. To remove it, one could change all_*_passes from a pointer to a "dummy" pass, and set properties_required in the initializer for the dummy pass. Paolo
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 \
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? 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.