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

[Michael Meissner <meissner at linux dot vnet dot ibm dot com>]

On Thu, Jun 01, 2017 at 03:43:22PM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Wed, May 31, 2017 at 06:33:37PM -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 default to the highest ISA.  */
> > +const int CLONE_DEFAULT		= 0;	/* default clone.  */
> > +const int CLONE_ISA_2_05	= 1;	/* ISA 2.05 (power6).  */
> > +const int CLONE_ISA_2_06	= 2;	/* ISA 2.06 (power7).  */
> > +const int CLONE_ISA_2_07	= 3;	/* ISA 2.07 (power8).  */
> > +const int CLONE_ISA_3_00	= 4;	/* ISA 3.00 (power9).  */
> > +const int CLONE_MAX		= 5;
> 
> With "you don't have to give the enum a name" I meant write it as
> 
> enum {
>   CLONE_DEFAULT = 0,
>   CLONE_ISA_2_05,
> [...]
>   CLONE_MASK
> };
> 
> If you do "const int", I think it should be "static const int"?

Ok.  I think I was under the impression that enums were more tightly typed on
C++ compared to C, and that you needed explicit casts to/from integer.

> > +/* Helper function for printing the function name when debugging.  */
> > +
> > +static const char *
> > +get_decl_name (tree fn)
> > +{
> > +  tree name;
> > +
> > +  if (!fn)
> > +    return "<null>";
> > +
> > +  name = DECL_NAME (fn);
> > +  if (!name)
> > +    return "<no-name>";
> > +
> > +  return IDENTIFIER_POINTER (name);
> > +}
> 
> Perhaps this would be useful to have in generic code?

Perhaps, but it is just for printing debug messages.  I moved it to a separate
function to simplify indentation issues.

> > +rs6000_clone_priority (tree fndecl)
> > +{
> > +  tree fn_opts = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
> > +  HOST_WIDE_INT isa_masks;
> > +  int ret = (int) CLONE_DEFAULT;
> 
> You don't need this cast afaics.
> 
> > +  tree attrs = lookup_attribute ("target", DECL_ATTRIBUTES (fndecl));
> > +  const char *attrs_str = NULL;
> > +
> > +  gcc_assert (attrs != NULL);
> > +  attrs = TREE_VALUE (TREE_VALUE (attrs));
> > +
> > +  gcc_assert (TREE_CODE (attrs) == STRING_CST);
> > +  attrs_str = TREE_STRING_POINTER (attrs);
> 
> And these asserts neither.  There are more of these: if the code
> immediately following an assert will obviously fail (in an obvious way)
> if the assert is false, then the assert is just noise, makes reading
> the code harder instead of easier.

Ok.

> > +/* This compares the priority of target features in function DECL1 and DECL2.
> > +   It returns positive value if DECL1 is higher priority, negative value if
> > +   DECL2 is higher priority and 0 if they are the same.  Note, priorities are
> > +   ordered from lowest (currently CLONE_ISA_3_0) to highest
> > +   (CLONE_DEFAULT).  */
> 
> This comment needs updating?  Swap CLONE_ISA_3_0 with CLONE_DEFAULT?

Yes, I missed updating this comment.

> > +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
> > +  if (targetm.has_ifunc_p ())
> 
> Hrm, I still don't see what you need the #ifdef for.  What in the
> following code won't compile without it?  Or does targetm.has_ifunc_p
> return the wrong answer?

Right now, we only enable ifunc by default under Linux, so I removed the
#ifdef.  We will see if it breaks on non Linux systems.

Are these patches ok to install?

[gcc]
2017-06-02  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* config/rs6000/rs6000.c (toplevel): Include attribs.h.
	(CLONE_*): New constants to define the processors we can generate
	code for with the target_clone attribute.
	(rs6000_clone_map): New array to identify which clone processors
	the current program is running on.
	(TARGET_COMPARE_VERSION_PRIORITY): Define to enable the
	target_clone attribute.
	(TARGET_GENERATE_VERSION_DISPATCHER_BODY): Likewise.
	(TARGET_GET_FUNCTION_VERSIONS_DISPATCHER): Likewise.
	(TARGET_OPTION_FUNCTION_VERSIONS): Likewise.
	(cpu_expand_builtin): Add support for target_clone attribute.
	(rs6000_valid_attribute_p): Allow "default" attribute.
	(get_decl_name): New debug function to simplify printing the
	current function name in debugging statements.
	(rs6000_clone_priority): New functions to support the target_clone
	attribute, and be able to generate code to switch between ISA 2.05
	through ISA 3.0 (power6 through power9).
	(rs6000_compare_version_priority): Likewise.
	(rs6000_get_function_versions_dispatcher): Likewise.
	(make_resolver_func): Likewise.
	(add_condition_to_bb): Likewise.
	(dispatch_function_versions): Likewise.
	(rs6000_generate_version_dispatcher_body): Likewise.
	(rs6000_can_inline_p): Call get_decl_name for debugging usage.
	(fusion_gpr_load_p): Fix a spacing issue.
	* doc/extend.texi (Common Function Attributes): Document that the
	PowerPC supports the target_clone attribute.

[gcc/testsuite]
2017-06-02  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* gcc.target/powerpc/clone1.c: New test.

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

Attachment: clone.patch08b
Description: Text document