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://meiss...@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: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797