Re: [PATCH] make has_gate and has_execute useless

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

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
>> >