Fix multi-versioning issues (PR ipa/80732).

Hello.

Following patch tries to resolve following 2 issues:

a) When one takes address of a function that uses target_clones attribute,
   default implementation is always returned.

b) Using dlsym("foo") should work and thus the resolver function should
   use the default name. Because of that, default implementation must be
   renamed.

Unfortunately, we currently do not support redirection of ipa_refs, thus
walk_tree is needed to resolve that. Hopefully there should not be any
different IPA_REF that needs to be handled.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Martin

PING^1

On 05/25/2017 12:05 PM, Martin Liška wrote:
> Hello.
> 
> Following patch tries to resolve following 2 issues:
> 
> a) When one takes address of a function that uses target_clones attribute,
>     default implementation is always returned.
> 
> b) Using dlsym("foo") should work and thus the resolver function should
>     use the default name. Because of that, default implementation must be
>     renamed.
> 
> Unfortunately, we currently do not support redirection of ipa_refs, thus
> walk_tree is needed to resolve that. Hopefully there should not be any
> different IPA_REF that needs to be handled.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Martin
>

PING^2

On 06/06/2017 08:59 AM, Martin Liška wrote:
> PING^1
> 
> On 05/25/2017 12:05 PM, Martin Liška wrote:
>> Hello.
>>
>> Following patch tries to resolve following 2 issues:
>>
>> a) When one takes address of a function that uses target_clones attribute,
>>     default implementation is always returned.
>>
>> b) Using dlsym("foo") should work and thus the resolver function should
>>     use the default name. Because of that, default implementation must be
>>     renamed.
>>
>> Unfortunately, we currently do not support redirection of ipa_refs, thus
>> walk_tree is needed to resolve that. Hopefully there should not be any
>> different IPA_REF that needs to be handled.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
>> Martin
>>
>

> Hello.
> 
> Following patch tries to resolve following 2 issues:
> 
> a) When one takes address of a function that uses target_clones attribute,
>    default implementation is always returned.
> 
> b) Using dlsym("foo") should work and thus the resolver function should
>    use the default name. Because of that, default implementation must be
>    renamed.
> 
> Unfortunately, we currently do not support redirection of ipa_refs, thus
> walk_tree is needed to resolve that. Hopefully there should not be any
> different IPA_REF that needs to be handled.

The cgraph interface for redirection is meant mostly for full IPA passes
that can not touch bodies directly, in this case I think it is fine to
walk all references.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Martin

> >From 198c8464978c21cd68d4743de5648ecfefd2e09c Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Wed, 17 May 2017 15:56:22 +0200
> Subject: [PATCH] Fix multi-versioning issues (PR ipa/80732).
> 
> gcc/ChangeLog:
> 
> 2017-05-19  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/80732
> 	* attribs.c (make_dispatcher_decl): Do not append '.ifunc'
> 	to dispatcher function name.
> 	* multiple_target.c (replace_function_decl): New function.
> 	(create_dispatcher_calls): Redirect both edges and references.
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-05-19  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/80732
> 	* gcc.target/i386/mvc5.c: Scan indirect_function.
> 	* gcc.target/i386/mvc7.c: Likewise.
> 	* gcc.target/i386/pr80732.c: New test.
> ---
>  gcc/attribs.c                           |   6 +-
>  gcc/multiple_target.c                   | 105 ++++++++++++++++++++++----------
>  gcc/testsuite/gcc.target/i386/mvc5.c    |   2 +-
>  gcc/testsuite/gcc.target/i386/mvc7.c    |   2 +-
>  gcc/testsuite/gcc.target/i386/pr80732.c |  85 ++++++++++++++++++++++++++
>  5 files changed, 161 insertions(+), 39 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr80732.c
> 
> diff --git a/gcc/attribs.c b/gcc/attribs.c
> index 4ba0eab8899..5eb19e82795 100644
> --- a/gcc/attribs.c
> +++ b/gcc/attribs.c
> @@ -888,12 +888,8 @@ make_dispatcher_decl (const tree decl)
>    tree func_decl;
>    char *func_name;
>    tree fn_type, func_type;
> -  bool is_uniq = false;
>  
> -  if (TREE_PUBLIC (decl) == 0)
> -    is_uniq = true;
> -
> -  func_name = make_unique_name (decl, "ifunc", is_uniq);
> +  func_name = xstrdup (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)));
>  
>    fn_type = TREE_TYPE (decl);
>    func_type = build_function_type (TREE_TYPE (fn_type),
> diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
> index 2ee6a9591ba..fba2636ba16 100644
> --- a/gcc/multiple_target.c
> +++ b/gcc/multiple_target.c
> @@ -34,6 +34,27 @@ along with GCC; see the file COPYING3.  If not see
>  #include "target.h"
>  #include "attribs.h"
>  #include "pretty-print.h"
> +#include "gimple-iterator.h"
> +#include "gimple-walk.h"
> +
> +/* Walker callback that replaces all FUNCTION_DECL of a function that's
> +   going to be versioned.  */
> +
> +static tree
> +replace_function_decl (tree *op, int *walk_subtrees, void *data)
> +{
> +  struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
> +  cgraph_function_version_info *info = (cgraph_function_version_info *)wi->info;
> +
> +  if (TREE_CODE (*op) == FUNCTION_DECL
> +      && info->this_node->decl == *op)
> +    {
> +      *op = info->dispatcher_resolver;
> +      *walk_subtrees = 0;
> +    }
> +
> +  return NULL;
> +}
>  
>  /* If the call in NODE has multiple target attribute with multiple fields,
>     replace it with dispatcher call and create dispatcher (once).  */
> @@ -41,51 +62,48 @@ along with GCC; see the file COPYING3.  If not see
>  static void
>  create_dispatcher_calls (struct cgraph_node *node)
>  {
> -  cgraph_edge *e;
> -  cgraph_edge *e_next = NULL;
> +  ipa_ref *ref;
> +
> +  if (!DECL_FUNCTION_VERSIONED (node->decl))
> +    return;
> +
> +  auto_vec<cgraph_edge *> edges_to_redirect;
> +  auto_vec<ipa_ref *> references_to_redirect;
> +
> +  for (unsigned i = 0; node->iterate_referring (i, ref); i++)
> +    references_to_redirect.safe_push (ref);
>  
>    /* We need to remember NEXT_CALLER as it could be modified in the loop.  */
> -  for (e = node->callers; e ;e = (e == NULL) ? e_next : e->next_caller)
> -    {
> -      tree resolver_decl;
> -      tree idecl;
> -      tree decl;
> -      gimple *call = e->call_stmt;
> -      struct cgraph_node *inode;
> -
> -      /* Checking if call of function is call of versioned function.
> -	 Versioned function are not inlined, so there is no need to
> -	 check for inline.  */
> -      if (!call
> -	  || !(decl = gimple_call_fndecl (call))
> -	  || !DECL_FUNCTION_VERSIONED (decl))
> -	continue;
> +  for (cgraph_edge *e = node->callers; e ; e = e->next_caller)
> +    edges_to_redirect.safe_push (e);
>  
> +  if (!edges_to_redirect.is_empty () || !references_to_redirect.is_empty ())
> +    {
>        if (!targetm.has_ifunc_p ())
>  	{
> -	  error_at (gimple_location (call),
> +	  error_at (DECL_SOURCE_LOCATION (node->decl),
>  		    "the call requires ifunc, which is not"
>  		    " supported by this target");
> -	  break;
> +	  return;
>  	}
>        else if (!targetm.get_function_versions_dispatcher)
>  	{
> -	  error_at (gimple_location (call),
> +	  error_at (DECL_SOURCE_LOCATION (node->decl),
>  		    "target does not support function version dispatcher");
> -	  break;
> +	  return;
>  	}
>  
> -      e_next = e->next_caller;
> -      idecl = targetm.get_function_versions_dispatcher (decl);
> +      tree idecl = targetm.get_function_versions_dispatcher (node->decl);
>        if (!idecl)
>  	{
> -	  error_at (gimple_location (call),
> +	  error_at (DECL_SOURCE_LOCATION (node->decl),
>  		    "default target_clones attribute was not set");
> -	  break;
> +	  return;
>  	}
> -      inode = cgraph_node::get (idecl);
> +
> +      cgraph_node *inode = cgraph_node::get (idecl);
>        gcc_assert (inode);
> -      resolver_decl = targetm.generate_version_dispatcher_body (inode);
> +      tree resolver_decl = targetm.generate_version_dispatcher_body (inode);
>  
>        /* Update aliases.  */
>        inode->alias = true;
> @@ -93,12 +111,35 @@ create_dispatcher_calls (struct cgraph_node *node)
>        if (!inode->analyzed)
>  	inode->resolve_alias (cgraph_node::get (resolver_decl));
>  
> -      e->redirect_callee (inode);
> -      e->redirect_call_stmt_to_callee ();
> -      /*  Since REDIRECT_CALLEE modifies NEXT_CALLER field we move to
> -	  previously set NEXT_CALLER.  */
> -      e = NULL;
> +      /* Redirect edges.  */
> +      unsigned i;
> +      cgraph_edge *e;
> +      FOR_EACH_VEC_ELT (edges_to_redirect, i, e)
> +	{
> +	  e->redirect_callee (inode);
> +	  e->redirect_call_stmt_to_callee ();
> +	}
> +
> +      /* Redirect references.  */
> +      FOR_EACH_VEC_ELT (references_to_redirect, i, ref)
> +	{
> +	  if (ref->use == IPA_REF_ADDR)
> +	    {
> +	      struct walk_stmt_info wi;
> +	      memset (&wi, 0, sizeof (wi));
> +	      wi.info = (void *)node->function_version ();
> +	      gimple_stmt_iterator it = gsi_for_stmt (ref->stmt);
> +	      if (ref->referring->decl != resolver_decl)
> +		walk_gimple_stmt (&it, NULL, replace_function_decl, &wi);
> +	    }
> +	  else
> +	    gcc_unreachable ();

Don't you need to handle static initializers here as well?

OK with this change or explanation why you don't :)
Thanks,
Honza
> +	}
>      }
> +
> +  symtab->change_decl_assembler_name (node->decl,
> +				      clone_function_name (node->decl,
> +							   "default"));
>  }
>  
>  /* Return length of attribute names string,
> diff --git a/gcc/testsuite/gcc.target/i386/mvc5.c b/gcc/testsuite/gcc.target/i386/mvc5.c
> index 8fea04c792b..677f79f3fd0 100644
> --- a/gcc/testsuite/gcc.target/i386/mvc5.c
> +++ b/gcc/testsuite/gcc.target/i386/mvc5.c
> @@ -1,7 +1,7 @@
>  /* { dg-do compile } */
>  /* { dg-require-ifunc "" } */
>  /* { dg-options "-fno-inline" } */
> -/* { dg-final { scan-assembler-times "foo.ifunc" 6 } } */
> +/* { dg-final { scan-assembler "foo,foo.resolver" } } */
>  
>  __attribute__((target_clones("default","avx","avx2")))
>  int
> diff --git a/gcc/testsuite/gcc.target/i386/mvc7.c b/gcc/testsuite/gcc.target/i386/mvc7.c
> index 9a9d7a3da30..a3697ba9b75 100644
> --- a/gcc/testsuite/gcc.target/i386/mvc7.c
> +++ b/gcc/testsuite/gcc.target/i386/mvc7.c
> @@ -3,7 +3,7 @@
>  /* { dg-final { scan-assembler "foo.resolver" } } */
>  /* { dg-final { scan-assembler "avx" } } */
>  /* { dg-final { scan-assembler "slm" } } */
> -/* { dg-final { scan-assembler-times "foo.ifunc" 4 } } */
> +/* { dg-final { scan-assembler "foo,foo.resolver" } } */
>  
>  __attribute__((target_clones("avx","default","arch=slm","arch=core-avx2")))
>  int foo ();
> diff --git a/gcc/testsuite/gcc.target/i386/pr80732.c b/gcc/testsuite/gcc.target/i386/pr80732.c
> new file mode 100644
> index 00000000000..fb168260eaa
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr80732.c
> @@ -0,0 +1,85 @@
> +/* PR ipa/80732 */
> +/* { dg-do run } */
> +/* { dg-options "-ldl -fPIC -rdynamic -O3 -g" } */
> +/* { dg-require-ifunc "" } */
> +/* { dg-require-effective-target fma4 } */
> +
> +
> +#include <dlfcn.h>
> +
> +__attribute__((target_clones("default","fma"),noinline,optimize("fast-math")))
> +double f1(double a, double b, double c)
> +{
> +    return a * b + c;
> +}
> +
> +double k1(double a, double b, double c, void **p)
> +{
> +    *p = f1;
> +    return f1(a, b, c);
> +}
> +
> +__attribute__((target("fma"),optimize("fast-math")))
> +static double f2_fma(double a, double b, double c)
> +{
> +    return a * b + c;
> +}
> +
> +__attribute__((optimize("fast-math")))
> +static double f2_default(double a, double b, double c)
> +{
> +    return a * b + c;
> +}
> +
> +static void *f2_resolve(void)
> +{
> +    __builtin_cpu_init ();
> +    if (__builtin_cpu_supports("fma"))
> +        return f2_fma;
> +    else
> +        return f2_default;
> +}
> +
> +double f2(double a, double b, double c) __attribute__((ifunc("f2_resolve")));
> +
> +double k2(double a, double b, double c, void **p)
> +{
> +    *p = f2;
> +    return f2(a, b, c);
> +}
> +
> +int main()
> +{
> +    char buffer[256];
> +    const char *expectation = "4.93038e-32, 4.93038e-32, 4.93038e-32";
> +
> +    volatile double a = 1.0000000000000002;
> +    volatile double b = -0.9999999999999998;
> +    volatile double c = 1.0;
> +
> +    void *hdl = dlopen(0, RTLD_NOW);
> +
> +    double (*pf1)(double, double, double) = dlsym(hdl, "f1");
> +    double (*pk1)(double, double, double, void**) = dlsym(hdl, "k1");
> +    double (*_pf1)(double, double, double);
> +
> +    double v1_1 = pf1(a, b, c);
> +    double v1_2 = pk1(a, b, c, (void**)&_pf1);
> +    double v1_3 = _pf1(a, b, c);
> +    __builtin_sprintf (buffer, "%g, %g, %g", v1_1, v1_2, v1_3);
> +    if (__builtin_strcmp (buffer, expectation) != 0)
> +      __builtin_abort ();
> +
> +    double (*pf2)(double, double, double) = dlsym(hdl, "f2");
> +    double (*pk2)(double, double, double, void**) = dlsym(hdl, "k2");
> +    double (*_pf2)(double, double, double);
> +
> +    double v2_1 = pf2(a, b, c);
> +    double v2_2 = pk2(a, b, c, (void**)&_pf2);
> +    double v2_3 = _pf2(a, b, c);
> +    __builtin_sprintf(buffer, "%g, %g, %g", v2_1, v2_2, v2_3);
> +    if (__builtin_strcmp (buffer, expectation) != 0)
> +      __builtin_abort ();
> +
> +    return 0;
> +}
> -- 
> 2.12.2
>