Re: [PATCH] Add attribute((target_clone(...))) to PowerPC

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi Mike,

On Thu, May 25, 2017 at 04:05:39PM -0400, Michael Meissner wrote:
> +/* On PowerPC, we have a limited number of target clones that we care about
> +   which means we can use an array to hold the options, rather than having more
> +   elaborate data structures to identify each possible variation.  Order the
> +   clones from the highest ISA to the least.  */
> +enum clone_list {
> +  CLONE_ISA_3_00,		/* ISA 3.00 (power9).  */
> +  CLONE_ISA_2_07,		/* ISA 2.07 (power8).  */
> +  CLONE_ISA_2_06,		/* ISA 2.06 (power7).  */
> +  CLONE_ISA_2_05,		/* ISA 2.05 (power6).  */
> +  CLONE_DEFAULT,		/* default clone.  */
> +  CLONE_MAX
> +};

Is this easier than the more natural ordering (from default to higher)?
Also, since you use the enum values as numbers, please make the first
on explicitly "= 0".  These go together: default 0 is nice to have.

> +static const struct clone_map rs6000_clone_map[ (int)CLONE_MAX ] = {

Space after cast; no spaces inside [].

> +static inline const char *
> +get_decl_name (tree fn)

Please don't use inline unless there is a good reason to.

> +  if (TARGET_DEBUG_TARGET)
> +    fprintf (stderr, "rs6000_get_function_version_priority (%s) => %d\n",
> +	     get_decl_name (fndecl), (int) ret);

"ret" already is an int.  Similarly, are the casts of the enum values
necessary?

> +  struct cgraph_function_version_info *default_version_info = NULL;

You always initialise this variable later on; don't set it to NULL
earlier.  You can move the declaration down to where the var is first
initialised.

> +  tree dispatch_decl = NULL;

For this one, you can put it inside the if (), and just explicitly
return NULL on the error path (you do that in one case already).

> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)

Is this the correct conditional to use?  It is not obvious to me why
it would be.  Does it have to be an #ifdef anyway, can't it be an if?

> +  if (targetm.has_ifunc_p ())
> +    {
> +      struct cgraph_function_version_info *it_v = NULL;
> +      struct cgraph_node *dispatcher_node = NULL;
> +      struct cgraph_function_version_info *dispatcher_version_info = NULL;

No NULL for these either please.  If you later add a path where you
forget to initialise one of these vars you will not get a warning
(and if nothing goes wrong these initialisations are distracting noise).

> +/* Make the resolver function decl to dispatch the versions of
> +   a multi-versioned function,  DEFAULT_DECL.  Create an

One space after comma.

> +  /* The resolver function should return a (void *). */

And two after a dot.

> +  gcc_assert (dispatch_decl != NULL);
> +  /* Mark dispatch_decl as "ifunc" with resolver as resolver_name.  */
> +  DECL_ATTRIBUTES (dispatch_decl)
> +    = make_attribute ("ifunc", resolver_name, DECL_ATTRIBUTES (dispatch_decl));

That assert is not very useful: the very next statement would segfault
if the assertion fails, giving just as much information.

> +  /* Create the alias for dispatch to resolver here.  */
> +  /*cgraph_create_function_alias (dispatch_decl, decl);*/

Do you need to keep this line?  Please add a comment saying why it is
disabled for now, or such.

> +  gcc_assert (new_bb != NULL);
> +  gseq = bb_seq (new_bb);

Same as before.

> +  convert_expr = build1 (CONVERT_EXPR, ptr_type_node,
> +	     		 build_fold_addr_expr (version_decl));

Indent is broken here.

> +  result_var = create_tmp_var (ptr_type_node);
> +  convert_stmt = gimple_build_assign (result_var, convert_expr); 

Space at end of line.

> +  if (clone_isa == (int)CLONE_DEFAULT)

Space after cast.  Do you need a cast here?

> +  predicate_decl = rs6000_builtin_decls [(int) RS6000_BUILTIN_CPU_SUPPORTS];

You don't need a cast here either afaics.

> +  make_edge (bb1, bb3, EDGE_FALSE_VALUE); 

Space at end of line.

> +  /* The first version in the vector is the default decl.  */
> +  memset ((void *) clones, '\0', sizeof (clones));

memset (clones, 0, sizeof clones);

or just initialise it in the first place:

tree clones[CLONE_MAX] = { 0 };

> +  /* On the PowerPC, we do not need to call __builtin_cpu_init, if we are using
> +     a new enough glibc.  If we ever need to call it, we would need to insert
> +     the code here to do the call.  */

Are we always using a new enough glibc?  If so, please clarify the
comment.

> +static tree 
> +rs6000_generate_version_dispatcher_body (void *node_p)

Trailing space.

> +  node = (cgraph_node *)node_p;

Space after cast.

> +On a PowerPC, you could compile a function with
> +@code{target_clones("cpu=power9,default")}.  GCC creates two function

"For PowerPC you can ..."?

> --- gcc/testsuite/gcc.target/powerpc/clone1.c	(.../svn+ssh://meissner@gcc.gnu.org/svn/gcc/trunk/gcc/testsuite/gcc.target/powerpc)	(revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/clone1.c	(.../gcc/testsuite/gcc.target/powerpc)	(revision 248446)
> @@ -0,0 +1,19 @@
> +/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */

s/powerpc64/powerpc/

Looks good so far, just needs some polish ;-)  Please consider changing
the clone_list enum to a more natural order (and does the enum need a
name, anyway?), tidy up layout stuff etc., and repost.

Thanks,


Segher