Andrew Carlotti <andrew.carlo...@arm.com> writes:
> On Thu, Oct 26, 2023 at 07:41:09PM +0100, 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.
>> >
>> > Note that C++ is currently the only frontend which supports
>> > multiversioning using the "target" attribute, whereas the
>> > "target_clones" attribute is additionally supported in C, D and Ada.
>> > 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.
>> >
>> >
>> > I could have implemented the target hooks slightly differently, by reusing
>> > the
>> > valid_attribute_p hook and adding attribute name checks to each backend
>> > implementation (c.f. the aarch64 implementation in patch 2/3). Would this
>> > be
>> > preferable?
>>
>> Having as much as possible in target-independent code seems better
>> to me FWIW. On that basis:
>>
>> >
>> > Otherwise, is this ok for master?
>> >
>> >
>> > gcc/c-family/ChangeLog:
>> >
>> > * c-attribs.cc (handle_target_version_attribute): New.
>> > (c_common_attribute_table): Add target_version.
>> > (handle_target_clones_attribute): Add conflict with
>> > target_version attribute.
>> >
>> > gcc/ChangeLog:
>> >
>> > * attribs.cc (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/cp/ChangeLog:
>> >
>> > * decl2.cc (check_classfn): Update comment to include
>> > target_version attributes.
>> >
>> >
>> > diff --git a/gcc/attribs.cc b/gcc/attribs.cc
>> > index
>> > b1300018d1e8ed8e02ded1ea721dc192a6d32a49..a3c4a81e8582ea4fd06b9518bf51fad7c998ddd6
>> > 100644
>> > --- a/gcc/attribs.cc
>> > +++ b/gcc/attribs.cc
>> > @@ -1233,8 +1233,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
>> > 072cfb69147bd6b314459c0bd48a0c1fb92d3e4d..1a224c036277d51ab4dc0d33a403177bd226e48a
>> > 100644
>> > --- a/gcc/c-family/c-attribs.cc
>> > +++ b/gcc/c-family/c-attribs.cc
>> > @@ -148,6 +148,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 *);
>> > @@ -480,6 +481,8 @@ const struct attribute_spec c_common_attribute_table[]
>> > =
>> > handle_error_attribute, NULL },
>> > { "target", 1, -1, true, false, false, false,
>> > handle_target_attribute, NULL },
>> > + { "target_version", 1, -1, true, false, false, false,
>> > + handle_target_version_attribute, NULL },
>> > { "target_clones", 1, -1, true, false, false, false,
>> > handle_target_clones_attribute, NULL },
>> > { "optimize", 1, -1, true, false, false, false,
>> > @@ -5569,6 +5572,45 @@ 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 (lookup_attribute ("target_clones", DECL_ATTRIBUTES (*node)))
>> > + {
>> > + warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
>> > + "with %qs attribute", name, "target_clones");
>> > + *no_add_attrs = true;
>> > + }
>> > + else if (!targetm.target_option.valid_version_attribute_p (*node, name,
>> > args,
>> > + flags))
>> > + *no_add_attrs = true;
>> > +
>> > + /* Check that there's no empty string in values of the attribute. */
>> > + for (tree t = args; t != NULL_TREE; t = TREE_CHAIN (t))
>> > + {
>> > + tree value = TREE_VALUE (t);
>> > + if (TREE_CODE (value) == STRING_CST
>> > + && TREE_STRING_LENGTH (value) == 1
>> > + && TREE_STRING_POINTER (value)[0] == '\0')
>> > + {
>> > + warning (OPT_Wattributes,
>> > + "empty string in attribute %<target_version%>");
>> > + *no_add_attrs = true;
>> > + }
>> > + }
>>
>> would it make sense to do the empty string test first, and only pass
>> the vetted arguments to the target hook? Also, a Google search suggests
>> that there aren't any pre-existing, conflicting uses of "target_version"
>> that take multiple arguments. So could this code check that there
>> is exactly one argument (by changing 1, -1 to 1, 1 in the spec above)
>> and then require it to be a nonempty string? It could then pass the
>> string itself to the target hook (probably as a const char *).
>>
>> (FWIW, it doesn't look like the Clang documentation has kept the door
>> open to multiple arguments.)
>>
>> I wonder if we could use attribute_spec::exclusions to describe the
>> mutual exclusion with "target_clones". It doesn't look like the
>> existing code does, though, so maybe not.
>>
>> I couldn't see anything that forbids a combination of "target" and
>> "target_version". Should that combination be allowed? In some ways
>> it makes conceptual sense, since using "target" is like changing the
>> command-line options. But I suppose we'd then need to diagnose conflicts
>> and deal with ordering issues. So perhaps "target" should be made
>> mutually exclusive as well.
>
> My aarch64 backend pass deals with backend issues by always applying
> target_version attribute changes after target_attribute changes. I don't
> think
> there's any additional conflicts to worry about, since adding a target_version
> is simply equivalent to enabling extra features in the target string.
>
> A similar thing would work for target_clones, but I didn't initially do that
> because it would require making the frontend exclusions target dependant.
> However, I think it's just a case of checking the backend hook to see whether
> target_clones gets expanded to target attributes or not.
>
> Clang currently disallows combining target attributes with either
> target_version or target_clones. However, I think it's worth being able to
> combine these attributes. For example, it could be useful to use the target
> attribute to select different tuning for an sve target_version.
Ah, ok, thanks for the explanation. That approach sounds good to me.
Richard
