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