On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote:
> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaund...@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, <tsaund...@mozilla.com> wrote:
> >> > From: Trevor Saunders <tsaund...@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
thanks
Trev
>
> Thanks,
> Richard.
>
> > Trev
> >
> >>
> >> Thanks,
> >> Richard.
> >>
> >> > Trev
> >> >
> >> > 2013-11-06 Trevor Saunders <tsaund...@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;
