This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]