[PATCH] Function Multiversioning Bug, checking for function versions

[PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

Hi,

   Currently, function multiversioning determines that two functions
are different by comparing the arch type and isa flags that are set
after the target string is processed. This leads to cases where  the
versions become identical when the command-line target options are
altered.

For example, these two versions:

__attribute__ target (("sse4.2")))
int foo ()
{
}

__attribute__ target (("popcnt")))
int foo ()
{
}

become identical when -mpopcnt and -msse4.2 are used while building,
leading to build errors.

To avoid this, I have modified the function version determination to
just compare the target string.
Patch attached. Is this alright to submit?

Thanks,
-Sri.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3439 bytes --]

	* config/i386/i386.c (ix86_function_versions): Use target string
	to check for function versions instead of target flags.


Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 193486)
+++ config/i386/i386.c	(working copy)
@@ -28646,47 +28646,6 @@ dispatch_function_versions (tree dispatch_decl,
   return 0;
 }
 
-/* This function returns true if FN1 and FN2 are versions of the same function,
-   that is, the targets of the function decls are different.  This assumes
-   that FN1 and FN2 have the same signature.  */
-
-static bool
-ix86_function_versions (tree fn1, tree fn2)
-{
-  tree attr1, attr2;
-  struct cl_target_option *target1, *target2;
-
-  if (TREE_CODE (fn1) != FUNCTION_DECL
-      || TREE_CODE (fn2) != FUNCTION_DECL)
-    return false;
-
-  attr1 = DECL_FUNCTION_SPECIFIC_TARGET (fn1);
-  attr2 = DECL_FUNCTION_SPECIFIC_TARGET (fn2);
-
-  /* Atleast one function decl should have target attribute specified.  */
-  if (attr1 == NULL_TREE && attr2 == NULL_TREE)
-    return false;
-
-  if (attr1 == NULL_TREE)
-    attr1 = target_option_default_node;
-  else if (attr2 == NULL_TREE)
-    attr2 = target_option_default_node;
-
-  target1 = TREE_TARGET_OPTION (attr1);
-  target2 = TREE_TARGET_OPTION (attr2);
-
-  /* target1 and target2 must be different in some way.  */
-  if (target1->x_ix86_isa_flags == target2->x_ix86_isa_flags
-      && target1->x_target_flags == target2->x_target_flags
-      && target1->arch == target2->arch
-      && target1->tune == target2->tune
-      && target1->x_ix86_fpmath == target2->x_ix86_fpmath
-      && target1->branch_cost == target2->branch_cost)
-    return false;
-
-  return true;
-}
-
 /* Comparator function to be used in qsort routine to sort attribute
    specification strings to "target".  */
 
@@ -28799,6 +28758,52 @@ ix86_mangle_function_version_assembler_name (tree
   return get_identifier (assembler_name);
 }
 
+/* This function returns true if FN1 and FN2 are versions of the same function,
+   that is, the target strings of the function decls are different.  This assumes
+   that FN1 and FN2 have the same signature.  */
+
+static bool
+ix86_function_versions (tree fn1, tree fn2)
+{
+  tree attr1, attr2;
+  const char *attr_str1, *attr_str2;
+  char *target1, *target2;
+  bool result;
+  
+  if (TREE_CODE (fn1) != FUNCTION_DECL
+      || TREE_CODE (fn2) != FUNCTION_DECL)
+    return false;
+
+  attr1 = lookup_attribute ("target", DECL_ATTRIBUTES (fn1));
+  attr2 = lookup_attribute ("target", DECL_ATTRIBUTES (fn2));
+
+  /* Atleast one function decl should have the target attribute specified.  */
+  if (attr1 == NULL_TREE && attr2 == NULL_TREE)
+    return false;
+
+  /* If one function does not have a target attribute, these are versions.  */
+  if (attr1 == NULL_TREE || attr2 == NULL_TREE)
+    return true;
+
+  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);
+
+  /* The sorted target strings must be different for fn1 and fn2
+     to be versions.  */
+  if (strcmp (target1, target2) == 0)
+    result = false;
+  else
+    result = true;
+
+  free (target1);
+  free (target2); 
+  
+  return result;
+}
+
 static tree 
 ix86_mangle_decl_assembler_name (tree decl, tree id)
 {

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

Hi,

   The previous patch was incomplete because the front-end strips off
invalid target attributes which I did not consider. The attached
updated patch handles this with updated test cases.

Thanks,
-Sri.

On Thu, Nov 15, 2012 at 2:08 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Hi,
>
>    Currently, function multiversioning determines that two functions
> are different by comparing the arch type and isa flags that are set
> after the target string is processed. This leads to cases where  the
> versions become identical when the command-line target options are
> altered.
>
> For example, these two versions:
>
> __attribute__ target (("sse4.2")))
> int foo ()
> {
> }
>
> __attribute__ target (("popcnt")))
> int foo ()
> {
> }
>
> become identical when -mpopcnt and -msse4.2 are used while building,
> leading to build errors.
>
> To avoid this, I have modified the function version determination to
> just compare the target string.
> Patch attached. Is this alright to submit?
>
> Thanks,
> -Sri.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 10457 bytes --]


	* doc/tm.texi.in (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS): Document
	new target hook.
	* doc/tm.texi: Regenerate.
	* c-family/c-common.c (handle_target_attribute): Retain target attribute
	for targets that support versioning.
	* target.def (supports_function_versions): New hook.
	* config/i386/i386.c (ix86_function_versions): Use target string
	to check for function versions instead of target flags.
	* testsuite/g++.dg/mv1.C: Remove target options.
	* testsuite/g++.dg/mv2.C: Ditto.
	* testsuite/g++.dg/mv3.C: Ditto.
	* testsuite/g++.dg/mv4.C: Ditto.
	* testsuite/g++.dg/mv5.C: Ditto.
	* cp/class.c (add_method): Remove calls
	to DECL_FUNCTION_SPECIFIC_TARGET.
	* config/i386/i386.c (ix86_function_versions): Use target string
	to check for function versions instead of target flags.
	* (ix86_supports_function_versions): New function.
	* (is_function_default_version): Check target string.
	* TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS: New macro.
	

Index: doc/tm.texi
===================================================================
--- doc/tm.texi	(revision 193485)
+++ doc/tm.texi	(working copy)
@@ -9937,6 +9937,11 @@ different target specific attributes, that is, the
 different target machines.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS (void)
+This target hook returns @code{true} if the target supports function
+multiversioning.
+@end deftypefn
+
 @deftypefn {Target Hook} bool TARGET_CAN_INLINE_P (tree @var{caller}, tree @var{callee})
 This target hook returns @code{false} if the @var{caller} function
 cannot inline @var{callee}, based on target specific information.  By
Index: doc/tm.texi.in
===================================================================
--- doc/tm.texi.in	(revision 193485)
+++ doc/tm.texi.in	(working copy)
@@ -9798,6 +9798,11 @@ different target specific attributes, that is, the
 different target machines.
 @end deftypefn
 
+@hook TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS
+This target hook returns @code{true} if the target supports function
+multiversioning.
+@end deftypefn
+
 @hook TARGET_CAN_INLINE_P
 This target hook returns @code{false} if the @var{caller} function
 cannot inline @var{callee}, based on target specific information.  By
Index: c-family/c-common.c
===================================================================
--- c-family/c-common.c	(revision 193485)
+++ c-family/c-common.c	(working copy)
@@ -8720,8 +8720,12 @@ handle_target_attribute (tree *node, tree name, tr
       warning (OPT_Wattributes, "%qE attribute ignored", name);
       *no_add_attrs = true;
     }
+  /* Do not strip invalid target attributes for targets which support function
+     multiversioning as the target string is used to determine versioned
+     functions.  */
   else if (! targetm.target_option.valid_attribute_p (*node, name, args,
-						      flags))
+						      flags)
+	   && ! targetm.target_option.supports_function_versions ())
     *no_add_attrs = true;
 
   return NULL_TREE;
Index: target.def
===================================================================
--- target.def	(revision 193485)
+++ target.def	(working copy)
@@ -2826,6 +2826,14 @@ DEFHOOK
  bool, (tree decl1, tree decl2),
  hook_bool_tree_tree_false)
 
+/* This function returns true if the target supports function
+   multiversioning.  */
+DEFHOOK
+(supports_function_versions,
+ "",
+ bool, (void),
+ hool_bool_void_false)
+
 /* Function to determine if one function can inline another function.  */
 #undef HOOK_PREFIX
 #define HOOK_PREFIX "TARGET_"
Index: testsuite/g++.dg/mv2.C
===================================================================
--- testsuite/g++.dg/mv2.C	(revision 193485)
+++ testsuite/g++.dg/mv2.C	(working copy)
@@ -2,7 +2,7 @@
    dispatching order when versions are for various ISAs.  */
 /* { dg-do run { target i?86-*-* x86_64-*-* } } */
 /* { dg-require-ifunc "" }  */
-/* { dg-options "-O2 -mno-sse -mno-mmx -mno-popcnt -mno-avx" } */
+/* { dg-options "-O2" } */
 
 #include <assert.h>
 
Index: testsuite/g++.dg/mv4.C
===================================================================
--- testsuite/g++.dg/mv4.C	(revision 193486)
+++ testsuite/g++.dg/mv4.C	(working copy)
@@ -4,7 +4,7 @@
 
 /* { dg-do compile { target i?86-*-* x86_64-*-* } } */
 /* { dg-require-ifunc "" }  */
-/* { dg-options "-O2 -mno-sse -mno-popcnt" } */
+/* { dg-options "-O2" } */
 
 int __attribute__ ((target ("sse")))
 foo ()
Index: testsuite/g++.dg/mv1.C
===================================================================
--- testsuite/g++.dg/mv1.C	(revision 193485)
+++ testsuite/g++.dg/mv1.C	(working copy)
@@ -1,7 +1,7 @@
 /* Test case to check if Multiversioning works.  */
 /* { dg-do run { target i?86-*-* x86_64-*-* } } */
 /* { dg-require-ifunc "" }  */
-/* { dg-options "-O2 -fPIC -mno-avx -mno-popcnt" } */
+/* { dg-options "-O2 -fPIC" } */
 
 #include <assert.h>
 
Index: testsuite/g++.dg/mv3.C
===================================================================
--- testsuite/g++.dg/mv3.C	(revision 193485)
+++ testsuite/g++.dg/mv3.C	(working copy)
@@ -10,7 +10,7 @@
    test should pass.  */
 
 /* { dg-do run { target i?86-*-* x86_64-*-* } } */
-/* { dg-options "-O2 -mno-sse -mno-popcnt" } */
+/* { dg-options "-O2" } */
 
 
 int __attribute__ ((target ("sse")))
Index: testsuite/g++.dg/mv5.C
===================================================================
--- testsuite/g++.dg/mv5.C	(revision 193486)
+++ testsuite/g++.dg/mv5.C	(working copy)
@@ -3,7 +3,7 @@
 
 /* { dg-do run { target i?86-*-* x86_64-*-* } } */
 /* { dg-require-ifunc "" }  */
-/* { dg-options "-O2  -mno-popcnt" } */
+/* { dg-options "-O2" } */
 
 
 /* Default version.  */
Index: cp/class.c
===================================================================
--- cp/class.c	(revision 193486)
+++ cp/class.c	(working copy)
@@ -1095,8 +1095,6 @@ add_method (tree type, tree method, tree using_dec
 	      && TREE_CODE (method) == FUNCTION_DECL
 	      && !DECL_EXTERN_C_P (fn)
 	      && !DECL_EXTERN_C_P (method)
-	      && (DECL_FUNCTION_SPECIFIC_TARGET (fn)
-		  || DECL_FUNCTION_SPECIFIC_TARGET (method))
 	      && targetm.target_option.function_versions (fn, method))
  	    {
 	      /* Mark functions as versions if necessary.  Modify the mangled
Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 193486)
+++ config/i386/i386.c	(working copy)
@@ -28646,47 +28646,6 @@ dispatch_function_versions (tree dispatch_decl,
   return 0;
 }
 
-/* This function returns true if FN1 and FN2 are versions of the same function,
-   that is, the targets of the function decls are different.  This assumes
-   that FN1 and FN2 have the same signature.  */
-
-static bool
-ix86_function_versions (tree fn1, tree fn2)
-{
-  tree attr1, attr2;
-  struct cl_target_option *target1, *target2;
-
-  if (TREE_CODE (fn1) != FUNCTION_DECL
-      || TREE_CODE (fn2) != FUNCTION_DECL)
-    return false;
-
-  attr1 = DECL_FUNCTION_SPECIFIC_TARGET (fn1);
-  attr2 = DECL_FUNCTION_SPECIFIC_TARGET (fn2);
-
-  /* Atleast one function decl should have target attribute specified.  */
-  if (attr1 == NULL_TREE && attr2 == NULL_TREE)
-    return false;
-
-  if (attr1 == NULL_TREE)
-    attr1 = target_option_default_node;
-  else if (attr2 == NULL_TREE)
-    attr2 = target_option_default_node;
-
-  target1 = TREE_TARGET_OPTION (attr1);
-  target2 = TREE_TARGET_OPTION (attr2);
-
-  /* target1 and target2 must be different in some way.  */
-  if (target1->x_ix86_isa_flags == target2->x_ix86_isa_flags
-      && target1->x_target_flags == target2->x_target_flags
-      && target1->arch == target2->arch
-      && target1->tune == target2->tune
-      && target1->x_ix86_fpmath == target2->x_ix86_fpmath
-      && target1->branch_cost == target2->branch_cost)
-    return false;
-
-  return true;
-}
-
 /* Comparator function to be used in qsort routine to sort attribute
    specification strings to "target".  */
 
@@ -28799,6 +28758,60 @@ ix86_mangle_function_version_assembler_name (tree
   return get_identifier (assembler_name);
 }
 
+/* This function returns true if FN1 and FN2 are versions of the same function,
+   that is, the target strings of the function decls are different.  This assumes
+   that FN1 and FN2 have the same signature.  */
+
+static bool
+ix86_function_versions (tree fn1, tree fn2)
+{
+  tree attr1, attr2;
+  const char *attr_str1, *attr_str2;
+  char *target1, *target2;
+  bool result;
+  
+  if (TREE_CODE (fn1) != FUNCTION_DECL
+      || TREE_CODE (fn2) != FUNCTION_DECL)
+    return false;
+
+  attr1 = lookup_attribute ("target", DECL_ATTRIBUTES (fn1));
+  attr2 = lookup_attribute ("target", DECL_ATTRIBUTES (fn2));
+
+  /* Atleast one function decl should have the target attribute specified.  */
+  if (attr1 == NULL_TREE && attr2 == NULL_TREE)
+    return false;
+
+  /* If one function does not have a target attribute, these are versions.  */
+  if (attr1 == NULL_TREE || attr2 == NULL_TREE)
+    return true;
+
+  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);
+
+  /* The sorted target strings must be different for fn1 and fn2
+     to be versions.  */
+  if (strcmp (target1, target2) == 0)
+    result = false;
+  else
+    result = true;
+
+  free (target1);
+  free (target2); 
+  
+  return result;
+}
+
+/* This target supports function multiversioning.  */
+
+static bool
+ix86_supports_function_versions (void)
+{
+  return true;
+}
+
 static tree 
 ix86_mangle_decl_assembler_name (tree decl, tree id)
 {
@@ -28893,7 +28906,7 @@ is_function_default_version (const tree decl)
 {
   return (TREE_CODE (decl) == FUNCTION_DECL
 	  && DECL_FUNCTION_VERSIONED (decl)
-	  && DECL_FUNCTION_SPECIFIC_TARGET (decl) == NULL_TREE);
+	  && lookup_attribute ("target", DECL_ATTRIBUTES (decl)) == NULL_TREE);
 }
 
 /* Make a dispatcher declaration for the multi-versioned function DECL.
@@ -42154,6 +42167,10 @@ ix86_memmodel_check (unsigned HOST_WIDE_INT val)
 #undef TARGET_OPTION_FUNCTION_VERSIONS
 #define TARGET_OPTION_FUNCTION_VERSIONS ix86_function_versions
 
+#undef TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS
+#define TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS \
+  ix86_supports_function_versions
+
 #undef TARGET_CAN_INLINE_P
 #define TARGET_CAN_INLINE_P ix86_can_inline_p
 

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Diego Novillo]

On 2012-11-16 14:55 , Sriraman Tallam wrote:
> Hi,
>
>     The previous patch was incomplete because the front-end strips off
> invalid target attributes which I did not consider. The attached
> updated patch handles this with updated test cases.
>
> Thanks,
> -Sri.
>
> On Thu, Nov 15, 2012 at 2:08 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> Hi,
>>
>>     Currently, function multiversioning determines that two functions
>> are different by comparing the arch type and isa flags that are set
>> after the target string is processed. This leads to cases where  the
>> versions become identical when the command-line target options are
>> altered.
>>
>> For example, these two versions:
>>
>> __attribute__ target (("sse4.2")))
>> int foo ()
>> {
>> }
>>
>> __attribute__ target (("popcnt")))
>> int foo ()
>> {
>> }
>>
>> become identical when -mpopcnt and -msse4.2 are used while building,
>> leading to build errors.
>>
>> To avoid this, I have modified the function version determination to
>> just compare the target string.
>> Patch attached. Is this alright to submit?
>>
>> Thanks,
>> -Sri.
>
> 	* doc/tm.texi.in (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS): Document
> 	new target hook.
> 	* doc/tm.texi: Regenerate.
> 	* c-family/c-common.c (handle_target_attribute): Retain target attribute
> 	for targets that support versioning.
> 	* target.def (supports_function_versions): New hook.
> 	* config/i386/i386.c (ix86_function_versions): Use target string
> 	to check for function versions instead of target flags.
> 	* testsuite/g++.dg/mv1.C: Remove target options.
> 	* testsuite/g++.dg/mv2.C: Ditto.
> 	* testsuite/g++.dg/mv3.C: Ditto.
> 	* testsuite/g++.dg/mv4.C: Ditto.
> 	* testsuite/g++.dg/mv5.C: Ditto.
> 	* cp/class.c (add_method): Remove calls
> 	to DECL_FUNCTION_SPECIFIC_TARGET.
> 	* config/i386/i386.c (ix86_function_versions): Use target string
> 	to check for function versions instead of target flags.
> 	* (ix86_supports_function_versions): New function.
> 	* (is_function_default_version): Check target string.
> 	* TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS: New macro.
> 	

Looks mostly OK.

> Index: cp/class.c
> ===================================================================
> --- cp/class.c	(revision 193486)
> +++ cp/class.c	(working copy)
> @@ -1095,8 +1095,6 @@ add_method (tree type, tree method, tree using_dec
>  	      && TREE_CODE (method) == FUNCTION_DECL
>  	      && !DECL_EXTERN_C_P (fn)
>  	      && !DECL_EXTERN_C_P (method)
> -	      && (DECL_FUNCTION_SPECIFIC_TARGET (fn)
> -		  || DECL_FUNCTION_SPECIFIC_TARGET (method))

I don't follow.  Given the new hook, why do you need to remove this check?

> +/* This function returns true if FN1 and FN2 are versions of the same function,
> +   that is, the target strings of the function decls are different.  This assumes
> +   that FN1 and FN2 have the same signature.  */
> +
> +static bool
> +ix86_function_versions (tree fn1, tree fn2)
> +{

Any particular reason why you moved this function down here?

> +  tree attr1, attr2;
> +  const char *attr_str1, *attr_str2;
> +  char *target1, *target2;
> +  bool result;
> +
> +  if (TREE_CODE (fn1) != FUNCTION_DECL
> +      || TREE_CODE (fn2) != FUNCTION_DECL)
> +    return false;
> +
> +  attr1 = lookup_attribute ("target", DECL_ATTRIBUTES (fn1));
> +  attr2 = lookup_attribute ("target", DECL_ATTRIBUTES (fn2));
> +
> +  /* Atleast one function decl should have the target attribute specified.  */

s/Atleast/At least/


Diego.

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

Hi Diego,

   Thanks for the review.

On Tue, Dec 18, 2012 at 8:04 AM, Diego Novillo <dnovillo@google.com> wrote:
> On 2012-11-16 14:55 , Sriraman Tallam wrote:
>>
>> Hi,
>>
>>     The previous patch was incomplete because the front-end strips off
>> invalid target attributes which I did not consider. The attached
>> updated patch handles this with updated test cases.
>>
>> Thanks,
>> -Sri.
>>
>> On Thu, Nov 15, 2012 at 2:08 PM, Sriraman Tallam <tmsriram@google.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>     Currently, function multiversioning determines that two functions
>>> are different by comparing the arch type and isa flags that are set
>>> after the target string is processed. This leads to cases where  the
>>> versions become identical when the command-line target options are
>>> altered.
>>>
>>> For example, these two versions:
>>>
>>> __attribute__ target (("sse4.2")))
>>> int foo ()
>>> {
>>> }
>>>
>>> __attribute__ target (("popcnt")))
>>> int foo ()
>>> {
>>> }
>>>
>>> become identical when -mpopcnt and -msse4.2 are used while building,
>>> leading to build errors.
>>>
>>> To avoid this, I have modified the function version determination to
>>> just compare the target string.
>>> Patch attached. Is this alright to submit?
>>>
>>> Thanks,
>>> -Sri.
>>
>>
>>         * doc/tm.texi.in (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS):
>> Document
>>         new target hook.
>>         * doc/tm.texi: Regenerate.
>>         * c-family/c-common.c (handle_target_attribute): Retain target
>> attribute
>>         for targets that support versioning.
>>         * target.def (supports_function_versions): New hook.
>>         * config/i386/i386.c (ix86_function_versions): Use target string
>>         to check for function versions instead of target flags.
>>         * testsuite/g++.dg/mv1.C: Remove target options.
>>         * testsuite/g++.dg/mv2.C: Ditto.
>>         * testsuite/g++.dg/mv3.C: Ditto.
>>         * testsuite/g++.dg/mv4.C: Ditto.
>>         * testsuite/g++.dg/mv5.C: Ditto.
>>         * cp/class.c (add_method): Remove calls
>>         to DECL_FUNCTION_SPECIFIC_TARGET.
>>         * config/i386/i386.c (ix86_function_versions): Use target string
>>         to check for function versions instead of target flags.
>>         * (ix86_supports_function_versions): New function.
>>         * (is_function_default_version): Check target string.
>>         * TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS: New macro.
>>
>
>
> Looks mostly OK.
>
>> Index: cp/class.c
>> ===================================================================
>> --- cp/class.c  (revision 193486)
>> +++ cp/class.c  (working copy)
>> @@ -1095,8 +1095,6 @@ add_method (tree type, tree method, tree using_dec
>>               && TREE_CODE (method) == FUNCTION_DECL
>>               && !DECL_EXTERN_C_P (fn)
>>               && !DECL_EXTERN_C_P (method)
>> -             && (DECL_FUNCTION_SPECIFIC_TARGET (fn)
>> -                 || DECL_FUNCTION_SPECIFIC_TARGET (method))
>
>
> I don't follow.  Given the new hook, why do you need to remove this check?

The function versions are now determined purely based on the string
value and not on DECL_FUNCTION_SPECIFIC_TARGET fields. So, this check
is not necessary.


>
>> +/* This function returns true if FN1 and FN2 are versions of the same
>> function,
>> +   that is, the target strings of the function decls are different.  This
>> assumes
>> +   that FN1 and FN2 have the same signature.  */
>> +
>> +static bool
>> +ix86_function_versions (tree fn1, tree fn2)
>> +{
>
>
> Any particular reason why you moved this function down here?

Just refactoring, keeping functions that are related to
ix86_dispatch_versions together.

>
>> +  tree attr1, attr2;
>> +  const char *attr_str1, *attr_str2;
>> +  char *target1, *target2;
>> +  bool result;
>> +
>> +  if (TREE_CODE (fn1) != FUNCTION_DECL
>> +      || TREE_CODE (fn2) != FUNCTION_DECL)
>> +    return false;
>> +
>> +  attr1 = lookup_attribute ("target", DECL_ATTRIBUTES (fn1));
>> +  attr2 = lookup_attribute ("target", DECL_ATTRIBUTES (fn2));
>> +
>> +  /* Atleast one function decl should have the target attribute
>> specified.  */
>
>
> s/Atleast/At least/

I will make this change.

Thanks,
-Sri.

>
>
> Diego.

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Diego Novillo]

On Tue, Dec 18, 2012 at 6:38 PM, Sriraman Tallam <tmsriram@google.com> wrote:

> The function versions are now determined purely based on the string
> value and not on DECL_FUNCTION_SPECIFIC_TARGET fields. So, this check
> is not necessary.

Ah, OK.  Thanks.

The patch is OK with that minor fix then.


Diego.

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

I committed this patch with the fix.

Thanks,
-Sri.

On Tue, Dec 18, 2012 at 4:13 PM, Diego Novillo <dnovillo@google.com> wrote:
> On Tue, Dec 18, 2012 at 6:38 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>
>> The function versions are now determined purely based on the string
>> value and not on DECL_FUNCTION_SPECIFIC_TARGET fields. So, this check
>> is not necessary.
>
> Ah, OK.  Thanks.
>
> The patch is OK with that minor fix then.
>
>
> Diego.

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Andreas Schwab]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 148388d..575e03a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,4 +1,7 @@
-<<<<<<< .mine
+2012-12-27  Andreas Schwab  <schwab@linux-m68k.org>
+
+	* target.def (supports_function_versions): Fix typo.
+
 2012-12-26  Sriraman Tallam  <tmsriram@google.com>
 
 	* doc/tm.texi.in (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS): Document
@@ -15,12 +18,10 @@
 	* (is_function_default_version): Check target string.
 	* TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS: New macro.
 
-=======
 2012-12-27  Steven Bosscher  <steven@gcc.gnu.org>
 
 	* cgraph.c (verify_cgraph_node): Don't allocate/free visited_nodes set.
 
->>>>>>> .r194729
 2012-12-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
 
 	PR target/53789
diff --git a/gcc/target.def b/gcc/target.def
index 79bb955..d0547be 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -2839,7 +2839,7 @@ DEFHOOK
 (supports_function_versions,
  "",
  bool, (void),
- hool_bool_void_false)
+ hook_bool_void_false)
 
 /* Function to determine if one function can inline another function.  */
 #undef HOOK_PREFIX
-- 
1.8.0.2


-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

On Thu, Dec 27, 2012 at 2:05 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 148388d..575e03a 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,4 +1,7 @@
> -<<<<<<< .mine
> +2012-12-27  Andreas Schwab  <schwab@linux-m68k.org>
> +
> +       * target.def (supports_function_versions): Fix typo.
> +

>  2012-12-26  Sriraman Tallam  <tmsriram@google.com>
>
>         * doc/tm.texi.in (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS): Document
> @@ -15,12 +18,10 @@
>         * (is_function_default_version): Check target string.
>         * TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS: New macro.
>
> -=======
>  2012-12-27  Steven Bosscher  <steven@gcc.gnu.org>
>
>         * cgraph.c (verify_cgraph_node): Don't allocate/free visited_nodes set.
>
> ->>>>>>> .r194729
>  2012-12-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
>
>         PR target/53789
> diff --git a/gcc/target.def b/gcc/target.def
> index 79bb955..d0547be 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -2839,7 +2839,7 @@ DEFHOOK
>  (supports_function_versions,
>   "",
>   bool, (void),
> - hool_bool_void_false)
> + hook_bool_void_false)

Thanks for the fix.

-Sri.

>
>  /* Function to determine if one function can inline another function.  */
>  #undef HOOK_PREFIX
> --
> 1.8.0.2
>
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Alexander Ivchenko]

I'm sorry - I didn't notice that it works only for c++ frontend. It
works for me now!

thanks,
Alexander

2012/12/27 Sriraman Tallam <tmsriram@google.com>:
> On Thu, Dec 27, 2012 at 2:05 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index 148388d..575e03a 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,4 +1,7 @@
>> -<<<<<<< .mine
>> +2012-12-27  Andreas Schwab  <schwab@linux-m68k.org>
>> +
>> +       * target.def (supports_function_versions): Fix typo.
>> +
>
>>  2012-12-26  Sriraman Tallam  <tmsriram@google.com>
>>
>>         * doc/tm.texi.in (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS): Document
>> @@ -15,12 +18,10 @@
>>         * (is_function_default_version): Check target string.
>>         * TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS: New macro.
>>
>> -=======
>>  2012-12-27  Steven Bosscher  <steven@gcc.gnu.org>
>>
>>         * cgraph.c (verify_cgraph_node): Don't allocate/free visited_nodes set.
>>
>> ->>>>>>> .r194729
>>  2012-12-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
>>
>>         PR target/53789
>> diff --git a/gcc/target.def b/gcc/target.def
>> index 79bb955..d0547be 100644
>> --- a/gcc/target.def
>> +++ b/gcc/target.def
>> @@ -2839,7 +2839,7 @@ DEFHOOK
>>  (supports_function_versions,
>>   "",
>>   bool, (void),
>> - hool_bool_void_false)
>> + hook_bool_void_false)
>
> Thanks for the fix.
>
> -Sri.
>
>>
>>  /* Function to determine if one function can inline another function.  */
>>  #undef HOOK_PREFIX
>> --
>> 1.8.0.2
>>
>>
>> --
>> Andreas Schwab, schwab@linux-m68k.org
>> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
>> "And now for something completely different."

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

I committed this trivial patch to fix some corner case bugs in
Function Multiversioning.

        * config/i386/i386.c (ix86_get_function_versions_dispatcher): Fix bug
        in loop predicate.
        (fold_builtin_cpu): Do not share cpu model decls across statements.

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c  (revision 194817)
+++ config/i386/i386.c  (working copy)
@@ -29290,7 +29290,7 @@ ix86_get_function_versions_dispatcher (void *decl)

   /* Set the dispatcher for all the versions.  */
   it_v = default_version_info;
-  while (it_v->next != NULL)
+  while (it_v != NULL)
     {
       it_v->dispatcher_resolver = dispatch_decl;
       it_v = it_v->next;
@@ -29626,8 +29626,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
       {"avx2",   F_AVX2}
     };

-  static tree __processor_model_type = NULL_TREE;
-  static tree __cpu_model_var = NULL_TREE;
+  tree __processor_model_type = NULL_TREE;
+  tree __cpu_model_var = NULL_TREE;

   if (__processor_model_type == NULL_TREE)
     __processor_model_type = build_processor_model_struct ();



Thanks,
-Sri.

On Thu, Dec 27, 2012 at 2:05 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 148388d..575e03a 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,4 +1,7 @@
> -<<<<<<< .mine
> +2012-12-27  Andreas Schwab  <schwab@linux-m68k.org>
> +
> +       * target.def (supports_function_versions): Fix typo.
> +
>  2012-12-26  Sriraman Tallam  <tmsriram@google.com>
>
>         * doc/tm.texi.in (TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS): Document
> @@ -15,12 +18,10 @@
>         * (is_function_default_version): Check target string.
>         * TARGET_OPTION_SUPPORTS_FUNCTION_VERSIONS: New macro.
>
> -=======
>  2012-12-27  Steven Bosscher  <steven@gcc.gnu.org>
>
>         * cgraph.c (verify_cgraph_node): Don't allocate/free visited_nodes set.
>
> ->>>>>>> .r194729
>  2012-12-25  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
>
>         PR target/53789
> diff --git a/gcc/target.def b/gcc/target.def
> index 79bb955..d0547be 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -2839,7 +2839,7 @@ DEFHOOK
>  (supports_function_versions,
>   "",
>   bool, (void),
> - hool_bool_void_false)
> + hook_bool_void_false)
>
>  /* Function to determine if one function can inline another function.  */
>  #undef HOOK_PREFIX
> --
> 1.8.0.2
>
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Jakub Jelinek]

On Wed, Jan 02, 2013 at 12:23:45PM -0800, Sriraman Tallam wrote:
> --- config/i386/i386.c  (revision 194817)
> +++ config/i386/i386.c  (working copy)
> @@ -29290,7 +29290,7 @@ ix86_get_function_versions_dispatcher (void *decl)
> 
>    /* Set the dispatcher for all the versions.  */
>    it_v = default_version_info;
> -  while (it_v->next != NULL)
> +  while (it_v != NULL)
>      {
>        it_v->dispatcher_resolver = dispatch_decl;
>        it_v = it_v->next;
> @@ -29626,8 +29626,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
>        {"avx2",   F_AVX2}
>      };
> 
> -  static tree __processor_model_type = NULL_TREE;
> -  static tree __cpu_model_var = NULL_TREE;
> +  tree __processor_model_type = NULL_TREE;
> +  tree __cpu_model_var = NULL_TREE;
> 
>    if (__processor_model_type == NULL_TREE)
>      __processor_model_type = build_processor_model_struct ();

If __processor_model_type is always NULL_TREE above, then you should write
  tree __processor_model_type = build_processor_model_struct ();
rather than the extra if with an always true condition.

	Jakub

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

On Wed, Jan 2, 2013 at 1:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jan 02, 2013 at 12:23:45PM -0800, Sriraman Tallam wrote:
>> --- config/i386/i386.c  (revision 194817)
>> +++ config/i386/i386.c  (working copy)
>> @@ -29290,7 +29290,7 @@ ix86_get_function_versions_dispatcher (void *decl)
>>
>>    /* Set the dispatcher for all the versions.  */
>>    it_v = default_version_info;
>> -  while (it_v->next != NULL)
>> +  while (it_v != NULL)
>>      {
>>        it_v->dispatcher_resolver = dispatch_decl;
>>        it_v = it_v->next;
>> @@ -29626,8 +29626,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
>>        {"avx2",   F_AVX2}
>>      };
>>
>> -  static tree __processor_model_type = NULL_TREE;
>> -  static tree __cpu_model_var = NULL_TREE;
>> +  tree __processor_model_type = NULL_TREE;
>> +  tree __cpu_model_var = NULL_TREE;
>>
>>    if (__processor_model_type == NULL_TREE)
>>      __processor_model_type = build_processor_model_struct ();
>
> If __processor_model_type is always NULL_TREE above, then you should write
>   tree __processor_model_type = build_processor_model_struct ();
> rather than the extra if with an always true condition.

Right, sorry! Oversight in a hurry to fix the bug.

-Sri.

>
>         Jakub

Re: [PATCH] Function Multiversioning Bug, checking for function versions

[Sriraman Tallam]

On Wed, Jan 2, 2013 at 1:01 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jan 02, 2013 at 12:23:45PM -0800, Sriraman Tallam wrote:
>> --- config/i386/i386.c  (revision 194817)
>> +++ config/i386/i386.c  (working copy)
>> @@ -29290,7 +29290,7 @@ ix86_get_function_versions_dispatcher (void *decl)
>>
>>    /* Set the dispatcher for all the versions.  */
>>    it_v = default_version_info;
>> -  while (it_v->next != NULL)
>> +  while (it_v != NULL)
>>      {
>>        it_v->dispatcher_resolver = dispatch_decl;
>>        it_v = it_v->next;
>> @@ -29626,8 +29626,8 @@ fold_builtin_cpu (tree fndecl, tree *args)
>>        {"avx2",   F_AVX2}
>>      };
>>
>> -  static tree __processor_model_type = NULL_TREE;
>> -  static tree __cpu_model_var = NULL_TREE;
>> +  tree __processor_model_type = NULL_TREE;
>> +  tree __cpu_model_var = NULL_TREE;
>>
>>    if (__processor_model_type == NULL_TREE)
>>      __processor_model_type = build_processor_model_struct ();
>
> If __processor_model_type is always NULL_TREE above, then you should write
>   tree __processor_model_type = build_processor_model_struct ();
> rather than the extra if with an always true condition.

Submitted this patch to fix this.

        * config/i386/i386.c (fold_builtin_cpu): Remove unnecessary checks for
        NULL.

--- config/i386/i386.c (revision 194827)
+++ config/i386/i386.c (working copy)
@@ -29626,16 +29626,10 @@ fold_builtin_cpu (tree fndecl, tree *args)
       {"avx2",   F_AVX2}
     };

-  tree __processor_model_type = NULL_TREE;
-  tree __cpu_model_var = NULL_TREE;
+  tree __processor_model_type = build_processor_model_struct ();
+  tree __cpu_model_var = make_var_decl (__processor_model_type,
+                                                              "__cpu_model");

-  if (__processor_model_type == NULL_TREE)
-    __processor_model_type = build_processor_model_struct ();
-
-  if (__cpu_model_var == NULL_TREE)
-    __cpu_model_var = make_var_decl (__processor_model_type,
-                                                         "__cpu_model");
-
   gcc_assert ((args != NULL) && (*args != NULL));

   param_string_cst = *args;

Thanks,
-Sri.


>
>         Jakub