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

login
register
mail settings
Submitter Aldy Hernandez
Date Feb. 15, 2013, 5:23 p.m.
Message ID <511E6EF5.6040709@redhat.com>
Download mbox | patch
Permalink /patch/220781/
State New
Headers show

Comments

Aldy Hernandez - Feb. 15, 2013, 5:23 p.m.
> 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

Eeech... this significantly complicates things.

After some further interaction off-list... attached is a patch using 
cfun for SWITCHABLE_TARGETs, caching the default optimization node, and 
using GC for the pointers.

Jakub, I got rid of the suggested ifdef's because I'd prefer the 
compiler to compile/stress the non-common SWITCHABLE_TARGET case.  All 
targets define SWITCHABLE_TARGET to 0/1, so there should never be an 
undefined symbol.  If you'd prefer the ifdef, I can grudgingly change it 
back :).

[Richard, unfortunately there are some mips specific changes.]

How does this look y'all?
+2013-02-15  Aldy Hernandez  <aldyh@redhat.com>
+	    Jakub Jelinek  <jakub@redhat.com>
+
+	PR target/52555
+	* genopinit.c (raw_optab_handler): Use this_fn_optabs.
+	(swap_optab_enable): Same.
+	(init_all_optabs): Use argument instead of global.
+	* tree.h (struct tree_optimization_option): New field
+	target_optabs.
+	* expr.h (init_all_optabs): Add argument to prototype.
+	(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.
+	(init_optabs): Add argument to init_all_optabs() call.
+	* function.c (invoke_set_current_function_hook): Handle per
+	function optabs.
+	* function.h (struct function): New field optabs.
+	* config/mips/mips.c (mips_set_mips16_mode): Handle when
+	optimization_current_node has changed.
c/family
+	PR target/52555
+	* c-common.c (handle_optimize_attribute): Call
+	save_optabs_if_changed.
+
Jakub Jelinek - Feb. 15, 2013, 5:35 p.m.
On Fri, Feb 15, 2013 at 11:23:01AM -0600, Aldy Hernandez wrote:
> +2013-02-15  Aldy Hernandez  <aldyh@redhat.com>
> +	    Jakub Jelinek  <jakub@redhat.com>
> +
> +	PR target/52555
> +	* genopinit.c (raw_optab_handler): Use this_fn_optabs.
> +	(swap_optab_enable): Same.
> +	(init_all_optabs): Use argument instead of global.
> +	* tree.h (struct tree_optimization_option): New field
> +	target_optabs.
> +	* expr.h (init_all_optabs): Add argument to prototype.
> +	(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.
> +	(init_optabs): Add argument to init_all_optabs() call.
> +	* function.c (invoke_set_current_function_hook): Handle per
> +	function optabs.
> +	* function.h (struct function): New field optabs.
> +	* config/mips/mips.c (mips_set_mips16_mode): Handle when
> +	optimization_current_node has changed.
> c/family
> +	PR target/52555
> +	* c-common.c (handle_optimize_attribute): Call
> +	save_optabs_if_changed.

Looks good, just a few nits.  But please wait for Richard's feedback on it.

> +	      if (!SWITCHABLE_TARGET)
> +		fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
> +	      else
> +		{
> +		  if (this_target_optabs == &default_target_optabs)
> +		    fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);

Just use
		if (!SWITCHABLE_TARGET
		    || 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);
> +		    }
> +		}

and reindent.

> +	    }
> +	  this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs
> +	    : this_target_optabs;

I'd prefer : here be below ? on the line above it.

> +      /* ?? An existing 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));

The comment is wrong,
void foo (void) __attribute__((optimize (2)));
void foo (void) __attribute__((optimize ("fast-math")));
void foo (void) __attribute__((optimize ("unroll-loops")));

void
foo (void)
{
}
is just fine, and for foo results in -O2 -ffast-math -funroll-loops
options being in effect.
Plus XDELETE on GC allocated memory is wrong.  Just remove the comment
and if and XDELETE lines.

	Jakub
Richard Sandiford - Feb. 16, 2013, 11:19 a.m.
Aldy Hernandez <aldyh@redhat.com> writes:
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index b203cdd..5e98485 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -16313,7 +16313,26 @@ mips_set_mips16_mode (int mips16_p)
>    if (mips16_p)
>      {
>        if (!mips16_globals)
> -	mips16_globals = save_target_globals ();
> +	{
> +	  if (optimization_current_node != optimization_default_node)
> +	    {
> +	      tree opts = optimization_current_node;
> +	      /* Temporarily switch to the default optimization node,
> +		 so that *this_target_optabs is set to the default,
> +		 not reflecting whatever the first mips16 function
> +		 uses for the optimize attribute.  */
> +	      optimization_current_node = optimization_default_node;
> +	      cl_optimization_restore
> +		(&global_options,
> +		 TREE_OPTIMIZATION (optimization_default_node));
> +	      mips16_globals = save_target_globals ();
> +	      optimization_current_node = opts;
> +	      cl_optimization_restore (&global_options,
> +				       TREE_OPTIMIZATION (opts));
> +	    }
> +	  else
> +	    mips16_globals = save_target_globals ();	  
> +	}

I think this should go in target-globals.c, maybe as
save_target_globals_default_opts.

> diff --git a/gcc/function.c b/gcc/function.c
> index 4ce2259..c5eea2e 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -4400,6 +4400,31 @@ invoke_set_current_function_hook (tree fndecl)
>  	}
>  
>        targetm.set_current_function (fndecl);
> +
> +      if (opts == optimization_default_node)
> +	this_fn_optabs = this_target_optabs;
> +      else
> +	{
> +	  struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
> +	  if (fn->optabs == NULL)
> +	    {
> +	      if (!SWITCHABLE_TARGET)
> +		fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
> +	      else
> +		{
> +		  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);
> +		    }
> +		}

Following on from Jakub's:

		if (!SWITCHABLE_TARGET
		    || this_target_optabs == &default_target_optabs)
		  fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);

I'd prefer just:

		if (this_target_optabs == &default_target_optabs)
		  fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);

Looks good to me otherwise, thanks.

Sorry that SWITCHABLE_TARGETS has been so much hassle.  TBH, like Jakub
says in the PR, I was hoping things like the optimize attribute could
use the target_globals stuff too.  Today we just care about optabs,
but e.g. arm.c has:

  if (TARGET_THUMB1 && optimize_size)
    {
      /* When optimizing for size on Thumb-1, it's better not
        to use the HI regs, because of the overhead of
        stacking them.  */
      for (regno = FIRST_HI_REGNUM;
	   regno <= LAST_HI_REGNUM; ++regno)
	fixed_regs[regno] = call_used_regs[regno] = 1;
    }

which affects other cached global state like IRA tables.

The rtx_costs also often depend on optimize_size, and are cached in
various places like expmed.c.  E.g. for:

int
foo (int x, int y)
{
  return x * 10;
}

the -O2 version on MIPS32 is:

        sll     $2,$4,1
        sll     $4,$4,3
        j       $31
        addu    $2,$2,$4

and the -Os version is:

        li      $2,10
        j       $31
        mul     $2,$4,$2

But even if an optimize attribute is added:

int __attribute__((optimize(2)))
foo (int x, int y)
{
  return x * 10;
}

what you get depends on whether -Os or -O2 was passed on the command line.
The attribute doesn't affect things either way.  The same thing happens
on x86_64.

So in the end I think we'll end up trying solve the same problem that
the SWITCHABLE_TARGETS stuff was trying to solve, but this time for
__attribute__((optimize)).

Richard

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 1e6afaa..a1d47a6 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 (*node);
+
       /* Restore current options.  */
       cl_optimization_restore (&global_options, &cur_opts);
     }
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index b203cdd..5e98485 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16313,7 +16313,26 @@  mips_set_mips16_mode (int mips16_p)
   if (mips16_p)
     {
       if (!mips16_globals)
-	mips16_globals = save_target_globals ();
+	{
+	  if (optimization_current_node != optimization_default_node)
+	    {
+	      tree opts = optimization_current_node;
+	      /* Temporarily switch to the default optimization node,
+		 so that *this_target_optabs is set to the default,
+		 not reflecting whatever the first mips16 function
+		 uses for the optimize attribute.  */
+	      optimization_current_node = optimization_default_node;
+	      cl_optimization_restore
+		(&global_options,
+		 TREE_OPTIMIZATION (optimization_default_node));
+	      mips16_globals = save_target_globals ();
+	      optimization_current_node = opts;
+	      cl_optimization_restore (&global_options,
+				       TREE_OPTIMIZATION (opts));
+	    }
+	  else
+	    mips16_globals = save_target_globals ();	  
+	}
       else
 	restore_target_globals (mips16_globals);
     }
diff --git a/gcc/expr.h b/gcc/expr.h
index f5063b4..15fcb47 100644
--- a/gcc/expr.h
+++ b/gcc/expr.h
@@ -718,7 +718,7 @@  extern bool split_comparison (enum rtx_code, enum machine_mode,
 /* Call this once to initialize the contents of the optabs
    appropriately for the current target machine.  */
 extern void init_optabs (void);
-extern void init_all_optabs (void);
+extern void init_all_optabs (struct target_optabs *);
 
 /* Call this to initialize an optab function entry.  */
 extern rtx init_one_libfunc (const char *);
diff --git a/gcc/function.c b/gcc/function.c
index 4ce2259..c5eea2e 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4400,6 +4400,31 @@  invoke_set_current_function_hook (tree fndecl)
 	}
 
       targetm.set_current_function (fndecl);
+
+      if (opts == optimization_default_node)
+	this_fn_optabs = this_target_optabs;
+      else
+	{
+	  struct function *fn = DECL_STRUCT_FUNCTION (fndecl);
+	  if (fn->optabs == NULL)
+	    {
+	      if (!SWITCHABLE_TARGET)
+		fn->optabs = TREE_OPTIMIZATION_OPTABS (opts);
+	      else
+		{
+		  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;
+	}
     }
 }
 
diff --git a/gcc/function.h b/gcc/function.h
index 89d71e5..53e28b7 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -580,6 +580,9 @@  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
diff --git a/gcc/genopinit.c b/gcc/genopinit.c
index 1bb2f77..fb80717 100644
--- a/gcc/genopinit.c
+++ b/gcc/genopinit.c
@@ -422,8 +422,8 @@  main (int argc, char **argv)
     fprintf (s_file, "  { %#08x, CODE_FOR_%s },\n", p->sort_num, p->name);
   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, "void\ninit_all_optabs (struct target_optabs *optabs)\n{\n");
+  fprintf (s_file, "  bool *ena = 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..dd621c3 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;
@@ -6150,7 +6151,7 @@  init_optabs (void)
     libfunc_hash = htab_create_ggc (10, hash_libfunc, eq_libfunc, NULL);
 
   /* Fill in the optabs with the insns we support.  */
-  init_all_optabs ();
+  init_all_optabs (this_fn_optabs);
 
   /* The ffs function operates on `int'.  Fall back on it if we do not
      have a libgcc2 function for that width.  */
@@ -6207,6 +6208,41 @@  init_optabs (void)
   targetm.init_libfuncs ();
 }
 
+/* Recompute the optabs and save them if they have changed.  */
+
+void
+save_optabs_if_changed (tree fndecl)
+{
+  /* ?? 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);
+
+  struct target_optabs *tmp_optabs = (struct target_optabs *)
+    ggc_alloc_atomic (sizeof (struct target_optabs));
+  tree optnode = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl);
+
+  /* Generate a new set of optabs into tmp_optabs.  */
+  init_all_optabs (tmp_optabs);
+
+  /* If the optabs changed, record it.  */
+  if (memcmp (tmp_optabs, this_target_optabs, sizeof (struct target_optabs)))
+    {
+      /* ?? An existing 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) = (unsigned char *) tmp_optabs;
+    }
+  else
+    {
+      TREE_OPTIMIZATION_OPTABS (optnode) = NULL;
+      ggc_free (tmp_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..740d438 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 *'.  */
+  unsigned char *GTY ((atomic)) 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 {