On Wed, Nov 29, 2023 at 05:53:56PM +0000, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlo...@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.
I thought this could be an issue, and had deliberately not committed patches
2+3 in this series in case fixing this required reverting to specific runtime
checks within each handler.
I've changed it to use your suggestion in the next version.
> > 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")
Fixed.
> > 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.
I don't think these need to be mutually exclusive, even in that case, although
if we wanted to make it work then we'd probably have to improve how the
existing code handles target attribute multiversioning.
> > 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".
Fixed.
> > + 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 ();
Fixed.
> 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)
> >
