Patchwork Multiversioning fixes (PR c++/55742)

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 18, 2013, 8:22 p.m.
Message ID <20130118202236.GE7269@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/213742/
State New
Headers show

Comments

Jakub Jelinek - Jan. 18, 2013, 8:22 p.m.
Hi!

As discussed in the PR, this patch attempts to change multiversioning
so that backwards compatibility with the previous target attribute behavior
is preserved.  In particular, e.g. for
void foo () __attribute__((target ("avx")));
void foo ()
{
}

the foo definition was just merged with the previous declaration and
used the avx target attribute.  The patch introduces
target ("default")
which should be used if a default multiversion variant is desirable.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I've mentioned a bunch of other issues with multiversioning in the PR
(including missing documentation for the feature), hope those can be
resolved incrementally.

2013-01-18  Jakub Jelinek  <jakub@redhat.com>

	PR c++/55742
	* config/i386/i386.c (ix86_valid_target_attribute_inner_p): Diagnose
	invalid args instead of ICEing on it.
	(ix86_valid_target_attribute_p): Allow target("default") attribute.
	(sorted_attr_string): Formatting fix.
	(ix86_mangle_function_version_assembler_n): Mangle target("default")
	as if no target attribute is present.
	(ix86_function_versions): Don't return true if one of the decls
	doesn't have target attribute.  If they don't and one of the decls
	is DECL_FUNCTION_VERSIONED, report an error.
	(is_function_default_version): Return true if there is
	target("default") attribute rather than no target attribute at all.

	* g++.dg/mv1.C: Adjust test.
	* g++.dg/mv2.C: Likewise.
	* g++.dg/mv5.C: Likewise.
	* g++.dg/mv6.C: Likewise.



	Jakub
Jason Merrill - Jan. 19, 2013, 4:19 p.m.
On 01/18/2013 03:22 PM, Jakub Jelinek wrote:
>     else if (TREE_CODE (args) != STRING_CST)
> -    gcc_unreachable ();
> +    {
> +      error ("invalid %<target%> attribute value");
> +      return false;
> +    }

Maybe say that it needs to be a string?

We also need more tests, both for the example in the PR (and your 
introductory text) and for error cases.

Thanks,
Jason

Patch

--- gcc/config/i386/i386.c.jj	2013-01-18 17:59:41.692943971 +0100
+++ gcc/config/i386/i386.c	2013-01-18 18:43:21.105326012 +0100
@@ -4223,7 +4223,10 @@  ix86_valid_target_attribute_inner_p (tre
     }
 
   else if (TREE_CODE (args) != STRING_CST)
-    gcc_unreachable ();
+    {
+      error ("invalid %<target%> attribute value");
+      return false;
+    }
 
   /* Handle multiple arguments separated by commas.  */
   next_optstr = ASTRDUP (TREE_STRING_POINTER (args));
@@ -4433,6 +4436,15 @@  ix86_valid_target_attribute_p (tree fnde
 {
   struct cl_target_option cur_target;
   bool ret = true;
+
+  /* attribute((target("default"))) does nothing, beyond
+     affecting multi-versioning.  */
+  if (TREE_VALUE (args)
+      && TREE_CODE (TREE_VALUE (args)) == STRING_CST
+      && TREE_CHAIN (args) == NULL_TREE
+      && strcmp (TREE_STRING_POINTER (TREE_VALUE (args)), "default") == 0)
+    return true;
+
   tree old_optimize = build_optimization_node ();
   tree new_target, new_optimize;
   tree func_optimize = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl);
@@ -28964,7 +28976,7 @@  sorted_attr_string (const char *str)
     if (str[i] == ',')
       argnum++;
 
-  attr_str = (char *)xmalloc (strlen (str) + 1);
+  attr_str = (char *) xmalloc (strlen (str) + 1);
   strcpy (attr_str, str);
 
   /* Replace "=,-" with "_".  */
@@ -29034,6 +29046,9 @@  ix86_mangle_function_version_assembler_n
   version_string
     = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (version_attr)));
 
+  if (strcmp (version_string, "default") == 0)
+    return id;
+
   attr_str = sorted_attr_string (version_string);
   assembler_name = (char *) xmalloc (strlen (orig_name)
 				     + strlen (attr_str) + 2);
@@ -29058,7 +29073,7 @@  ix86_function_versions (tree fn1, tree f
   const char *attr_str1, *attr_str2;
   char *target1, *target2;
   bool result;
-  
+
   if (TREE_CODE (fn1) != FUNCTION_DECL
       || TREE_CODE (fn2) != FUNCTION_DECL)
     return false;
@@ -29070,12 +29085,35 @@  ix86_function_versions (tree fn1, tree f
   if (attr1 == NULL_TREE && attr2 == NULL_TREE)
     return false;
 
-  /* If one function does not have a target attribute, these are versions.  */
+  /* Diagnose missing target attribute if one of the decls is already
+     multi-versioned.  */
   if (attr1 == NULL_TREE || attr2 == NULL_TREE)
-    return true;
+    {
+      if (DECL_FUNCTION_VERSIONED (fn1) || DECL_FUNCTION_VERSIONED (fn2))
+	{
+	  if (attr2 != NULL_TREE)
+	    {
+	      tree tem = fn1;
+	      fn1 = fn2;
+	      fn2 = tem;
+	      attr1 = attr2;
+	    }
+	  error_at (DECL_SOURCE_LOCATION (fn2),
+		    "missing %<target%> attribute for multi-versioned %D",
+		    fn2);
+	  error_at (DECL_SOURCE_LOCATION (fn1),
+		    "previous declaration of %D", fn1);
+	  /* Prevent diagnosing of the same error multiple times.  */
+	  DECL_ATTRIBUTES (fn2)
+	    = tree_cons (get_identifier ("target"),
+			 copy_node (TREE_VALUE (attr1)),
+			 DECL_ATTRIBUTES (fn2));
+	}
+      return false;
+    }
 
-  attr_str1 =  TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr1)));
-  attr_str2 =  TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr2)));
+  attr_str1 = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr1)));
+  attr_str2 = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr2)));
 
   target1 = sorted_attr_string (attr_str1);
   target2 = sorted_attr_string (attr_str2);
@@ -29196,9 +29234,14 @@  make_dispatcher_decl (const tree decl)
 static bool
 is_function_default_version (const tree decl)
 {
-  return (TREE_CODE (decl) == FUNCTION_DECL
-	  && DECL_FUNCTION_VERSIONED (decl)
-	  && lookup_attribute ("target", DECL_ATTRIBUTES (decl)) == NULL_TREE);
+  if (TREE_CODE (decl) != FUNCTION_DECL
+      || !DECL_FUNCTION_VERSIONED (decl))
+    return false;
+  tree attr = lookup_attribute ("target", DECL_ATTRIBUTES (decl));
+  gcc_assert (attr);
+  attr = TREE_VALUE (TREE_VALUE (attr));
+  return (TREE_CODE (attr) == STRING_CST
+	  && strcmp (TREE_STRING_POINTER (attr), "default") == 0);
 }
 
 /* Make a dispatcher declaration for the multi-versioned function DECL.
--- gcc/testsuite/g++.dg/mv1.C.jj	2013-01-18 17:59:41.567944606 +0100
+++ gcc/testsuite/g++.dg/mv1.C	2013-01-18 18:31:36.223533556 +0100
@@ -6,7 +6,7 @@ 
 #include <assert.h>
 
 /* Default version.  */
-int foo ();
+int foo () __attribute__ ((target("default")));
 /* The other versions of foo.  Mix up the ordering and 
    check if the dispatching does it in the order of priority. */
 /* Check combination of target attributes.  */
@@ -65,7 +65,8 @@  int main ()
   return 0;
 }
 
-int foo ()
+int __attribute__ ((target("default")))
+foo ()
 {
   return 0;
 }
--- gcc/testsuite/g++.dg/mv2.C.jj	2013-01-18 17:59:41.456945279 +0100
+++ gcc/testsuite/g++.dg/mv2.C	2013-01-18 18:31:36.224533535 +0100
@@ -7,7 +7,7 @@ 
 #include <assert.h>
 
 /* Default version.  */
-int foo ();
+int foo () __attribute__ ((target ("default")));
 /* The dispatch checks should be in the exact reverse order of the
    declarations below.  */
 int foo () __attribute__ ((target ("mmx")));
@@ -51,7 +51,7 @@  int main ()
   return 0;
 }
 
-int
+int __attribute__ ((target("default")))
 foo ()
 {
   return 0;
--- gcc/testsuite/g++.dg/mv5.C.jj	2013-01-18 17:59:41.587944512 +0100
+++ gcc/testsuite/g++.dg/mv5.C	2013-01-18 18:31:36.224533535 +0100
@@ -7,7 +7,7 @@ 
 
 
 /* Default version.  */
-inline int
+inline int __attribute__ ((target ("default")))
 foo ()
 {
   return 0;
--- gcc/testsuite/g++.dg/mv6.C.jj	2013-01-18 17:59:41.507944953 +0100
+++ gcc/testsuite/g++.dg/mv6.C	2013-01-18 18:31:36.224533535 +0100
@@ -8,6 +8,7 @@  class Foo
 {
  public:
   /* Default version of foo.  */
+  __attribute__ ((target("default")))
   int foo ()
   {
     return 0;