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