Re: [PATCH] Fix multi-versioning issues (PR ipa/80732).

[Jan Hubicka <hubicka@ucw.cz>]

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