Alfie Richards <alfie.richa...@arm.com> writes:
> This patch adds support for the combination of target_clones and
> target_version in the definition of a versioned function.
>
> This patch changes is_function_default_version to consider a function
> declaration annotated with target_clones containing default to be a
> default version. It also changes the common_function_version hook to
> consider two functions annotated with target_clones and/or
> target_versions to be common if their specified versions don't overlap.
>
> This takes advantage of refactoring done in previous patches changing
> how target_clones are expanded.
>
> gcc/ChangeLog:
>
> * attribs.cc (is_function_default_version): Add logic for
> target_clones defining the default version.
> * config/aarch64/aarch64.cc (aarch64_common_function_versions): Add
> logic for a target_clones and target_version, or two target_clones
> coexisting in a version set.
>
> gcc/c-family/ChangeLog:
>
> * c-attribs.cc: Add support for target_version and target_clones
> coexisting.
>
> gcc/testsuite/ChangeLog:
>
> * g++.target/aarch64/mv-and-mvc1.C: New test.
> * g++.target/aarch64/mv-and-mvc2.C: New test.
> * g++.target/aarch64/mv-and-mvc3.C: New test.
> * g++.target/aarch64/mv-and-mvc4.C: New test.
> ---
> gcc/attribs.cc | 7 +++
> gcc/c-family/c-attribs.cc | 2 -
> gcc/config/aarch64/aarch64.cc | 46 ++++++++++++++++++-
> .../g++.target/aarch64/mv-and-mvc1.C | 38 +++++++++++++++
> .../g++.target/aarch64/mv-and-mvc2.C | 29 ++++++++++++
> .../g++.target/aarch64/mv-and-mvc3.C | 41 +++++++++++++++++
> .../g++.target/aarch64/mv-and-mvc4.C | 38 +++++++++++++++
> gcc/testsuite/g++.target/aarch64/mv-error1.C | 13 ++++++
> gcc/testsuite/g++.target/aarch64/mv-error13.C | 13 ++++++
> gcc/testsuite/g++.target/aarch64/mv-error2.C | 10 ++++
> gcc/testsuite/g++.target/aarch64/mv-error3.C | 13 ++++++
> gcc/testsuite/g++.target/aarch64/mv-error7.C | 9 ++++
> gcc/testsuite/g++.target/aarch64/mv-error8.C | 21 +++++++++
> gcc/testsuite/g++.target/aarch64/mv-error9.C | 12 +++++
> 14 files changed, 289 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-and-mvc1.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-and-mvc2.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-and-mvc3.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-and-mvc4.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-error1.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-error13.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-error2.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-error3.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-error7.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-error8.C
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-error9.C
>
> diff --git a/gcc/attribs.cc b/gcc/attribs.cc
> index 687e6d4143a..f877dc4f6e3 100644
> --- a/gcc/attribs.cc
> +++ b/gcc/attribs.cc
> @@ -1327,6 +1327,13 @@ is_function_default_version (const tree decl)
> }
> else
> {
> + if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (decl)))
> + {
> + int num_def = 0;
> + auto_vec<string_slice> versions = get_clone_versions (decl, &num_def);
> + return num_def > 0;
> + }
> +
Might be worth expanding the name to num_default. At first I read it
as "number of definitions". The "auto_vec<string_slice> versions = "
looks redundant.
> attr = lookup_attribute ("target_version", DECL_ATTRIBUTES (decl));
> if (!attr)
> return true;
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 642d724f6c6..f2cc43ad641 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -249,13 +249,11 @@ static const struct attribute_spec::exclusions
> attr_target_clones_exclusions[] =
> ATTR_EXCL ("always_inline", true, true, true),
> ATTR_EXCL ("target", TARGET_HAS_FMV_TARGET_ATTRIBUTE,
> TARGET_HAS_FMV_TARGET_ATTRIBUTE, TARGET_HAS_FMV_TARGET_ATTRIBUTE),
> - 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),
> ATTR_EXCL (NULL, false, false, false),
> };
We can remove the exclusions altogether if the list is empty.
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 420bbba9be2..f6cb7903d88 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -20671,7 +20671,51 @@ aarch64_common_function_versions (tree fn1, tree fn2)
> || TREE_CODE (fn2) != FUNCTION_DECL)
> return false;
>
> - return (aarch64_compare_version_priority (fn1, fn2) != 0);
> + if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (fn2)))
> + {
> + tree temp = fn1;
> + fn1 = fn2;
> + fn2 = temp;
> + }
This can use std::swap.
> +
> + if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (fn1)))
> + {
> + auto_vec<string_slice> fn1_versions = get_clone_versions (fn1);
> + // fn1 is target_clone
> + if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (fn2)))
> + {
> + auto_vec<string_slice> fn2_versions = get_clone_versions (fn2);
> + for (string_slice v1 : fn1_versions)
> + {
> + aarch64_fmv_feature_mask v1_mask;
> + aarch64_parse_fmv_features (v1, NULL, &v1_mask, NULL);
> + for (string_slice v2 : fn2_versions)
> + {
> + aarch64_fmv_feature_mask v2_mask;
> + aarch64_parse_fmv_features (v2, NULL, &v2_mask, NULL);
> + if (v1_mask == v2_mask)
> + return false;
> + }
> + }
> + return true;
> + }
> + else
> + {
> + aarch64_fmv_feature_mask v2_mask = get_feature_mask_for_version (fn2);
> + for (string_slice v1 : fn1_versions)
> + {
> + aarch64_fmv_feature_mask v1_mask;
> + aarch64_parse_fmv_features (v1, NULL, &v1_mask, NULL);
> + if (v1_mask == v2_mask)
> + return false;
> + }
> + return true;
> + }
> + }
> + // Can assume both are target_version
> + else
> + // Both are target_version
> + return (aarch64_compare_version_priority (fn1, fn2) != 0);
Formatting nit: redundant brackets around the expression. For files that
already use /* ... */ comments, I think we should stick to those.
> }
>
> /* Implement TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P. Use an opt-out
> [...]
It looks like all of the error tests are for duplicate target_versions.
It would be good to also test duplicates in and between target_clones
(if we don't do that elsewhere), and duplicates between target_versions
and target_clones.
Thanks,
Richard
