* [PATCH] Function Multiversioning Bug, checking for function versions @ 2012-11-15 22:08 Sriraman Tallam 2012-11-16 19:55 ` Sriraman Tallam 0 siblings, 1 reply; 13+ messages in thread From: Sriraman Tallam @ 2012-11-15 22:08 UTC (permalink / raw) To: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li [-- 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) { ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-11-15 22:08 [PATCH] Function Multiversioning Bug, checking for function versions Sriraman Tallam @ 2012-11-16 19:55 ` Sriraman Tallam 2012-12-18 16:05 ` Diego Novillo 2012-12-27 10:06 ` Andreas Schwab 0 siblings, 2 replies; 13+ messages in thread From: Sriraman Tallam @ 2012-11-16 19:55 UTC (permalink / raw) To: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li [-- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-11-16 19:55 ` Sriraman Tallam @ 2012-12-18 16:05 ` Diego Novillo 2012-12-18 23:38 ` Sriraman Tallam 2012-12-27 10:06 ` Andreas Schwab 1 sibling, 1 reply; 13+ messages in thread From: Diego Novillo @ 2012-12-18 16:05 UTC (permalink / raw) To: Sriraman Tallam; +Cc: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-12-18 16:05 ` Diego Novillo @ 2012-12-18 23:38 ` Sriraman Tallam 2012-12-19 0:13 ` Diego Novillo 0 siblings, 1 reply; 13+ messages in thread From: Sriraman Tallam @ 2012-12-18 23:38 UTC (permalink / raw) To: Diego Novillo; +Cc: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-12-18 23:38 ` Sriraman Tallam @ 2012-12-19 0:13 ` Diego Novillo 2012-12-27 1:58 ` Sriraman Tallam 0 siblings, 1 reply; 13+ messages in thread From: Diego Novillo @ 2012-12-19 0:13 UTC (permalink / raw) To: Sriraman Tallam; +Cc: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-12-19 0:13 ` Diego Novillo @ 2012-12-27 1:58 ` Sriraman Tallam 0 siblings, 0 replies; 13+ messages in thread From: Sriraman Tallam @ 2012-12-27 1:58 UTC (permalink / raw) To: Diego Novillo; +Cc: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-11-16 19:55 ` Sriraman Tallam 2012-12-18 16:05 ` Diego Novillo @ 2012-12-27 10:06 ` Andreas Schwab 2012-12-27 19:17 ` Sriraman Tallam 2013-01-02 20:23 ` Sriraman Tallam 1 sibling, 2 replies; 13+ messages in thread From: Andreas Schwab @ 2012-12-27 10:06 UTC (permalink / raw) To: Sriraman Tallam; +Cc: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-12-27 10:06 ` Andreas Schwab @ 2012-12-27 19:17 ` Sriraman Tallam 2012-12-28 7:31 ` Alexander Ivchenko 2013-01-02 20:23 ` Sriraman Tallam 1 sibling, 1 reply; 13+ messages in thread From: Sriraman Tallam @ 2012-12-27 19:17 UTC (permalink / raw) To: Andreas Schwab; +Cc: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-12-27 19:17 ` Sriraman Tallam @ 2012-12-28 7:31 ` Alexander Ivchenko 0 siblings, 0 replies; 13+ messages in thread From: Alexander Ivchenko @ 2012-12-28 7:31 UTC (permalink / raw) To: Sriraman Tallam Cc: Andreas Schwab, GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2012-12-27 10:06 ` Andreas Schwab 2012-12-27 19:17 ` Sriraman Tallam @ 2013-01-02 20:23 ` Sriraman Tallam 2013-01-02 21:01 ` Jakub Jelinek 1 sibling, 1 reply; 13+ messages in thread From: Sriraman Tallam @ 2013-01-02 20:23 UTC (permalink / raw) To: Andreas Schwab; +Cc: GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2013-01-02 20:23 ` Sriraman Tallam @ 2013-01-02 21:01 ` Jakub Jelinek 2013-01-02 21:48 ` Sriraman Tallam 2013-01-03 1:00 ` Sriraman Tallam 0 siblings, 2 replies; 13+ messages in thread From: Jakub Jelinek @ 2013-01-02 21:01 UTC (permalink / raw) To: Sriraman Tallam Cc: Andreas Schwab, GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2013-01-02 21:01 ` Jakub Jelinek @ 2013-01-02 21:48 ` Sriraman Tallam 2013-01-03 1:00 ` Sriraman Tallam 1 sibling, 0 replies; 13+ messages in thread From: Sriraman Tallam @ 2013-01-02 21:48 UTC (permalink / raw) To: Jakub Jelinek Cc: Andreas Schwab, GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Function Multiversioning Bug, checking for function versions 2013-01-02 21:01 ` Jakub Jelinek 2013-01-02 21:48 ` Sriraman Tallam @ 2013-01-03 1:00 ` Sriraman Tallam 1 sibling, 0 replies; 13+ messages in thread From: Sriraman Tallam @ 2013-01-03 1:00 UTC (permalink / raw) To: Jakub Jelinek Cc: Andreas Schwab, GCC Patches, H.J. Lu, Zamyatin, Igor, David Li 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-01-03 1:00 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-15 22:08 [PATCH] Function Multiversioning Bug, checking for function versions Sriraman Tallam 2012-11-16 19:55 ` Sriraman Tallam 2012-12-18 16:05 ` Diego Novillo 2012-12-18 23:38 ` Sriraman Tallam 2012-12-19 0:13 ` Diego Novillo 2012-12-27 1:58 ` Sriraman Tallam 2012-12-27 10:06 ` Andreas Schwab 2012-12-27 19:17 ` Sriraman Tallam 2012-12-28 7:31 ` Alexander Ivchenko 2013-01-02 20:23 ` Sriraman Tallam 2013-01-02 21:01 ` Jakub Jelinek 2013-01-02 21:48 ` Sriraman Tallam 2013-01-03 1:00 ` Sriraman Tallam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).