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
