Re: [PATCH] make has_gate and has_execute useless

[Richard Biener <richard dot guenther at gmail dot com>]

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
>