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

[Diego Novillo <dnovillo at google dot com>]

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.