PR 56524: TREE_OPTIMIZATION_OPTABS vs. mips16

[Richard Sandiford <rdsandiford at googlemail dot com>]

This assert in save_optabs_if_changed triggered for gcc.dg/pr43564.c
on mips64-linux-gnu (-mabi=32/-mips16):

  /* ?? If this fails, we should temporarily restore the default
     target first (set_cfun (NULL) ??), do the rest of this function,
     and then restore it.  */
  gcc_assert (this_target_optabs == &default_target_optabs);

We can't do what the comment says because set_cfun (NULL) might not
choose default_target_optabs.  (On MIPS, default_target_optabs is
always for the standard encoding, in order to reduce the differences
between the -mips16 command-line option and "mips16" function attribute.)

The patch instead records which value of this_target_optabs was used
to generate TREE_OPTIMIZATION_OPTABS.  This has the additional advantage
that we won't recompute the optabs if successive functions use the same
optimisation node and same non-default target (which, given the above,
is relatively likely with -mips16).

Recording the base this_target_optabs also lets us initialise
TREE_OPTIMIZATION_OPTABS lazily, so that the C front end doesn't need
to compute optabs when parsing the "optimize" attribute.  That's probably
only a minor benefit though.

After this change, cfun->optabs is only useful on switchable targets
if we regularly switch between functions that have the same non-default
optimisation node and _different_ subtargets.  This seems like a
relatively unusual case so I removed cfun->optabs for simplicity.
However, Jakub didn't think that was a good idea, and since "simplicity"
is a bit of weak argument (especially as Jakub says for something that's
already implemented), I was going to submit two alternatives, one that
kept cfun->optabs and one that didn't.

However, I think there are two other benefits to removing cfun->optabs:

- It means that the TREE_OPTIMIZATION_OPTABS are unequivocally owned by
  the optimisation node, rather than shared between the node and the
  function structure.  It's therefore safe to clear the old optabs when
  we want to recompute them.

- The current implementation of the cache is:

	  struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
	  if (fn->optabs == NULL)
	    {
	      if (this_target_optabs == &default_target_optabs)
		fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
	      else
		{
		  fn->optabs = (unsigned char *)
		    ggc_alloc_atomic (sizeof (struct target_optabs));
		  init_all_optabs ((struct target_optabs *) fn->optabs);
		}
	    }
	  this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs
	                              : this_target_optabs;

  The equivalent code under the new scheme would be:

	  struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
	  if (fn->optabs == NULL)
	    {
	      init_tree_optimization_optabs (opts);
	      fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
	    }
	  this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs
	                              : this_target_optabs;

  But this wouldn't help if the optimisation node does not need different
  optabs from this_target_optabs.  fn->optabs would remain null and
  we'd still go through init_tree_optimization_optabs each time.

  And I think that's almost always going to be the case with optabs
  on MIPS, the only switchable target.  I think the only optabs that
  depend on flags are the floating-point division patterns, which depend
  on flag_unsafe_math_optimizations when using -mfix-sb1.  But the SB-1
  doesn't have MIPS16 support, so it's unlikely that you'd use the two
  together.  And that's building on top of what already seems an unlikely
  situation.  Assuming I've not missed some patterns (which is a very risky
  assumption), I'm not sure anyone would benefit in practice.

  We could add a flag to say whether cfun->optabs is valid, but that's
  adding even more complexity.

Tested on x86_64-linux-gnu and mips64-linux-gnu.  OK to install?

Richard


gcc/
	PR middle-end/56524
	* tree.h (tree_optimization_option): Rename target_optabs to optabs.
	Add base_optabs.
	(TREE_OPTIMIZATION_OPTABS): Update after previous field change.
	(TREE_OPTIMIZATION_BASE_OPTABS): New macro.
	(save_optabs_if_changed): Replace with...
	(init_tree_optimization_optabs): ...this.
	* optabs.c (save_optabs_if_changed): Rename to...
	(init_tree_optimization_optabs): ...this.  Take the optimization node
	as argument.  Do nothing if the base optabs are already correct.
	* function.h (function): Remove optabs field.
	* function.c (invoke_set_current_function_hook): Call
	init_tree_optimization_optabs.  Use the result to initialize
	this_fn_optabs.

gcc/c-family/
	PR middle-end/56524
	* c-common.c (handle_optimize_attribute): Don't call
	save_optabs_if_changed.

gcc/testsuite/
	PR middle-end/56524
	* gcc.target/mips/pr56524.c: New test.

Index: gcc/tree.h
===================================================================
--- gcc/tree.h	2013-03-07 19:23:42.019041307 +0000
+++ gcc/tree.h	2013-03-07 19:42:05.427263768 +0000
@@ -3589,21 +3589,26 @@ struct GTY(()) tree_optimization_option
 
   /* Target optabs for this set of optimization options.  This is of
      type `struct target_optabs *'.  */
-  unsigned char *GTY ((atomic)) target_optabs;
+  unsigned char *GTY ((atomic)) optabs;
+
+  /* The value of this_target_optabs against which the optabs above were
+     generated.  */
+  struct target_optabs *GTY ((skip)) base_optabs;
 };
 
 #define TREE_OPTIMIZATION(NODE) \
   (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts)
 
 #define TREE_OPTIMIZATION_OPTABS(NODE) \
-  (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs)
+  (OPTIMIZATION_NODE_CHECK (NODE)->optimization.optabs)
+
+#define TREE_OPTIMIZATION_BASE_OPTABS(NODE) \
+  (OPTIMIZATION_NODE_CHECK (NODE)->optimization.base_optabs)
 
 /* Return a tree node that encapsulates the current optimization options.  */
 extern tree build_optimization_node (void);
 
-/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the
-   current set of optabs has changed.  */
-extern void save_optabs_if_changed (tree);
+extern void init_tree_optimization_optabs (tree);
 
 /* Target options used by a function.  */
 
Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	2013-03-07 19:23:42.019041307 +0000
+++ gcc/optabs.c	2013-03-07 19:42:05.426263760 +0000
@@ -6208,19 +6208,25 @@ init_optabs (void)
   targetm.init_libfuncs ();
 }
 
-/* Recompute the optabs and save them if they have changed.  */
+/* Use the current target and options to initialize
+   TREE_OPTIMIZATION_OPTABS (OPTNODE).  */
 
 void
-save_optabs_if_changed (tree fndecl)
+init_tree_optimization_optabs (tree optnode)
 {
-  /* ?? If this fails, we should temporarily restore the default
-     target first (set_cfun (NULL) ??), do the rest of this function,
-     and then restore it.  */
-  gcc_assert (this_target_optabs == &default_target_optabs);
+  /* Quick exit if we have already computed optabs for this target.  */
+  if (TREE_OPTIMIZATION_BASE_OPTABS (optnode) == this_target_optabs)
+    return;
 
+  /* Forget any previous information and set up for the current target.  */
+  TREE_OPTIMIZATION_BASE_OPTABS (optnode) = this_target_optabs;
   struct target_optabs *tmp_optabs = (struct target_optabs *)
-    ggc_alloc_atomic (sizeof (struct target_optabs));
-  tree optnode = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl);
+    TREE_OPTIMIZATION_OPTABS (optnode);
+  if (tmp_optabs)
+    memset (tmp_optabs, 0, sizeof (struct target_optabs));
+  else
+    tmp_optabs = (struct target_optabs *)
+      ggc_alloc_atomic (sizeof (struct target_optabs));
 
   /* Generate a new set of optabs into tmp_optabs.  */
   init_all_optabs (tmp_optabs);
Index: gcc/function.h
===================================================================
--- gcc/function.h	2013-03-07 19:23:42.019041307 +0000
+++ gcc/function.h	2013-03-07 19:42:05.425263752 +0000
@@ -580,9 +580,6 @@ struct GTY(()) function {
      a string describing the reason for failure.  */
   const char * GTY((skip)) cannot_be_copied_reason;
 
-  /* Optabs for this function.  This is of type `struct target_optabs *'.  */
-  unsigned char *GTY ((atomic)) optabs;
-
   /* Collected bit flags.  */
 
   /* Number of units of general registers that need saving in stdarg
Index: gcc/function.c
===================================================================
--- gcc/function.c	2013-03-07 19:23:42.019041307 +0000
+++ gcc/function.c	2013-03-07 19:42:05.425263752 +0000
@@ -4400,25 +4400,14 @@ invoke_set_current_function_hook (tree f
 	}
 
       targetm.set_current_function (fndecl);
+      this_fn_optabs = this_target_optabs;
 
-      if (opts == optimization_default_node)
-	this_fn_optabs = this_target_optabs;
-      else
+      if (opts != optimization_default_node)
 	{
-	  struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
-	  if (fn->optabs == NULL)
-	    {
-	      if (this_target_optabs == &default_target_optabs)
-		fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
-	      else
-		{
-		  fn->optabs = (unsigned char *)
-		    ggc_alloc_atomic (sizeof (struct target_optabs));
-		  init_all_optabs ((struct target_optabs *) fn->optabs);
-		}
-	    }
-	  this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs
-	                              : this_target_optabs;
+	  init_tree_optimization_optabs (opts);
+	  if (TREE_OPTIMIZATION_OPTABS (opts))
+	    this_fn_optabs = (struct target_optabs *)
+	      TREE_OPTIMIZATION_OPTABS (opts);
 	}
     }
 }
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	2013-03-07 19:23:42.019041307 +0000
+++ gcc/c-family/c-common.c	2013-03-07 19:42:05.424263744 +0000
@@ -8947,8 +8947,6 @@ handle_optimize_attribute (tree *node, t
       DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
 	= build_optimization_node ();
 
-      save_optabs_if_changed (*node);
-
       /* Restore current options.  */
       cl_optimization_restore (&global_options, &cur_opts);
     }
Index: gcc/testsuite/gcc.target/mips/pr56524.c
===================================================================
--- /dev/null	2013-03-06 23:07:14.594799386 +0000
+++ gcc/testsuite/gcc.target/mips/pr56524.c	2013-03-07 19:43:01.038692028 +0000
@@ -0,0 +1,8 @@
+/* { dg-options "-mips16" } */
+
+void bar (void) {}
+
+void __attribute__((optimize("schedule-insns")))
+foo (void)
+{
+}