Patchwork make has_gate and has_execute useless

login
register
mail settings
Submitter Trevor Saunders
Date Nov. 7, 2013, 4 p.m.
Message ID <1383840050-26620-1-git-send-email-tsaunders@mozilla.com>
Download mbox | patch
Permalink /patch/289399/
State New
Headers show

Comments

Trevor Saunders - Nov. 7, 2013, 4 p.m.
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.
Jeff Law - Nov. 7, 2013, 5:30 p.m.
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
Trevor Saunders - Nov. 7, 2013, 6:24 p.m.
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
Richard Guenther - Nov. 8, 2013, 9:37 a.m.
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
>
Trevor Saunders - Nov. 8, 2013, 3:21 p.m.
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
> >
Trevor Saunders - Nov. 11, 2013, 12:39 a.m.
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
> >
Richard Guenther - Nov. 11, 2013, 11:58 a.m.
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
>> >
Paolo Bonzini - Nov. 11, 2013, 5:41 p.m.
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

Patch

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			\