On Mon, Jan 16, 2017 at 10:42 PM, Jeff Law <l...@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
