Hi Diego, Thanks for the review.
On Tue, Dec 18, 2012 at 8:04 AM, Diego Novillo <dnovi...@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 <tmsri...@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.