[PATCH], PowerPC target_clones minor support

[PATCH], PowerPC target_clones minor support

[Michael Meissner]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

Some minor changes to the PowerPC target_clones support:

1) I added a warning if target_clones was used and the compiler whas configured
with an older glibc where __builtin_cpu_supports always returns 0;

2) I reworked how the ifunc resolver function is generated, and always made it
a static function;

3) I added an executable target_clones test, and I made both clone tests
dependent on GCC being configured with a new glibc.

I have done a full bootstrap and make check test on a little endian power8
system, and there were no regressions.  I did the bootstrap using the Advance
Toolchain 10.0-3 libraries, and verified that the 2 clone tests were run.
Are these patches ok to apply to the trunk?

[gcc]
2017-06-28  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* config/rs6000/rs6000.c
	(rs6000_get_function_versions_dispatcher): Add warning if the
	compiler is not configured to use at least GLIBC version 2.23.
	(make_resolver_func): Make resolver function private and not a
	COMDAT function.  Create the name with clone_function_name instead
	of make_unique_name.

[gcc/testsuite]
2017-06-28  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* gcc.target/powerpc/clone2.c: New runtime test for
	target_clones.
	* gcc.target/powerpc/clone1.c: Add check to make sure the
	__builtin_cpu_supports function is fully supported.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797

[-- Attachment #2: clone.patch13b --]
[-- Type: text/plain, Size: 5050 bytes --]

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(.../svn+ssh://meissner@gcc.gnu.org/svn/gcc/trunk/gcc/config/rs6000)	(revision 249737)
+++ gcc/config/rs6000/rs6000.c	(.../gcc/config/rs6000)	(working copy)
@@ -37266,6 +37266,11 @@ rs6000_get_function_versions_dispatcher 
 
   default_node = default_version_info->this_node;
 
+#ifndef TARGET_LIBC_PROVIDES_HWCAP_IN_TCB
+  warning_at (DECL_SOURCE_LOCATION (default_node->decl), 0,
+	      "target_clone needs at least glibc 2.23");
+#endif
+
   if (targetm.has_ifunc_p ())
     {
       struct cgraph_function_version_info *it_v = NULL;
@@ -37311,29 +37316,24 @@ make_resolver_func (const tree default_d
 		    const tree dispatch_decl,
 		    basic_block *empty_bb)
 {
-  /* IFUNC's have to be globally visible.  So, if the default_decl is
-     not, then the name of the IFUNC should be made unique.  */
-  bool is_uniq = (TREE_PUBLIC (default_decl) == 0);
-
   /* Append the filename to the resolver function if the versions are
      not externally visible.  This is because the resolver function has
      to be externally visible for the loader to find it.  So, appending
      the filename will prevent conflicts with a resolver function from
      another module which is based on the same version name.  */
-  char *resolver_name = make_unique_name (default_decl, "resolver", is_uniq);
+  tree decl_name = clone_function_name (default_decl, "resolver");
+  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
 
   /* The resolver function should return a (void *).  */
   tree type = build_function_type_list (ptr_type_node, NULL_TREE);
   tree decl = build_fn_decl (resolver_name, type);
-  tree decl_name = get_identifier (resolver_name);
   SET_DECL_ASSEMBLER_NAME (decl, decl_name);
 
   DECL_NAME (decl) = decl_name;
   TREE_USED (decl) = 1;
   DECL_ARTIFICIAL (decl) = 1;
   DECL_IGNORED_P (decl) = 0;
-  /* IFUNC resolvers have to be externally visible.  */
-  TREE_PUBLIC (decl) = 1;
+  TREE_PUBLIC (decl) = 0;
   DECL_UNINLINABLE (decl) = 1;
 
   /* Resolver is not external, body is generated.  */
@@ -37344,15 +37344,6 @@ make_resolver_func (const tree default_d
   DECL_INITIAL (decl) = make_node (BLOCK);
   DECL_STATIC_CONSTRUCTOR (decl) = 0;
 
-  if (DECL_COMDAT_GROUP (default_decl) || TREE_PUBLIC (default_decl))
-    {
-      /* In this case, each translation unit with a call to this
-	 versioned function will put out a resolver.  Ensure it
-	 is comdat to keep just one copy.  */
-      DECL_COMDAT (decl) = 1;
-      make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
-    }
-
   /* Build result decl and add to function_decl.  */
   tree t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_ARTIFICIAL (t) = 1;
@@ -37374,7 +37365,7 @@ make_resolver_func (const tree default_d
     = make_attribute ("ifunc", resolver_name, DECL_ATTRIBUTES (dispatch_decl));
 
   cgraph_node::create_same_body_alias (dispatch_decl, decl);
-  XDELETEVEC (resolver_name);
+
   return decl;
 }
 
Index: gcc/testsuite/gcc.target/powerpc/clone2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/clone2.c	(.../svn+ssh://meissner@gcc.gnu.org/svn/gcc/trunk/gcc/testsuite/gcc.target/powerpc)	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/clone2.c	(.../gcc/testsuite/gcc.target/powerpc)	(revision 249738)
@@ -0,0 +1,31 @@
+/* { dg-do run { target { powerpc*-*-linux* } } } */
+/* { dg-options "-mvsx -O2" } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-require-effective-target ppc_cpu_supports_hw } */
+
+#include <stddef.h>
+#include <stdlib.h>
+
+/* Power9 (aka, ISA 3.0) has a MODSD instruction to do modulus, while Power8
+   (aka, ISA 2.07) has to do modulus with divide and multiply.  Make sure that
+   the basic support for target_clones runs.
+
+   Restrict ourselves to Linux, since IFUNC might not be supported in other
+   operating systems.  */
+
+__attribute__((__target_clones__("cpu=power9,default")))
+long mod_func (long a, long b)
+{
+  return a % b;
+}
+
+#define X 53L
+#define Y 7L
+int
+main (void)
+{
+  if (mod_func (X, Y) != (X % Y))
+    abort ();
+
+  return 0;
+}
Index: gcc/testsuite/gcc.target/powerpc/clone1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/clone1.c	(.../svn+ssh://meissner@gcc.gnu.org/svn/gcc/trunk/gcc/testsuite/gcc.target/powerpc)	(revision 249737)
+++ gcc/testsuite/gcc.target/powerpc/clone1.c	(.../gcc/testsuite/gcc.target/powerpc)	(working copy)
@@ -2,6 +2,7 @@
 /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
 /* { dg-options "-mcpu=power8 -O2" } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-require-effective-target ppc_cpu_supports_hw } */
 
 /* Power9 (aka, ISA 3.0) has a MODSD instruction to do modulus, while Power8
    (aka, ISA 2.07) has to do modulus with divide and multiply.  Make sure

Re: [PATCH], PowerPC target_clones minor support

[Segher Boessenkool]

On Wed, Jun 28, 2017 at 02:28:23PM -0400, Michael Meissner wrote:
> Some minor changes to the PowerPC target_clones support:
> 
> 1) I added a warning if target_clones was used and the compiler whas configured
> with an older glibc where __builtin_cpu_supports always returns 0;
> 
> 2) I reworked how the ifunc resolver function is generated, and always made it
> a static function;
> 
> 3) I added an executable target_clones test, and I made both clone tests
> dependent on GCC being configured with a new glibc.

> 	* config/rs6000/rs6000.c
> 	(rs6000_get_function_versions_dispatcher): Add warning if the
> 	compiler is not configured to use at least GLIBC version 2.23.

Please say what is really tested for here (namely,
TARGET_LIBC_PROVIDES_HWCAP_IN_TCB).

>    /* Append the filename to the resolver function if the versions are
>       not externally visible.  This is because the resolver function has
>       to be externally visible for the loader to find it.  So, appending
>       the filename will prevent conflicts with a resolver function from
>       another module which is based on the same version name.  */
> -  char *resolver_name = make_unique_name (default_decl, "resolver", is_uniq);
> +  tree decl_name = clone_function_name (default_decl, "resolver");
> +  const char *resolver_name = IDENTIFIER_POINTER (decl_name);

I think the comment needs some updating now?

> --- gcc/testsuite/gcc.target/powerpc/clone2.c	(.../svn+ssh://meissner@gcc.gnu.org/svn/gcc/trunk/gcc/testsuite/gcc.target/powerpc)	(revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/clone2.c	(.../gcc/testsuite/gcc.target/powerpc)	(revision 249738)
> @@ -0,0 +1,31 @@
> +/* { dg-do run { target { powerpc*-*-linux* } } } */
> +/* { dg-options "-mvsx -O2" } */
> +/* { dg-require-effective-target powerpc_p9vector_ok } */
> +/* { dg-require-effective-target ppc_cpu_supports_hw } */

What a funny name (it reads as "the CPU supports the hardware").  Yes
I'm easily amused ;-)

The patch is okay for trunk modulo with those things looked at.  Sorry
for the slow review.


Segher

Re: [PATCH], PowerPC target_clones minor support

[Michael Meissner]

[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]

On Fri, Jul 07, 2017 at 07:22:04AM -0500, Segher Boessenkool wrote:
> On Wed, Jun 28, 2017 at 02:28:23PM -0400, Michael Meissner wrote:
> > Some minor changes to the PowerPC target_clones support:
> > 
> > 1) I added a warning if target_clones was used and the compiler whas configured
> > with an older glibc where __builtin_cpu_supports always returns 0;
> > 
> > 2) I reworked how the ifunc resolver function is generated, and always made it
> > a static function;
> > 
> > 3) I added an executable target_clones test, and I made both clone tests
> > dependent on GCC being configured with a new glibc.
> 
> > 	* config/rs6000/rs6000.c
> > 	(rs6000_get_function_versions_dispatcher): Add warning if the
> > 	compiler is not configured to use at least GLIBC version 2.23.
> 
> Please say what is really tested for here (namely,
> TARGET_LIBC_PROVIDES_HWCAP_IN_TCB).

I've reworded both the warning message and the ChangeLog entry.
 
> >    /* Append the filename to the resolver function if the versions are
> >       not externally visible.  This is because the resolver function has
> >       to be externally visible for the loader to find it.  So, appending
> >       the filename will prevent conflicts with a resolver function from
> >       another module which is based on the same version name.  */
> > -  char *resolver_name = make_unique_name (default_decl, "resolver", is_uniq);
> > +  tree decl_name = clone_function_name (default_decl, "resolver");
> > +  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
> 
> I think the comment needs some updating now?

Yes.

> > --- gcc/testsuite/gcc.target/powerpc/clone2.c	(.../svn+ssh://meissner@gcc.gnu.org/svn/gcc/trunk/gcc/testsuite/gcc.target/powerpc)	(revision 0)
> > +++ gcc/testsuite/gcc.target/powerpc/clone2.c	(.../gcc/testsuite/gcc.target/powerpc)	(revision 249738)
> > @@ -0,0 +1,31 @@
> > +/* { dg-do run { target { powerpc*-*-linux* } } } */
> > +/* { dg-options "-mvsx -O2" } */
> > +/* { dg-require-effective-target powerpc_p9vector_ok } */
> > +/* { dg-require-effective-target ppc_cpu_supports_hw } */
> 
> What a funny name (it reads as "the CPU supports the hardware").  Yes
> I'm easily amused ;-)
> 
> The patch is okay for trunk modulo with those things looked at.  Sorry
> for the slow review.

Here is the patch I committed:

[gcc]
2017-07-07  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* config/rs6000/rs6000.c (rs6000_get_function_versions_dispatcher):
	Add warning if GCC was not configured to link against a GLIBC that
	exports the hardware capability bits.
	(make_resolver_func): Make resolver function private and not a
	COMDAT function.  Create the name with clone_function_name instead
	of make_unique_name.

[gcc/testsuite]
2017-07-07  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* gcc.target/powerpc/clone1.c: Add check to make sure the
	__builtin_cpu_supports function is fully supported.
	* gcc.target/powerpc/clone2.c: New runtime test for
	target_clones.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797

[-- Attachment #2: clone.patch16b --]
[-- Type: text/plain, Size: 4845 bytes --]

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 250054)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -37283,6 +37283,12 @@ rs6000_get_function_versions_dispatcher 
 
   default_node = default_version_info->this_node;
 
+#ifndef TARGET_LIBC_PROVIDES_HWCAP_IN_TCB
+  warning_at (DECL_SOURCE_LOCATION (default_node->decl), 0,
+	      "target_clone needs GLIBC (2.23 and newer) to export hardware "
+	      "capability bits");
+#endif
+
   if (targetm.has_ifunc_p ())
     {
       struct cgraph_function_version_info *it_v = NULL;
@@ -37328,29 +37334,19 @@ make_resolver_func (const tree default_d
 		    const tree dispatch_decl,
 		    basic_block *empty_bb)
 {
-  /* IFUNC's have to be globally visible.  So, if the default_decl is
-     not, then the name of the IFUNC should be made unique.  */
-  bool is_uniq = (TREE_PUBLIC (default_decl) == 0);
-
-  /* Append the filename to the resolver function if the versions are
-     not externally visible.  This is because the resolver function has
-     to be externally visible for the loader to find it.  So, appending
-     the filename will prevent conflicts with a resolver function from
-     another module which is based on the same version name.  */
-  char *resolver_name = make_unique_name (default_decl, "resolver", is_uniq);
-
-  /* The resolver function should return a (void *).  */
+  /* Make the resolver function static.  The resolver function returns
+     void *.  */
+  tree decl_name = clone_function_name (default_decl, "resolver");
+  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
   tree type = build_function_type_list (ptr_type_node, NULL_TREE);
   tree decl = build_fn_decl (resolver_name, type);
-  tree decl_name = get_identifier (resolver_name);
   SET_DECL_ASSEMBLER_NAME (decl, decl_name);
 
   DECL_NAME (decl) = decl_name;
   TREE_USED (decl) = 1;
   DECL_ARTIFICIAL (decl) = 1;
   DECL_IGNORED_P (decl) = 0;
-  /* IFUNC resolvers have to be externally visible.  */
-  TREE_PUBLIC (decl) = 1;
+  TREE_PUBLIC (decl) = 0;
   DECL_UNINLINABLE (decl) = 1;
 
   /* Resolver is not external, body is generated.  */
@@ -37361,15 +37357,6 @@ make_resolver_func (const tree default_d
   DECL_INITIAL (decl) = make_node (BLOCK);
   DECL_STATIC_CONSTRUCTOR (decl) = 0;
 
-  if (DECL_COMDAT_GROUP (default_decl) || TREE_PUBLIC (default_decl))
-    {
-      /* In this case, each translation unit with a call to this
-	 versioned function will put out a resolver.  Ensure it
-	 is comdat to keep just one copy.  */
-      DECL_COMDAT (decl) = 1;
-      make_decl_one_only (decl, DECL_ASSEMBLER_NAME (decl));
-    }
-
   /* Build result decl and add to function_decl.  */
   tree t = build_decl (UNKNOWN_LOCATION, RESULT_DECL, NULL_TREE, ptr_type_node);
   DECL_ARTIFICIAL (t) = 1;
@@ -37391,7 +37378,7 @@ make_resolver_func (const tree default_d
     = make_attribute ("ifunc", resolver_name, DECL_ATTRIBUTES (dispatch_decl));
 
   cgraph_node::create_same_body_alias (dispatch_decl, decl);
-  XDELETEVEC (resolver_name);
+
   return decl;
 }
 
Index: gcc/testsuite/gcc.target/powerpc/clone1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/clone1.c	(revision 250054)
+++ gcc/testsuite/gcc.target/powerpc/clone1.c	(working copy)
@@ -2,6 +2,7 @@
 /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
 /* { dg-options "-mcpu=power8 -O2" } */
 /* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-require-effective-target ppc_cpu_supports_hw } */
 
 /* Power9 (aka, ISA 3.0) has a MODSD instruction to do modulus, while Power8
    (aka, ISA 2.07) has to do modulus with divide and multiply.  Make sure
Index: gcc/testsuite/gcc.target/powerpc/clone2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/clone2.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/clone2.c	(revision 0)
@@ -0,0 +1,31 @@
+/* { dg-do run { target { powerpc*-*-linux* } } } */
+/* { dg-options "-mvsx -O2" } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-require-effective-target ppc_cpu_supports_hw } */
+
+#include <stddef.h>
+#include <stdlib.h>
+
+/* Power9 (aka, ISA 3.0) has a MODSD instruction to do modulus, while Power8
+   (aka, ISA 2.07) has to do modulus with divide and multiply.  Make sure that
+   the basic support for target_clones runs.
+
+   Restrict ourselves to Linux, since IFUNC might not be supported in other
+   operating systems.  */
+
+__attribute__((__target_clones__("cpu=power9,default")))
+long mod_func (long a, long b)
+{
+  return a % b;
+}
+
+#define X 53L
+#define Y 7L
+int
+main (void)
+{
+  if (mod_func (X, Y) != (X % Y))
+    abort ();
+
+  return 0;
+}