Re: [PATCH v2 4/5] Add support for target_version attribute

[Richard Sandiford <richard.sandiford@arm.com>]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> This patch adds support for the "target_version" attribute to the middle
> end and the C++ frontend, which will be used to implement function
> multiversioning in the aarch64 backend.
>
> On targets that don't use the "target" attribute for multiversioning,
> there is no conflict between the "target" and "target_clones"
> attributes.  This patch therefore makes the mutual exclusion in
> C-family, D and Ada conditonal upon the value of the
> expanded_clones_attribute target hook.
>
> The "target_version" attribute is only added to C++ in this patch,
> because this is currently the only frontend which supports
> multiversioning using the "target" attribute.  Support for the
> "target_version" attribute will be extended to C at a later date.
>
> Targets that currently use the "target" attribute for function
> multiversioning (i.e. i386 and rs6000) are not affected by this patch.
>
> Ok for master?
>
> gcc/ChangeLog:
>
> 	* attribs.cc (decl_attributes): Pass attribute name to target.
> 	(is_function_default_version): Update comment to specify
> 	incompatibility with target_version attributes.
> 	* cgraphclones.cc (cgraph_node::create_version_clone_with_body):
> 	Call valid_version_attribute_p for target_version attributes.
> 	* target.def (valid_version_attribute_p): New hook.
> 	(expanded_clones_attribute): New hook.
> 	* doc/tm.texi.in: Add new hooks.
> 	* doc/tm.texi: Regenerate.
> 	* multiple_target.cc (create_dispatcher_calls): Remove redundant
> 	is_function_default_version check.
> 	(expand_target_clones): Use target hook for attribute name.
> 	* targhooks.cc (default_target_option_valid_version_attribute_p):
> 	New.
> 	* targhooks.h (default_target_option_valid_version_attribute_p):
> 	New.
> 	* tree.h (DECL_FUNCTION_VERSIONED): Update comment to include
> 	target_version attributes.
>
> gcc/c-family/ChangeLog:
>
> 	* c-attribs.cc (CLONES_USES_TARGET): New macro.
> 	(attr_target_exclusions): Use new macro.
> 	(attr_target_clones_exclusions): Ditto, and add target_version.
> 	(attr_target_version_exclusions): New.
> 	(c_common_attribute_table): Add target_version.
> 	(handle_target_version_attribute): New.
>
> gcc/ada/ChangeLog:
>
> 	* gcc-interface/utils.cc (CLONES_USES_TARGET): New macro.
> 	(attr_target_exclusions): Use new macro.
> 	(attr_target_clones_exclusions): Ditto.
>
> gcc/d/ChangeLog:
>
> 	* d-attribs.cc (CLONES_USES_TARGET): New macro.
> 	(attr_target_exclusions): Use new macro.
> 	(attr_target_clones_exclusions): Ditto.
>
> gcc/cp/ChangeLog:
>
> 	* decl2.cc (check_classfn): Update comment to include
> 	target_version attributes.
>
>
> diff --git a/gcc/ada/gcc-interface/utils.cc b/gcc/ada/gcc-interface/utils.cc
> index e33a63948cebdeafc3abcdd539a35141969ad978..8850943cb3326568b4679a73405f50487aa1b7c6 100644
> --- a/gcc/ada/gcc-interface/utils.cc
> +++ b/gcc/ada/gcc-interface/utils.cc
> @@ -143,16 +143,21 @@ static const struct attribute_spec::exclusions attr_noinline_exclusions[] =
>    { NULL, false, false, false },
>  };
>  
> +#define CLONES_USES_TARGET \
> +  (strcmp (targetm.target_option.expanded_clones_attribute, \
> +	   "target") == 0)
> +

Sorry for the slower review on this part.  I was hoping inspiration
would strike for a way to resolve this, but it hasn't, so:

The codebase usually avoids static variables that need dynamic
initialisation.  So although macros are not the preferred way of
doing things, I think one is probably appropriate here.  How about:

  TARGET_HAS_FMV_TARGET_ATTRIBUTE

with the default being true, and with AArch64 defining it to false?

This would replace the expanded_clones_attribute hook, with:

  const char *new_attr_name = targetm.target_option.expanded_clones_attribute;

becoming:

  const char *new_attr_name = (TARGET_HAS_FMV_TARGET_ATTRIBUTE
			       ? "target" : "target_version");

I realise this is anything but elegant, but I think it's probably
the least worst option, given where we are.

>  static const struct attribute_spec::exclusions attr_target_exclusions[] =
>  {
> -  { "target_clones", true, true, true },
> +  { "target_clones", CLONES_USES_TARGET, CLONES_USES_TARGET,
> +    CLONES_USES_TARGET },
>    { NULL, false, false, false },
>  };
>  
>  static const struct attribute_spec::exclusions attr_target_clones_exclusions[] =
>  {
>    { "always_inline", true, true, true },
> -  { "target", true, true, true },
> +  { "target", CLONES_USES_TARGET, CLONES_USES_TARGET, CLONES_USES_TARGET },
>    { NULL, false, false, false },
>  };
>  
> diff --git a/gcc/attribs.cc b/gcc/attribs.cc
> index f9fd258598914ce2112ecaaeaad6c63cd69a44e2..27533023ef5c481ba085c2f0c605dfb992987b3e 100644
> --- a/gcc/attribs.cc
> +++ b/gcc/attribs.cc
> @@ -657,7 +657,8 @@ decl_attributes (tree *node, tree attributes, int flags,
>       options to the attribute((target(...))) list.  */
>    if (TREE_CODE (*node) == FUNCTION_DECL
>        && current_target_pragma
> -      && targetm.target_option.valid_attribute_p (*node, NULL_TREE,
> +      && targetm.target_option.valid_attribute_p (*node,
> +						  get_identifier("target"),

Formatting nit: should be a space before ("target")

>  						  current_target_pragma, 0))
>      {
>        tree cur_attr = lookup_attribute ("target", attributes);
> @@ -1241,8 +1242,9 @@ make_dispatcher_decl (const tree decl)
>    return func_decl;  
>  }
>  
> -/* Returns true if decl is multi-versioned and DECL is the default function,
> -   that is it is not tagged with target specific optimization.  */
> +/* Returns true if DECL is multi-versioned using the target attribute, and this
> +   is the default version.  This function can only be used for targets that do
> +   not support the "target_version" attribute.  */
>  
>  bool
>  is_function_default_version (const tree decl)
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index b3b41ef123a0f171f57acb1b7f7fdde716428c00..8e33b7c3f4a9e7dcaa299eeff0eea92240f7ef0a 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -149,6 +149,7 @@ static tree handle_alloc_align_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_assume_aligned_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_assume_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_target_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_target_version_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_target_clones_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_optimize_attribute (tree *, tree, tree, int, bool *);
>  static tree ignore_attribute (tree *, tree, tree, int, bool *);
> @@ -228,16 +229,29 @@ static const struct attribute_spec::exclusions attr_noinline_exclusions[] =
>    ATTR_EXCL (NULL, false, false, false),
>  };
>  
> +#define CLONES_USES_TARGET \
> +  (strcmp (targetm.target_option.expanded_clones_attribute, \
> +	   "target") == 0)
> +
>  static const struct attribute_spec::exclusions attr_target_exclusions[] =
>  {
> -  ATTR_EXCL ("target_clones", true, true, true),
> +  ATTR_EXCL ("target_clones", CLONES_USES_TARGET, CLONES_USES_TARGET,
> +	     CLONES_USES_TARGET),
>    ATTR_EXCL (NULL, false, false, false),
>  };
>  
>  static const struct attribute_spec::exclusions attr_target_clones_exclusions[] =
>  {
>    ATTR_EXCL ("always_inline", true, true, true),
> -  ATTR_EXCL ("target", true, true, true),
> +  ATTR_EXCL ("target", CLONES_USES_TARGET, CLONES_USES_TARGET,
> +	     CLONES_USES_TARGET),
> +  ATTR_EXCL ("target_version", true, true, true),
> +  ATTR_EXCL (NULL, false, false, false),
> +};
> +
> +static const struct attribute_spec::exclusions attr_target_version_exclusions[] =
> +{
> +  ATTR_EXCL ("target_clones", true, true, true),

Just FTR: I suppose this should also include "target" if there is ever
a port that uses "target" and "target_version" for the same thing, but
there's no need to predict that case.

>    ATTR_EXCL (NULL, false, false, false),
>  };
>  
> @@ -505,6 +519,9 @@ const struct attribute_spec c_common_attribute_table[] =
>    { "target",                 1, -1, true, false, false, false,
>  			      handle_target_attribute,
>  			      attr_target_exclusions },
> +  { "target_version",         1, 1, true, false, false, false,
> +			      handle_target_version_attribute,
> +			      attr_target_version_exclusions },
>    { "target_clones",          1, -1, true, false, false, false,
>  			      handle_target_clones_attribute,
>  			      attr_target_clones_exclusions },
> @@ -5670,6 +5687,25 @@ handle_target_attribute (tree *node, tree name, tree args, int flags,
>    return NULL_TREE;
>  }
>  
> +/* Handle a "target_version" attribute.  */
> +
> +static tree
> +handle_target_version_attribute (tree *node, tree name, tree args, int flags,
> +				  bool *no_add_attrs)
> +{
> +  /* Ensure we have a function type.  */
> +  if (TREE_CODE (*node) != FUNCTION_DECL)
> +    {
> +      warning (OPT_Wattributes, "%qE attribute ignored", name);
> +      *no_add_attrs = true;
> +    }
> +  else if (!targetm.target_option.valid_version_attribute_p (*node, name, args,
> +							     flags))
> +    *no_add_attrs = true;
> +
> +  return NULL_TREE;
> +}
> +
>  /* Handle a "target_clones" attribute.  */
>  
>  static tree
> diff --git a/gcc/cgraphclones.cc b/gcc/cgraphclones.cc
> index 29d28ef895a73a223695cbb86aafbc845bbe7688..8af6b23d8c0306920e0fdcb3559ef047a16689f4 100644
> --- a/gcc/cgraphclones.cc
> +++ b/gcc/cgraphclones.cc
> @@ -78,6 +78,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tree-eh.h"
>  #include "tree-cfg.h"
>  #include "tree-inline.h"
> +#include "attribs.h"
>  #include "dumpfile.h"
>  #include "gimple-pretty-print.h"
>  #include "alloc-pool.h"
> @@ -1048,7 +1049,17 @@ cgraph_node::create_version_clone_with_body
>        location_t saved_loc = input_location;
>        tree v = TREE_VALUE (target_attributes);
>        input_location = DECL_SOURCE_LOCATION (new_decl);
> -      bool r = targetm.target_option.valid_attribute_p (new_decl, NULL, v, 1);
> +      bool r;
> +      tree name_id = get_attribute_name (target_attributes);
> +      const char* name_str = IDENTIFIER_POINTER (name_id);

Formatting nit, sorry, but: "const char* name_str".

> +      if (strcmp (name_str, "target") == 0)
> +	r = targetm.target_option.valid_attribute_p (new_decl, name_id, v, 1);
> +      else if (strcmp (name_str, "target_version") == 0)
> +	r = targetm.target_option.valid_version_attribute_p (new_decl, name_id,
> +							     v, 1);
> +      else
> +	gcc_assert(false);

gcc_unreachable ();

LGTM otherwise, thanks.

Richard

> +
>        input_location = saved_loc;
>        if (!r)
>  	return NULL;
> diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
> index 9e666e5eecee07ae7c742c3a2b27e85899945c4e..e607aa14d284d545d122e04b0eae1247fd301882 100644
> --- a/gcc/cp/decl2.cc
> +++ b/gcc/cp/decl2.cc
> @@ -832,8 +832,8 @@ check_classfn (tree ctype, tree function, tree template_parms)
>        tree c2 = get_constraints (fndecl);
>  
>        /* While finding a match, same types and params are not enough
> -	 if the function is versioned.  Also check version ("target")
> -	 attributes.  */
> +	 if the function is versioned.  Also check for different target
> +	 specific attributes.  */
>        if (same_type_p (TREE_TYPE (TREE_TYPE (function)),
>  		       TREE_TYPE (TREE_TYPE (fndecl)))
>  	  && compparms (p1, p2)
> diff --git a/gcc/d/d-attribs.cc b/gcc/d/d-attribs.cc
> index c0dc0e24ded871c136e54e5527e901d16cfa5ceb..7fe68565e70dd1124aac63601416dad68600a34e 100644
> --- a/gcc/d/d-attribs.cc
> +++ b/gcc/d/d-attribs.cc
> @@ -126,16 +126,22 @@ static const struct attribute_spec::exclusions attr_noinline_exclusions[] =
>    ATTR_EXCL (NULL, false, false, false),
>  };
>  
> +#define CLONES_USES_TARGET \
> +  (strcmp (targetm.target_option.expanded_clones_attribute, \
> +	   "target") == 0)
> +
>  static const struct attribute_spec::exclusions attr_target_exclusions[] =
>  {
> -  ATTR_EXCL ("target_clones", true, true, true),
> +  ATTR_EXCL ("target_clones", CLONES_USES_TARGET, CLONES_USES_TARGET,
> +	     CLONES_USES_TARGET),
>    ATTR_EXCL (NULL, false, false, false),
>  };
>  
>  static const struct attribute_spec::exclusions attr_target_clones_exclusions[] =
>  {
>    ATTR_EXCL ("always_inline", true, true, true),
> -  ATTR_EXCL ("target", true, true, true),
> +  ATTR_EXCL ("target", CLONES_USES_TARGET, CLONES_USES_TARGET,
> +	     CLONES_USES_TARGET),
>    ATTR_EXCL (NULL, false, false, false),
>  };
>  
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index d83ca73b1aff90d3c181436afedc162b977a4158..6f6b133803f4574fcf0112b1385eec861112ddd5 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -10644,6 +10644,23 @@ the function declaration to hold a pointer to a target-specific
>  @code{struct cl_target_option} structure.
>  @end deftypefn
>  
> +@deftypefn {Target Hook} bool TARGET_OPTION_VALID_VERSION_ATTRIBUTE_P (tree @var{fndecl}, tree @var{name}, tree @var{args}, int @var{flags})
> +This hook is called to parse @code{attribute(target_version("..."))},
> +which allows setting target-specific options on individual function versions.
> +These function-specific options may differ
> +from the options specified on the command line.  The hook should return
> +@code{true} if the options are valid.
> +
> +The hook should set the @code{DECL_FUNCTION_SPECIFIC_TARGET} field in
> +the function declaration to hold a pointer to a target-specific
> +@code{struct cl_target_option} structure.
> +@end deftypefn
> +
> +@deftypevr {Target Hook} {const char *} TARGET_OPTION_EXPANDED_CLONES_ATTRIBUTE
> +Contains the name of the attribute used for the version description string
> +when expanding clones for a function with the target_clones attribute.
> +@end deftypevr
> +
>  @deftypefn {Target Hook} void TARGET_OPTION_SAVE (struct cl_target_option *@var{ptr}, struct gcc_options *@var{opts}, struct gcc_options *@var{opts_set})
>  This hook is called to save any additional target-specific information
>  in the @code{struct cl_target_option} structure for function-specific
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 3d3ae12cc2ff62025b1138430de501a33961fd90..149c88f627be20a9a35ead2eaebdb704e51927fa 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -7028,6 +7028,10 @@ on this implementation detail.
>  
>  @hook TARGET_OPTION_VALID_ATTRIBUTE_P
>  
> +@hook TARGET_OPTION_VALID_VERSION_ATTRIBUTE_P
> +
> +@hook TARGET_OPTION_EXPANDED_CLONES_ATTRIBUTE
> +
>  @hook TARGET_OPTION_SAVE
>  
>  @hook TARGET_OPTION_RESTORE
> diff --git a/gcc/multiple_target.cc b/gcc/multiple_target.cc
> index a2ed048d7dd28ec470953fcd8a0dc86817e4b7dc..3db57c2b13d612a37240d9dcf58ad21b2286633c 100644
> --- a/gcc/multiple_target.cc
> +++ b/gcc/multiple_target.cc
> @@ -66,10 +66,6 @@ create_dispatcher_calls (struct cgraph_node *node)
>  {
>    ipa_ref *ref;
>  
> -  if (!DECL_FUNCTION_VERSIONED (node->decl)
> -      || !is_function_default_version (node->decl))
> -    return;
> -
>    if (!targetm.has_ifunc_p ())
>      {
>        error_at (DECL_SOURCE_LOCATION (node->decl),
> @@ -377,6 +373,7 @@ expand_target_clones (struct cgraph_node *node, bool definition)
>        return false;
>      }
>  
> +  const char *new_attr_name = targetm.target_option.expanded_clones_attribute;
>    cgraph_function_version_info *decl1_v = NULL;
>    cgraph_function_version_info *decl2_v = NULL;
>    cgraph_function_version_info *before = NULL;
> @@ -392,7 +389,7 @@ expand_target_clones (struct cgraph_node *node, bool definition)
>        char *attr = attrs[i];
>  
>        /* Create new target clone.  */
> -      tree attributes = make_attribute ("target", attr,
> +      tree attributes = make_attribute (new_attr_name, attr,
>  					DECL_ATTRIBUTES (node->decl));
>  
>        char *suffix = XNEWVEC (char, strlen (attr) + 1);
> @@ -430,7 +427,7 @@ expand_target_clones (struct cgraph_node *node, bool definition)
>    XDELETEVEC (attr_str);
>  
>    /* Setting new attribute to initial function.  */
> -  tree attributes = make_attribute ("target", "default",
> +  tree attributes = make_attribute (new_attr_name, "default",
>  				    DECL_ATTRIBUTES (node->decl));
>    DECL_ATTRIBUTES (node->decl) = attributes;
>    node->local = false;
> diff --git a/gcc/target.def b/gcc/target.def
> index 0996da0f71a85f8217a41ceb08de8b21087e4ed9..1d2e0d8bf03a8b949ec636e6a78a111308d3dd71 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -6533,6 +6533,31 @@ the function declaration to hold a pointer to a target-specific\n\
>   bool, (tree fndecl, tree name, tree args, int flags),
>   default_target_option_valid_attribute_p)
>  
> +/* Function to validate the attribute((target_version(...))) strings.  If
> +   the option is validated, the hook should also fill in
> +   DECL_FUNCTION_SPECIFIC_TARGET in the function decl node.  */
> +DEFHOOK
> +(valid_version_attribute_p,
> + "This hook is called to parse @code{attribute(target_version(\"...\"))},\n\
> +which allows setting target-specific options on individual function versions.\n\
> +These function-specific options may differ\n\
> +from the options specified on the command line.  The hook should return\n\
> +@code{true} if the options are valid.\n\
> +\n\
> +The hook should set the @code{DECL_FUNCTION_SPECIFIC_TARGET} field in\n\
> +the function declaration to hold a pointer to a target-specific\n\
> +@code{struct cl_target_option} structure.",
> + bool, (tree fndecl, tree name, tree args, int flags),
> + default_target_option_valid_version_attribute_p)
> +
> +/* Attribute to be used when expanding clones for functions with
> +   target_clones attribute.  */
> +DEFHOOKPOD
> +(expanded_clones_attribute,
> + "Contains the name of the attribute used for the version description string\n\
> +when expanding clones for a function with the target_clones attribute.",
> + const char *, "target")
> +
>  /* Function to save any extra target state in the target options structure.  */
>  DEFHOOK
>  (save,
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index 189549cb1c742c37c17623141989b492a7c2b2f8..ff2957fd9fd8389e23992281b35e8e5467072f7d 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -192,6 +192,7 @@ extern bool default_hard_regno_scratch_ok (unsigned int);
>  extern bool default_mode_dependent_address_p (const_rtx, addr_space_t);
>  extern bool default_new_address_profitable_p (rtx, rtx_insn *, rtx);
>  extern bool default_target_option_valid_attribute_p (tree, tree, tree, int);
> +extern bool default_target_option_valid_version_attribute_p (tree, tree, tree, int);
>  extern bool default_target_option_pragma_parse (tree, tree);
>  extern bool default_target_can_inline_p (tree, tree);
>  extern bool default_update_ipa_fn_target_info (unsigned int &, const gimple *);
> diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
> index 4f5b240f8d65eeeaf73418c9f1e2c2684b257cfa..b693352d7eae555912477b6e431dd9c016105007 100644
> --- a/gcc/targhooks.cc
> +++ b/gcc/targhooks.cc
> @@ -1789,7 +1789,19 @@ default_target_option_valid_attribute_p (tree ARG_UNUSED (fndecl),
>  					 int ARG_UNUSED (flags))
>  {
>    warning (OPT_Wattributes,
> -	   "target attribute is not supported on this machine");
> +	   "%<target%> attribute is not supported on this machine");
> +
> +  return false;
> +}
> +
> +bool
> +default_target_option_valid_version_attribute_p (tree ARG_UNUSED (fndecl),
> +						 tree ARG_UNUSED (name),
> +						 tree ARG_UNUSED (args),
> +						 int ARG_UNUSED (flags))
> +{
> +  warning (OPT_Wattributes,
> +	   "%<target_version%> attribute is not supported on this machine");
>  
>    return false;
>  }
> diff --git a/gcc/tree.h b/gcc/tree.h
> index 086b55f0375435d53a1604b6659da4f19fce3d17..d7841af19b20b0dc0ae28b433d5150e9c4763eff 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -3500,8 +3500,8 @@ extern vec<tree, va_gc> **decl_debug_args_insert (tree);
>     (FUNCTION_DECL_CHECK (NODE)->function_decl.function_specific_optimization)
>  
>  /* In FUNCTION_DECL, this is set if this function has other versions generated
> -   using "target" attributes.  The default version is the one which does not
> -   have any "target" attribute set. */
> +   to support different architecture feature sets, e.g. using "target" or
> +   "target_version" attributes.  */
>  #define DECL_FUNCTION_VERSIONED(NODE)\
>     (FUNCTION_DECL_CHECK (NODE)->function_decl.versioned_function)
>