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

[Michael Meissner <meissner@linux.vnet.ibm.com>]

On Tue, May 30, 2017 at 04:51:34PM -0500, Segher Boessenkool wrote:
> 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.

It is easier to write the loops going up, but I have changed it to const ints
and deleted the enum.

> > +static const struct clone_map rs6000_clone_map[ (int)CLONE_MAX ] = {
> 
> Space after cast; no spaces inside [].

Yep.

> > +static inline const char *
> > +get_decl_name (tree fn)
> 
> Please don't use inline unless there is a good reason to.

Ok.

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

Yep.

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

Ok.

> > +  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).

Ok.

> > +#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?

Yes I believe it is.  ASM_OUTPUT_TYPE_DIRECTIVE is only defined in sysv4.h.
You need the .type directive to be able to declare .ifunc functions (plus
enabling ifunc which we now do as a default).  AIX and non-Linux systems will
not be able to use target_clones.

> > +  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).

I've recoded these.

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

Ok.

> > +  /* The resolver function should return a (void *). */
> 
> And two after a dot.

Ok.

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

Ok.

> > +  /* 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.

I will probably need to call cgraph_create_function_alias in the next round
when I fix what I consider to be the big problem with target_clones (namely,
outside of the function you don't use the target clones, you only use the ifunc
support for the current module.  But I will comment it for now.

> 
> > +  gcc_assert (new_bb != NULL);
> > +  gseq = bb_seq (new_bb);
> 
> Same as before.

Ok.

> > +  convert_expr = build1 (CONVERT_EXPR, ptr_type_node,
> > +	     		 build_fold_addr_expr (version_decl));
> 
> Indent is broken here.

Ok.

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

See above.

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

Ummm, it was my understanding in C++, you no longer get a free cast to void *,
and when you do need to use it in the mem* functions, you need an explicit
case.

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

The expansion of the __builtin_cpu_supports ensures we have a new enough glibc,
but I can expand on the comment (basically x86 needs to call
__builtin_cpu_init, we don't).

> > +static tree 
> > +rs6000_generate_version_dispatcher_body (void *node_p)
> 
> Trailing space.

Ok.

> > +  node = (cgraph_node *)node_p;
> 
> Space after cast.

Ok.

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

Ok.

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

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