diff mbox

, PowerPC target_clones minor support

Message ID 20170707184521.GA20306@ibm-tiger.the-meissners.org
State New
Headers show

Commit Message

Michael Meissner July 7, 2017, 6:45 p.m. UTC
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.
diff mbox

Patch

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;
+}