Patchwork Function Multiversioning Bug, checking for function versions

login
register
mail settings
Submitter Sriraman Tallam
Date Nov. 16, 2012, 7:55 p.m.
Message ID <CAAs8HmxZb_5ZYuwpPfwJ0DRDOnmRuGVj=h1SBBU9Pv-uF7P1Ng@mail.gmail.com>
Download mbox | patch
Permalink /patch/199729/
State New
Headers show

Comments

Sriraman Tallam - Nov. 16, 2012, 7:55 p.m.
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.
Diego Novillo - Dec. 18, 2012, 4:04 p.m.
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.
Sriraman Tallam - Dec. 18, 2012, 11:38 p.m.
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.
Diego Novillo - Dec. 19, 2012, 12:13 a.m.
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.
Sriraman Tallam - Dec. 27, 2012, 1:58 a.m.
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.

Patch

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