[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.