From: David Malcolm <dmalcolm@redhat.com>
To: Jeff Law <law@redhat.com>,
gcc-patches@gcc.gnu.org,
Richard Biener <richard.guenther@gmail.com>
Cc: David Malcolm <dmalcolm@redhat.com>
Subject: [PATCH, v2] (9e) Update "startwith" logic for pass-skipping to handle __RTL functions
Date: Thu, 19 Jan 2017 16:52:00 -0000 [thread overview]
Message-ID: <1484846531-30284-1-git-send-email-dmalcolm@redhat.com> (raw)
In-Reply-To: <CAFiYyc35cXBrOKMP+rUq20UA=cdf7do-AcA98BCAMk3vvWsZJg@mail.gmail.com>
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
next prev parent reply other threads:[~2017-01-19 16:47 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-10 2:04 [PATCH 9/9] Add "__RTL" to cc1 (v8) David Malcolm
2017-01-10 2:04 ` [PATCH 9j] testsuite: add x86_64-specific files David Malcolm
2017-01-16 21:59 ` Jeff Law
2017-01-10 2:04 ` [PATCH 9h] testsuite: add platform-independent files David Malcolm
2017-01-16 21:56 ` Jeff Law
2017-01-10 2:04 ` [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions David Malcolm
2017-01-16 21:42 ` Jeff Law
2017-01-17 9:28 ` Richard Biener
2017-01-17 21:10 ` [PATCH] Introduce opt_pass::skip virtual function David Malcolm
2017-01-18 16:39 ` [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions Jeff Law
2017-01-18 17:19 ` David Malcolm
2017-01-19 9:31 ` Richard Biener
2017-01-19 16:52 ` David Malcolm [this message]
2017-01-20 8:07 ` [PATCH, v2] (9e) " Richard Biener
2017-01-20 14:57 ` David Malcolm
2017-01-20 15:20 ` Richard Biener
2017-01-24 17:55 ` [committed][PATCH 9/9] Add "__RTL" to cc1 (v9) David Malcolm
2017-01-24 18:06 ` Joseph Myers
2017-01-23 23:59 ` [PATCH, v2] (9e) Update "startwith" logic for pass-skipping to handle __RTL functions Jeff Law
2017-01-20 20:37 ` [PATCH 9e] " Jeff Law
2017-01-10 2:04 ` [PATCH 9a] Add "__RTL" to C frontend David Malcolm
2017-01-10 22:57 ` Joseph Myers
2017-01-10 2:04 ` [PATCH 9c] callgraph: handle __RTL functions David Malcolm
2017-01-16 21:25 ` Jeff Law
2017-01-17 9:21 ` Richard Biener
2017-01-17 12:36 ` Jan Hubicka
2017-01-17 17:25 ` David Malcolm
2017-01-18 12:51 ` Jan Hubicka
2017-01-18 0:35 ` Jeff Law
2017-01-10 2:04 ` [PATCH 9i] testsuite: add aarch64-specific files David Malcolm
2017-01-16 22:00 ` Jeff Law
2017-01-10 2:04 ` [PATCH 9g] Extend .md and RTL parsing to support being wired up to cc1 David Malcolm
2017-01-16 22:04 ` Jeff Law
2017-01-16 23:09 ` David Malcolm
2017-01-10 2:04 ` [PATCH 9f] Add a way for the C frontend to compile __RTL-tagged functions David Malcolm
2017-01-16 21:55 ` Jeff Law
2017-01-16 23:23 ` David Malcolm
2017-01-22 9:05 ` Jeff Law
2017-01-10 2:04 ` [PATCH 9b] Don't assume that copy tables were initialized David Malcolm
2017-01-10 13:37 ` Richard Biener
2017-01-10 2:04 ` [PATCH 9d] Don't call delete_tree_ssa for __RTL functions David Malcolm
2017-01-10 13:42 ` Richard Biener
2017-01-16 21:15 ` Jeff Law
2017-01-16 21:14 ` Jeff Law
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1484846531-30284-1-git-send-email-dmalcolm@redhat.com \
--to=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).