On Tue, 2017-01-17 at 10:28 +0100, Richard Biener wrote:
> 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.
>
Another approach for handling this would be to add another virtual function
to class opt_pass, something like:
virtual void skip ();
The patch as-is groups together all of the various state-management
hacks into passes.c:skip_pass, whereas with a virtual function, all of
these hacks could be moved to the passes themselves.
Here's a patch (on top of 9e) which implements the idea.
Do you prefer this approach?
gcc/ChangeLog:
* cfgrtl.c (pass_into_cfg_layout_mode::skip): New method.
(pass_outof_cfg_layout_mode::skip): New method.
* final.c (pass_shorten_branches::skip): New method.
* ira.c (pass_reload::skip): New method.
* passes.c: Drop redundant include of "insn-addr.h".
(opt_pass::skip): New method.
(skip_pass): Delete.
(execute_one_pass): Use virtual function when skipping passes.
* tree-pass.h (opt_pass::skip): New virtual function.
---
gcc/cfgrtl.c | 22 ++++++++++++++++++++++
gcc/final.c | 9 +++++++++
gcc/ira.c | 9 +++++++++
gcc/passes.c | 42 ++++++++----------------------------------
gcc/tree-pass.h | 8 ++++++++
5 files changed, 56 insertions(+), 34 deletions(-)
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index b3b1146..953f807 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -3519,9 +3519,20 @@ public:
cfg_layout_initialize (0);
return 0;
}
+ void skip (function *) FINAL OVERRIDE;
}; // class pass_into_cfg_layout_mode
+/* Minimal version of this pass, for use when skipping this pass
+ with __RTL functions that start at a later pass. */
+
+void
+pass_into_cfg_layout_mode::skip (function *)
+{
+ cfg_layout_rtl_register_cfg_hooks ();
+ cfun->curr_properties |= PROP_cfglayout;
+}
+
} // anon namespace
rtl_opt_pass *
@@ -3554,6 +3565,7 @@ public:
/* opt_pass methods: */
virtual unsigned int execute (function *);
+ void skip (function *) FINAL OVERRIDE;
}; // class pass_outof_cfg_layout_mode
@@ -3571,6 +3583,16 @@ pass_outof_cfg_layout_mode::execute (function *fun)
return 0;
}
+/* Minimal version of this pass, for use when skipping this pass
+ with __RTL functions that start at a later pass. */
+
+void
+pass_outof_cfg_layout_mode::skip (function *)
+{
+ rtl_register_cfg_hooks ();
+ cfun->curr_properties &= ~PROP_cfglayout;
+}
+
} // anon namespace
rtl_opt_pass *
diff --git a/gcc/final.c b/gcc/final.c
index 2483381..e869031 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -4594,6 +4594,15 @@ public:
return rest_of_handle_shorten_branches ();
}
+ /* The INSN_ADDRESSES vec is normally set up by shorten_branches.
+ If we're skipping the pass for an __RTL function that starts at a
+ later pass, set it up for the benefit of passes that run after
+ this. */
+ void skip (function *) FINAL OVERRIDE
+ {
+ INSN_ADDRESSES_ALLOC (get_max_uid ());
+ }
+
}; // class pass_shorten_branches
} // anon namespace
diff --git a/gcc/ira.c b/gcc/ira.c
index 96b4b62..97f81da 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -5585,6 +5585,15 @@ public:
return 0;
}
+ /* Pass "reload" sets the global "reload_completed", and many
+ things depend on this (e.g. instructions in .md files).
+ Hence when skipping the pass for __RTL functions with "startwith" a
+ later pass, we need to set the flag. */
+ void skip (function *) FINAL OVERRIDE
+ {
+ reload_completed = 1;
+ }
+
}; // class pass_reload
} // anon namespace
diff --git a/gcc/passes.c b/gcc/passes.c
index 6954d1e..deb72f0 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -59,7 +59,6 @@ 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;
@@ -100,6 +99,13 @@ opt_pass::execute (function *)
return 0;
}
+/* The base class implementation of "skip" is a no-op. */
+
+void
+opt_pass::skip (function *)
+{
+}
+
opt_pass::opt_pass (const pass_data &data, context *ctxt)
: pass_data (data),
sub (NULL),
@@ -2351,38 +2357,6 @@ should_skip_pass_p (opt_pass *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
@@ -2424,7 +2398,7 @@ execute_one_pass (opt_pass *pass)
if (should_skip_pass_p (pass))
{
- skip_pass (pass);
+ pass->skip (cfun);
return true;
}
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 85bfba7..c32e126 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -95,6 +95,14 @@ public:
TODO_flags_finish. */
virtual unsigned int execute (function *fun);
+ /* A hook for use by the __GIMPLE and __RTL "frontends" when starting
+ at a specific pass. This hook is called for all of the skipped passes
+ before the start pass, in order to give them a chance to update any
+ internal state that isn't captured in the __GIMPLE or __RTL dumps.
+ Any usage of this function is considered a wart, since any such state
+ really ought to be captured in the dump. */
+ virtual void skip (function *fun);
+
protected:
opt_pass (const pass_data&, gcc::context *);
--
1.8.5.3
