[PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[Richard Biener]

On Mon, Jan 16, 2017 at 10:42 PM, Jeff Law <law@redhat.com> 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.
>>
>>
>>
>> -  /* 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?
>
>
>>
>> +  /* 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.
>
>
>
>> +/* 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.
>
>
>> +{
>> +  /* 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.
>
>
>> +
>> +  /* 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
>
>> +
>> +  /* 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.

All the above needs a bit of cleanup in the way we use (or not use) PROP_xxx.
For example right now you can't startwith a __GIMPLE with a pass inside the
loop pipeline because those passes expect loops to be initialized and be in
loop-closed SSA.  And with the hack above for the property providers you'll
always run pass_crited (that's a bad user of a PROP_).

Ideally we'd figure out required properties from the startwith pass
(but there's not
an easy way to compute it w/o actually "executing" the passes) and then enable
enough passes on the way to it providing those properties.

Or finally restructure things in a way that the pass manager automatically runs
property provider passes before passes requiring properties that are
not yet available...

Instead of those pass->name comparisions we could invent a new flag in the
pass structure whether a pass should always be run for __GIMPLE or ___RTL
but that's a bit noisy right now.

So I'm fine with the (localized) "hacks" for the moment.

Richard.

> jeff