| Message ID | 1484015904-32671-6-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. jeff
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
On 01/17/2017 02:28 AM, Richard Biener wrote: >> >> 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. David suggested that we could have a method in the pass manager that would be run if the pass is skipped. "run_if_skipped" or some such. What I like about that idea is the hack and the real code end up in the same place. So someone working on (for example) reload has a much better chance of catching that they need to update the run_if_skipped method as they make changes to reload. It doesn't fix all the problems in this space, but I think it's cleaner than bundling the hacks into the pass manager itself. Would that work for you? It does for me. jeff
On Wed, 2017-01-18 at 09:36 -0700, Jeff Law wrote: > On 01/17/2017 02:28 AM, Richard Biener wrote: > > > > > > 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. > David suggested that we could have a method in the pass manager that > would be run if the pass is skipped. "run_if_skipped" or some such. > > What I like about that idea is the hack and the real code end up in > the > same place. So someone working on (for example) reload has a much > better chance of catching that they need to update the run_if_skipped > method as they make changes to reload. It doesn't fix all the > problems > in this space, but I think it's cleaner than bundling the hacks into > the > pass manager itself. > > Would that work for you? It does for me. > > jeff FWIW I posted an implementation of the idea here: https://gcc.gnu.org/ml/gcc-patches/2017-01/msg01268.html
On Wed, Jan 18, 2017 at 5:36 PM, Jeff Law <law@redhat.com> wrote: > On 01/17/2017 02:28 AM, Richard Biener wrote: >>> >>> >>> 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. > > David suggested that we could have a method in the pass manager that would > be run if the pass is skipped. "run_if_skipped" or some such. > > What I like about that idea is the hack and the real code end up in the same > place. So someone working on (for example) reload has a much better chance > of catching that they need to update the run_if_skipped method as they make > changes to reload. It doesn't fix all the problems in this space, but I > think it's cleaner than bundling the hacks into the pass manager itself. > > Would that work for you? It does for me. I think that walks in the wrong direction and just distributes the hack over multiple files. I'd rather have it in one place. Richard. > jeff >
On 01/19/2017 02:26 AM, Richard Biener wrote: > On Wed, Jan 18, 2017 at 5:36 PM, Jeff Law <law@redhat.com> wrote: >> On 01/17/2017 02:28 AM, Richard Biener wrote: >>>> >>>> >>>> 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. >> >> David suggested that we could have a method in the pass manager that would >> be run if the pass is skipped. "run_if_skipped" or some such. >> >> What I like about that idea is the hack and the real code end up in the same >> place. So someone working on (for example) reload has a much better chance >> of catching that they need to update the run_if_skipped method as they make >> changes to reload. It doesn't fix all the problems in this space, but I >> think it's cleaner than bundling the hacks into the pass manager itself. >> >> Would that work for you? It does for me. > > I think that walks in the wrong direction and just distributes the > hack over multiple > files. > > I'd rather have it in one place. We disagree, but I don't feel strongly enough about it to object. Though I'll probably chime in regularly as the list of hacks grows or gets out-of-date :-) Jeff
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. */ 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; + } 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) + /* Run any property provider. */ + 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 +2423,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. */