[PATCH, v2] (9e) Update "startwith" logic for pass-skipping to handle __RTL functions

[David Malcolm <dmalcolm at redhat dot 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