Patchwork PR target/52555: attribute optimize is overriding command line options

login
register
mail settings
Submitter Aldy Hernandez
Date Feb. 13, 2013, 6:07 p.m.
Message ID <511BD678.2090206@redhat.com>
Download mbox | patch
Permalink /patch/220219/
State New
Headers show

Comments

Aldy Hernandez - Feb. 13, 2013, 6:07 p.m.
> Sorry, just noticed:
>
>> +  /* If the optabs changed, record it in the node.  */
>> +  if (memcmp (tmp_target_optabs, &default_target_optabs,
>> +	      sizeof (struct target_optabs)))
>
> This should be this_target_optabs rather than &default_target_optabs.
> Nothing but target code and initialisers should use &default_target_optabs
> directly.

Fixed.

>
> I don't think that needs a retest though.  It only makes a difference
> on MIPS.

I verified that the testcase is still fixed by this patch (just in 
case).  Thanks so much for the review.

Final patch attached.

Jakub, OK?
+	PR target/52555
+	* genopinit.c (main): Use this_fn_optabs in generated
+	init_all_optabs, raw_optab_handler, and swap_optab_enable.
+	* tree.h (struct tree_optimization_option): New field
+	target_optabs.
+	(TREE_OPTIMIZATION_OPTABS): New.
+	(save_optabs_if_changed): Protoize.
+	* optabs.h: Declare this_fn_optabs.
+	* optabs.c (save_optabs_if_changed): New.
+	Declare this_fn_optabs.
+	* function.c (invoke_set_current_function_hook): Set
+	this_fn_optabs if there is one in the optimization node.
c-family/
+	PR target/52555
+	* c-common.c (handle_optimize_attribute): Call
+	save_optabs_if_changed.
Jakub Jelinek - Feb. 13, 2013, 7:54 p.m.
On Wed, Feb 13, 2013 at 12:07:52PM -0600, Aldy Hernandez wrote:
> >Sorry, just noticed:
> >
> >>+  /* If the optabs changed, record it in the node.  */
> >>+  if (memcmp (tmp_target_optabs, &default_target_optabs,
> >>+	      sizeof (struct target_optabs)))
> >
> >This should be this_target_optabs rather than &default_target_optabs.
> >Nothing but target code and initialisers should use &default_target_optabs
> >directly.
> 
> Fixed.
> 
> >
> >I don't think that needs a retest though.  It only makes a difference
> >on MIPS.
> 
> I verified that the testcase is still fixed by this patch (just in
> case).  Thanks so much for the review.
> 
> Final patch attached.

Actually, thinking more about SWITCHABLE_TARGETS, I don't think this works
at all on those targets.  this_target_optab is some random optab from
whatever has been left there by previous function, it can be either the
same, or different target (mips vs. mips16?).  So, for SWITCHABLE_TARGETS,
I'm afraid you really can't save the optabs in the optimization node
(because, there could be different optabs for say all of
-Ofast + mips, -Ofast + mips16, -O2 + mips and -O2 + mips16 combinations),
but you probably can't save it even from within the target hook, because
some non-default optimization node might be in effect.
So, for SWITCHABLE_TARGETS, I think you can only save it in
cfun->this_fn_optab or similar, and perhaps cache the default for mips16
only when you know the default optimization node is in effect (or force it
into effect temporarily).
For !SWITCHABLE_TARGETS, you can easily remember it in the optimization
nodes and just copy the pointer over to cfun->this_fn_optab.
And unfortunately it seems optimize attribute is supported everywhere, just
the target attribute is not.

But, as soon as it is reachable from cfun->this_fn_optab, we might need to
GC allocate it, while optimization nodes are never freed, cfun is freed I
think.

	Jakub

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..3711e69 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -8925,6 +8925,8 @@  handle_optimize_attribute (tree *node, tree name, tree args,
       DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)
 	= build_optimization_node ();
 
+      save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node));
+
       /* Restore current options.  */
       cl_optimization_restore (&global_options, &cur_opts);
     }
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..688242a 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4383,6 +4383,7 @@  static bool in_dummy_function;
 static void
 invoke_set_current_function_hook (tree fndecl)
 {
+  this_fn_optabs = this_target_optabs;
   if (!in_dummy_function)
     {
       tree opts = ((fndecl)
@@ -4397,6 +4398,11 @@  invoke_set_current_function_hook (tree fndecl)
 	{
 	  optimization_current_node = opts;
 	  cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts));
+
+	  /* Change optabs if needed.  */
+	  if (TREE_OPTIMIZATION_OPTABS (opts))
+	    this_fn_optabs
+	      = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts);
 	}
 
       targetm.set_current_function (fndecl);
diff --git a/gcc/genopinit.c b/gcc/genopinit.c
index 1bb2f77..13ebdc5 100644
--- a/gcc/genopinit.c
+++ b/gcc/genopinit.c
@@ -423,7 +423,7 @@  main (int argc, char **argv)
   fprintf (s_file, "};\n\n");
 
   fprintf (s_file, "void\ninit_all_optabs (void)\n{\n");
-  fprintf (s_file, "  bool *ena = this_target_optabs->pat_enable;\n");
+  fprintf (s_file, "  bool *ena = this_fn_optabs->pat_enable;\n");
   for (i = 0; patterns.iterate (i, &p); ++i)
     fprintf (s_file, "  ena[%u] = HAVE_%s;\n", i, p->name);
   fprintf (s_file, "}\n\n");
@@ -456,7 +456,7 @@  main (int argc, char **argv)
 	   "raw_optab_handler (unsigned scode)\n"
 	   "{\n"
 	   "  int i = lookup_handler (scode);\n"
-	   "  return (i >= 0 && this_target_optabs->pat_enable[i]\n"
+	   "  return (i >= 0 && this_fn_optabs->pat_enable[i]\n"
 	   "          ? pats[i].icode : CODE_FOR_nothing);\n"
 	   "}\n\n");
 
@@ -468,8 +468,8 @@  main (int argc, char **argv)
 	   "  int i = lookup_handler (scode);\n"
 	   "  if (i >= 0)\n"
 	   "    {\n"
-	   "      bool ret = this_target_optabs->pat_enable[i];\n"
-	   "      this_target_optabs->pat_enable[i] = set;\n"
+	   "      bool ret = this_fn_optabs->pat_enable[i];\n"
+	   "      this_fn_optabs->pat_enable[i] = set;\n"
 	   "      return ret;\n"
 	   "    }\n"
 	   "  else\n"
diff --git a/gcc/optabs.c b/gcc/optabs.c
index c1dacf4..00a13da 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -44,6 +44,7 @@  along with GCC; see the file COPYING3.  If not see
 
 struct target_optabs default_target_optabs;
 struct target_libfuncs default_target_libfuncs;
+struct target_optabs *this_fn_optabs = &default_target_optabs;
 #if SWITCHABLE_TARGET
 struct target_optabs *this_target_optabs = &default_target_optabs;
 struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs;
@@ -6207,6 +6208,40 @@  init_optabs (void)
   targetm.init_libfuncs ();
 }
 
+/* Recompute the optabs.  If they have changed, save the new set of
+   optabs in the optimization node OPTNODE.  */
+
+void
+save_optabs_if_changed (tree optnode)
+{
+  struct target_optabs *save_fn_optabs = this_fn_optabs;
+  struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs);
+
+  /* Generate a new set of optabs into tmp_target_optabs.  */
+  this_fn_optabs = tmp_target_optabs;
+  init_all_optabs ();
+  this_fn_optabs = save_fn_optabs;
+
+  /* If the optabs changed, record it in the node.  */
+  if (memcmp (tmp_target_optabs, this_target_optabs,
+	      sizeof (struct target_optabs)))
+    {
+      /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates
+	 multiple ((optimize)) attributes for the same function.  Is
+	 this even valid?  For now, just clobber the existing entry
+	 with the new optabs.  */
+      if (TREE_OPTIMIZATION_OPTABS (optnode))
+	XDELETE (TREE_OPTIMIZATION_OPTABS (optnode));
+
+      TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs;
+    }
+  else
+    {
+      TREE_OPTIMIZATION_OPTABS (optnode) = NULL;
+      XDELETE (tmp_target_optabs);
+    }
+}
+
 /* A helper function for init_sync_libfuncs.  Using the basename BASE,
    install libfuncs into TAB for BASE_N for 1 <= N <= MAX.  */
 
diff --git a/gcc/optabs.h b/gcc/optabs.h
index c08adcf..4de4409 100644
--- a/gcc/optabs.h
+++ b/gcc/optabs.h
@@ -76,6 +76,7 @@  struct target_optabs {
 };
 
 extern struct target_optabs default_target_optabs;
+extern struct target_optabs *this_fn_optabs;
 #if SWITCHABLE_TARGET
 extern struct target_optabs *this_target_optabs;
 #else
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c
new file mode 100644
index 0000000..7016834
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c
@@ -0,0 +1,10 @@ 
+/* { dg-options "-ffast-math" } */
+
+float farg;
+unsigned val;
+
+void __attribute__((optimize("O")))
+test()
+{
+  val = __builtin_ceilf(farg);
+}
diff --git a/gcc/tree.h b/gcc/tree.h
index c3c814c..eddbca8 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3586,14 +3586,25 @@  struct GTY(()) tree_optimization_option {
 
   /* The optimization options used by the user.  */
   struct cl_optimization opts;
+
+  /* Target optabs for this set of optimization options.  This is of
+     type `struct target_optabs *'.  */
+  void *GTY ((skip)) target_optabs;
 };
 
 #define TREE_OPTIMIZATION(NODE) \
   (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts)
 
+#define TREE_OPTIMIZATION_OPTABS(NODE) \
+  (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_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);
+
 /* Target options used by a function.  */
 
 struct GTY(()) tree_target_option {