Re: [PATCH, v2] (9e) Update "startwith" logic for pass-skipping to handle __RTL functions

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

On Thu, Jan 19, 2017 at 6:22 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Mon, 2017-01-16 at 14:42 -0700, Jeff Law wrote:
>> On 01/09/2017 07:38 PM, David Malcolm wrote:
>> > gcc/ChangeLog:
>> >     * passes.c: Include "insn-addr.h".
>> >     (should_skip_pass_p): Add logging.  Update logic for running
>> >     "expand" to be compatible with both __GIMPLE and __RTL.  Guard
>> >     property-provider override so it is only done for gimple
>> > passes.
>> >     Don't skip dfinit.
>> >     (skip_pass): New function.
>> >     (execute_one_pass): Call skip_pass when skipping passes.
>> > ---
>> >  gcc/passes.c | 65
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>> >  1 file changed, 58 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/gcc/passes.c b/gcc/passes.c
>> > index 31262ed..6954d1e 100644
>> > --- a/gcc/passes.c
>> > +++ b/gcc/passes.c
>> > @@ -59,6 +59,7 @@ along with GCC; see the file COPYING3.  If not
>> > see
>> >  #include "cfgrtl.h"
>> >  #include "tree-ssa-live.h"  /* For remove_unused_locals.  */
>> >  #include "tree-cfgcleanup.h"
>> > +#include "insn-addr.h" /* for INSN_ADDRESSES_ALLOC.  */
>> insn-addr?  Yuk.
>>
>>
>> >
>> >  using namespace gcc;
>> >
>> > @@ -2315,26 +2316,73 @@ should_skip_pass_p (opt_pass *pass)
>> >    if (!cfun->pass_startwith)
>> >      return false;
>> >
>> > -  /* We can't skip the lowering phase yet -- ideally we'd
>> > -     drive that phase fully via properties.  */
>> > -  if (!(cfun->curr_properties & PROP_ssa))
>> > -    return false;
>> > + /* For __GIMPLE functions, we have to at least start when we
>> > leave
>> > +     SSA.  */
>> > +  if (pass->properties_destroyed & PROP_ssa)
>> > +    {
>> > +      if (!quiet_flag)
>> > +   fprintf (stderr, "starting anyway when leaving SSA: %s\n",
>> > pass->name);
>> > +      cfun->pass_startwith = NULL;
>> > +      return false;
>> > +    }
>> This seems to need a comment -- it's not obvious how destroying the
>> SSA
>> property maps to a pass that can not be skipped.
>
> Added:
>
>   /* For __GIMPLE functions, we have to at least start when we leave
>      SSA.  Hence, we need to detect the "expand" pass, and stop skipping
>      when we encounter it.  A cheap way to identify "expand" is it to
>      detect the destruction of PROP_ssa.
>      For __RTL functions, we invoke "rest_of_compilation" directly, which
>      is after "expand", and hence we don't reach this conditional.  */
>
>> > -  /* And also run any property provider.  */
>> > -  if (pass->properties_provided != 0)
>> > +  /* Run any property provider.  */
>> > +  if (pass->type == GIMPLE_PASS
>> > +      && pass->properties_provided != 0)
>> >      return false;
>> So comment needed here too.  I read this as "if a gimple pass
>> provides a
>> property then it should not be skipped.  Which means that an RTL pass
>> that provides a property can?
>
> Added:
>
>   /* For GIMPLE passes, run any property provider (but continue skipping
>      afterwards).
>      We don't want to force running RTL passes that are property providers:
>      "expand" is covered above, and the only pass other than "expand" that
>      provides a property is "into_cfglayout" (PROP_cfglayout), which does
>      too much for a dumped __RTL function.  */
>
> ...the problem being that into_cfglayout's execute vfunc calls
> cfg_layout_initialize, which does a lot more that just
> cfg_layout_rtl_register_cfg_hooks (the skip hack does just the latter).
>
>> > +  /* Don't skip df init; later RTL passes need it.  */
>> > +  if (strstr (pass->name, "dfinit") != NULL)
>> > +    return false;
>> Which seems like a failing in RTL passes saying they need DF init.
>
> There isn't a "PROP_df"; should there be?
> Or is this hack accepable?
>
>> > +/* Skip the given pass, for handling passes before "startwith"
>> > +   in __GIMPLE and__RTL-marked functions.
>> > +   In theory, this ought to be a no-op, but some of the RTL passes
>> > +   need additional processing here.  */
>> > +
>> > +static void
>> > +skip_pass (opt_pass *pass)
>> ...
>> This all feels like a failing in how we handle state in the RTL
>> world.
>> And I suspect it's prone to error.  Imagine if I'm hacking on
>> something
>> in the RTL world and my code depends on something else being set up.
>>   I
>> really ought to have a way within my pass to indicate what I depend
>> on.
>> Having it hidden away in passes.c makes it easy to miss/forget.
>
> Indeed, it's a hack.  I preferred the vfunc idea, but Richi prefers
> to keep it all in one place.
>
>> > +{
>> > +  /* Pass "reload" sets the global "reload_completed", and many
>> > +     things depend on this (e.g. instructions in .md files).  */
>> > +  if (strcmp (pass->name, "reload") == 0)
>> > +    reload_completed = 1;
>> Seems like this ought to be a property provided by LRA/reload.
>
> If we have a __RTL function with a "startwith" of a pass after reload,
> we don't want to run "reload" when iterating through the pass list to
> reach the start pass, since presumably it could change the insns.  So
> if LRA/reload provide a property, say PROP_reload_completed, we'd still
> need a way to *not* run reload, whilst setting the reload_completed
> global.  So I don't think that a property necessarily buys us much
> here (it'd still be a hack either way...).
>
> Or is your observation more about having a way to identify the pass
> without doing a strcmp?
>
>> > +
>> > +  /* The INSN_ADDRESSES vec is normally set up by
>> > +     shorten_branches; set it up for the benefit of passes that
>> > +     run after this.  */
>> > +  if (strcmp (pass->name, "shorten") == 0)
>> > +    INSN_ADDRESSES_ALLOC (get_max_uid ());
>> Similarly ought to be provided by shorten-branches
>
> Similar to the reload_completed discussion above.
>
>
>> > +
>> > +  /* Update the cfg hooks as appropriate.  */
>> > +  if (strcmp (pass->name, "into_cfglayout") == 0)
>> > +    {
>> > +      cfg_layout_rtl_register_cfg_hooks ();
>> > +      cfun->curr_properties |= PROP_cfglayout;
>> > +    }
>> > +  if (strcmp (pass->name, "outof_cfglayout") == 0)
>> > +    {
>> > +      rtl_register_cfg_hooks ();
>> > +      cfun->curr_properties &= ~PROP_cfglayout;
>> > +    }
>> > +}
>> This feels somewhat different, but still a hack.
>>
>> I don't have strong suggestions on how to approach this, but what
>> we've
>> got here feels like a hack and one prone to bitrot.
>
> Given that Richi seems to prefer the "contain it all in once place"
> to the virtual function idea, there's not much more I can offer to fix it.
>
> Updated version of the patch attached (just adding the missing comments)
>
> Is this version OK?

Ok.

Richard.

> Changed in v2:
> - added some comments to should_skip_pass_p
>
> gcc/ChangeLog:
>         * passes.c: Include "insn-addr.h".
>         (should_skip_pass_p): Add logging.  Update logic for running
>         "expand" to be compatible with both __GIMPLE and __RTL.  Guard
>         property-provider override so it is only done for gimple passes.
>         Don't skip dfinit.
>         (skip_pass): New function.
>         (execute_one_pass): Call skip_pass when skipping passes.
> ---
>  gcc/passes.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 67 insertions(+), 7 deletions(-)
>
> diff --git a/gcc/passes.c b/gcc/passes.c
> index 31262ed..9886693 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -59,6 +59,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "cfgrtl.h"
>  #include "tree-ssa-live.h"  /* For remove_unused_locals.  */
>  #include "tree-cfgcleanup.h"
> +#include "insn-addr.h" /* for INSN_ADDRESSES_ALLOC.  */
>
>  using namespace gcc;
>
> @@ -2315,26 +2316,82 @@ should_skip_pass_p (opt_pass *pass)
>    if (!cfun->pass_startwith)
>      return false;
>
> -  /* We can't skip the lowering phase yet -- ideally we'd
> -     drive that phase fully via properties.  */
> -  if (!(cfun->curr_properties & PROP_ssa))
> -    return false;
> +  /* For __GIMPLE functions, we have to at least start when we leave
> +     SSA.  Hence, we need to detect the "expand" pass, and stop skipping
> +     when we encounter it.  A cheap way to identify "expand" is it to
> +     detect the destruction of PROP_ssa.
> +     For __RTL functions, we invoke "rest_of_compilation" directly, which
> +     is after "expand", and hence we don't reach this conditional.  */
> +  if (pass->properties_destroyed & PROP_ssa)
> +    {
> +      if (!quiet_flag)
> +       fprintf (stderr, "starting anyway when leaving SSA: %s\n", pass->name);
> +      cfun->pass_startwith = NULL;
> +      return false;
> +    }
>
>    if (determine_pass_name_match (pass->name, cfun->pass_startwith))
>      {
> +      if (!quiet_flag)
> +       fprintf (stderr, "found starting pass: %s\n", pass->name);
>        cfun->pass_startwith = NULL;
>        return false;
>      }
>
> -  /* And also run any property provider.  */
> -  if (pass->properties_provided != 0)
> +  /* For GIMPLE passes, run any property provider (but continue skipping
> +     afterwards).
> +     We don't want to force running RTL passes that are property providers:
> +     "expand" is covered above, and the only pass other than "expand" that
> +     provides a property is "into_cfglayout" (PROP_cfglayout), which does
> +     too much for a dumped __RTL function.  */
> +  if (pass->type == GIMPLE_PASS
> +      && pass->properties_provided != 0)
>      return false;
>
> +  /* Don't skip df init; later RTL passes need it.  */
> +  if (strstr (pass->name, "dfinit") != NULL)
> +    return false;
> +
> +  if (!quiet_flag)
> +    fprintf (stderr, "skipping pass: %s\n", pass->name);
> +
>    /* If we get here, then we have a "startwith" that we haven't seen yet;
>       skip the pass.  */
>    return true;
>  }
>
> +/* Skip the given pass, for handling passes before "startwith"
> +   in __GIMPLE and__RTL-marked functions.
> +   In theory, this ought to be a no-op, but some of the RTL passes
> +   need additional processing here.  */
> +
> +static void
> +skip_pass (opt_pass *pass)
> +{
> +  /* Pass "reload" sets the global "reload_completed", and many
> +     things depend on this (e.g. instructions in .md files).  */
> +  if (strcmp (pass->name, "reload") == 0)
> +    reload_completed = 1;
> +
> +  /* The INSN_ADDRESSES vec is normally set up by
> +     shorten_branches; set it up for the benefit of passes that
> +     run after this.  */
> +  if (strcmp (pass->name, "shorten") == 0)
> +    INSN_ADDRESSES_ALLOC (get_max_uid ());
> +
> +  /* Update the cfg hooks as appropriate.  */
> +  if (strcmp (pass->name, "into_cfglayout") == 0)
> +    {
> +      cfg_layout_rtl_register_cfg_hooks ();
> +      cfun->curr_properties |= PROP_cfglayout;
> +    }
> +  if (strcmp (pass->name, "outof_cfglayout") == 0)
> +    {
> +      rtl_register_cfg_hooks ();
> +      cfun->curr_properties &= ~PROP_cfglayout;
> +    }
> +}
> +
>  /* Execute PASS. */
>
>  bool
> @@ -2375,7 +2432,10 @@ execute_one_pass (opt_pass *pass)
>      }
>
>    if (should_skip_pass_p (pass))
> -    return true;
> +    {
> +      skip_pass (pass);
> +      return true;
> +    }
>
>    /* Pass execution event trigger: useful to identify passes being
>       executed.  */
> --
> 1.8.5.3
>