| Message ID | 1484846531-30284-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
On Fri, 2017-01-20 at 09:06 +0100, Richard Biener wrote: > 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. [...snip...] Thanks. Current status of the RTL "frontend" is that patches 1-8 are in trunk, so that we're building the code for reading RTL dumps, but patch 9 (the code to wire that up to cc1 with "__RTL" so that it's actually usable from DejaGnu) isn't. I believe the only remaining part of patch 9 that hasn't been approved yet is: "[PATCH 9f] Add a way for the C frontend to compile __RTL-tagged functions" https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00590.html Richi: if that patch is approved, are you OK with patch 9 in early stage 4?
On January 20, 2017 3:56:31 PM GMT+01:00, David Malcolm <dmalcolm@redhat.com> wrote: >On Fri, 2017-01-20 at 09:06 +0100, Richard Biener wrote: >> 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. > >[...snip...] > >Thanks. > >Current status of the RTL "frontend" is that patches 1-8 are in trunk, >so that we're building the code for reading RTL dumps, but patch 9 (the >code to wire that up to cc1 with "__RTL" so that it's actually usable >from DejaGnu) isn't. > >I believe the only remaining part of patch 9 that hasn't been approved >yet is: > >"[PATCH 9f] Add a way for the C frontend to compile __RTL-tagged >functions" > https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00590.html > >Richi: if that patch is approved, are you OK with patch 9 in early >stage 4? Yes.
On 01/19/2017 10:22 AM, David Malcolm wrote: > > ...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? I think it's acceptable for now. I think long term we're going to want some kind of DF properties. > >>> +{ >>> + /* 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? It's not really the strcmp, but the general hackishness of this stuff. I would also have preferred the virtual function for skipping a pass, but not enough to argue that much about it. > >>> + >>> + /* 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. Right. > > >>> + >>> + /* 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. > Agreed/ > 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. OK. THanks for the updates. It should make it easier for others to see what's going on in here. jeff
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. */