<alfie.richa...@arm.com> writes:
> From: Alfie Richards <alfie.richa...@arm.com>
>
> This patch introduces the TARGET_CHECK_TARGET_CLONE_VERSION hook
> which is used to determine if a target_clones version string parses.
>
> The hook has a flag to enable emitting diagnostics.
>
> This is as specified in the Arm C Language Extension. The purpose of this
> is to be able to ignore invalid versions to allow some portability of code
> using target_clones attributes.
>
> Currently this is only properly implemented for the Aarch64 backend.
>
> For riscv which is the only other backend which uses target_version
> semantics a partial implementation is present, where this hook is used
> to check parsing, in which errors will be emitted on a failed parse
> rather than warnings. A refactor of the riscv parsing logic would be
> required to enable this functionality fully.
>
> This fixes PR 118339 where parse failures could cause ICE in Aarch64.
>
> gcc/ChangeLog:
>
> PR target/118339
> * target.def: Add check_target_clone_version hook.
> * tree.cc (get_clone_attr_versions): Add filter argument.
> (get_clone_versions): Add filter argument.
> * tree.h (get_clone_attr_versions): Add filter.
> (get_clone_versions): Add filter argument.
> * config/aarch64/aarch64.cc (aarch64_check_target_clone_version):
> New function
> (TARGET_CHECK_TARGET_CLONE_VERSION): New define.
> * config/riscv/riscv.cc (riscv_check_target_clone_version):
> New function.
> (TARGET_CHECK_TARGET_CLONE_VERSION): New define.
> * doc/tm.texi: Regenerated.
> * doc/tm.texi.in: Add documentation for new hook.
> * hooks.h (hook_stringslice_locationtptr_true): New function.
> * hooks.cc (hook_stringslice_locationtptr_true): New function.
>
> gcc/c-family/ChangeLog:
>
> * c-attribs.cc (handle_target_clones_attribute): Update to use new hook.
> ---
> gcc/c-family/c-attribs.cc | 23 +++++++++++---
> gcc/config/aarch64/aarch64.cc | 57 +++++++++++++++++++++++++++++++++++
> gcc/config/riscv/riscv.cc | 25 +++++++++++++++
> gcc/doc/tm.texi | 9 ++++++
> gcc/doc/tm.texi.in | 2 ++
> gcc/hooks.cc | 8 +++++
> gcc/hooks.h | 3 ++
> gcc/target.def | 13 ++++++++
> gcc/tree.cc | 17 ++++++++---
> gcc/tree.h | 13 +++++---
> 10 files changed, 156 insertions(+), 14 deletions(-)
>
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 041a00479bd..86d2b60c83a 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -6249,12 +6249,25 @@ handle_target_clones_attribute (tree *node, tree
> name, tree ARG_UNUSED (args),
> }
> }
>
> - auto_vec<string_slice> versions = get_clone_attr_versions (args, NULL);
> -
> - if (versions.length () == 1)
> + int num_defaults = 0;
> + auto_vec<string_slice> versions = get_clone_attr_versions
> + (args,
> + &num_defaults,
> + false);
> +
> + for (auto v : versions)
> + targetm.check_target_clone_version
> + (v, &DECL_SOURCE_LOCATION (*node));
No need for the line break here.
> +
> + /* Lone target_clones version is always ignored for target attr
> semantics.
> + Only ignore under target_version semantics if it is a default
> + version. */
> + if (versions.length () == 1
> + && (TARGET_HAS_FMV_TARGET_ATTRIBUTE || num_defaults == 1))
> {
> - warning (OPT_Wattributes,
> - "single %<target_clones%> attribute is ignored");
> + if (TARGET_HAS_FMV_TARGET_ATTRIBUTE)
> + warning (OPT_Wattributes,
> + "single %<target_clones%> attribute is ignored");
> *no_add_attrs = true;
> }
> else
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 78db3a436a7..5415af2a73b 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -31886,6 +31886,60 @@ aarch64_expand_fp_spaceship (rtx dest, rtx op0, rtx
> op1, rtx hint)
> }
> }
>
> +/* Checks if the function version string STR parses correctly.
> + If it is an invalid string, returns true to reject the string, otherwise
> + returns false.
> + If loc is not NULL and the string is invalid, emits a warning. */
This is summarising the documentation for the hook, rather than
describing something specific to aarch64. It might be better using
the usual boilerplate:
/* Implement TARGET_CHECK_TARGET_CLONE_VERSION. */
so that there is only one place that needs updating if we want to
improve the documentation later.
> +
> +bool
> +aarch64_check_target_clone_version (string_slice str, location_t *loc)
> +{
> + str = str.strip ();
> +
> + if (str == "default")
> + return true;
> +
> + enum aarch_parse_opt_result parse_res;
> + auto isa_flags = aarch64_asm_isa_flags;
> + std::string invalid_extension;
> +
> + parse_res
> + = aarch64_parse_fmv_features (str, &isa_flags, NULL, &invalid_extension);
> +
> + if (loc == NULL)
> + return parse_res == AARCH_PARSE_OK;
> +
> + switch (parse_res)
> + {
> + case AARCH_PARSE_OK:
> + return true;
> + case AARCH_PARSE_MISSING_ARG:
> + warning_at (*loc, OPT_Wattributes,
> + "empty string not valid for a %<target_clones%> version");
> + break;
> + case AARCH_PARSE_INVALID_FEATURE:
> + warning_at (*loc, OPT_Wattributes,
> + "invalid feature modifier %qs in version %qB for "
> + "%<target_clones%> attribute",
> + invalid_extension.c_str (), &str);
> + break;
> + case AARCH_PARSE_DUPLICATE_FEATURE:
> + warning_at (*loc, OPT_Wattributes,
> + "duplicate feature modifier %qs in version %qB for "
> + "%<target_clones%> attribute",
> + invalid_extension.c_str (), &str);
> + break;
> + case AARCH_PARSE_INVALID_ARG:
> + warning_at (*loc, OPT_Wattributes,
> + "invalid feature %qs in version %qB for "
> + "%<target_clones%> attribute",
> + invalid_extension.c_str (), &str);
> + break;
> + gcc_unreachable ();
> + }
> + return false;
What I meant in the previous review was that this might be better as:
switch (parse_res)
{
case AARCH_PARSE_OK:
return true;
case AARCH_PARSE_MISSING_ARG:
warning_at (*loc, OPT_Wattributes,
"empty string not valid for a %<target_clones%> version");
return false;
case AARCH_PARSE_INVALID_FEATURE:
warning_at (*loc, OPT_Wattributes,
"invalid feature modifier %qs in version %qB for "
"%<target_clones%> attribute",
invalid_extension.c_str (), &str);
return false;
case AARCH_PARSE_DUPLICATE_FEATURE:
warning_at (*loc, OPT_Wattributes,
"duplicate feature modifier %qs in version %qB for "
"%<target_clones%> attribute",
invalid_extension.c_str (), &str);
return false;
case AARCH_PARSE_INVALID_ARG:
warning_at (*loc, OPT_Wattributes,
"invalid feature %qs in version %qB for "
"%<target_clones%> attribute",
invalid_extension.c_str (), &str);
return false;
}
gcc_unreachable ();
> +}
> +
> /* Target-specific selftests. */
>
> #if CHECKING_P
> @@ -32743,6 +32797,9 @@ aarch64_libgcc_floating_mode_supported_p
> #undef TARGET_OPTION_FUNCTION_VERSIONS
> #define TARGET_OPTION_FUNCTION_VERSIONS aarch64_common_function_versions
>
> +#undef TARGET_CHECK_TARGET_CLONE_VERSION
> +#define TARGET_CHECK_TARGET_CLONE_VERSION aarch64_check_target_clone_version
> +
> #undef TARGET_COMPARE_VERSION_PRIORITY
> #define TARGET_COMPARE_VERSION_PRIORITY aarch64_compare_version_priority
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 6bbbe65b32f..419eec7ed16 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -19,6 +19,7 @@ You should have received a copy of the GNU General Public
> License
> along with GCC; see the file COPYING3. If not see
> <http://www.gnu.org/licenses/>. */
>
> +#include "input.h"
> #define IN_TARGET_CODE 1
>
> #define INCLUDE_STRING
> @@ -14065,6 +14066,27 @@ riscv_common_function_versions (tree fn1, tree fn2)
> return riscv_compare_version_priority (fn1, fn2) != 0;
> }
>
> +/* Checks if the function version specifying string STR parses correctly.
> + If it is an invalid string, currently emits a diagnostic at LOC.
> + Always returns true. */
> +
> +bool
> +riscv_check_target_clone_version (string_slice str, location_t *loc_p)
> +{
> + struct riscv_feature_bits mask;
> + int prio;
> +
> + location_t loc = loc_p ? *loc_p : UNKNOWN_LOCATION;
> +
> + /* Currently it is not possible to parse without emitting errors on failure
> + so do not reject on a failed parse, as this would then emit two
> + diagnostics. Instead let errors be emitted which will halt
> + compilation. */
> + parse_features_for_version (str, loc, mask, prio);
FWIW, the patch I posted should allow loc_p to be passed directly.
> +
> + return true;
> +}
> +
> /* Implement TARGET_MANGLE_DECL_ASSEMBLER_NAME, to add function
> multiversioning
> suffixes. */
>
> @@ -15791,6 +15813,9 @@ synthesize_add (rtx operands[3])
> #undef TARGET_COMPARE_VERSION_PRIORITY
> #define TARGET_COMPARE_VERSION_PRIORITY riscv_compare_version_priority
>
> +#undef TARGET_CHECK_TARGET_CLONE_VERSION
> +#define TARGET_CHECK_TARGET_CLONE_VERSION riscv_check_target_clone_version
> +
> #undef TARGET_OPTION_FUNCTION_VERSIONS
> #define TARGET_OPTION_FUNCTION_VERSIONS riscv_common_function_versions
>
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 4c338c382ad..b37708d9be9 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -12289,6 +12289,15 @@ function version at run-time for a given set of
> function versions.
> body must be generated.
> @end deftypefn
>
> +@deftypefn {Target Hook} bool TARGET_CHECK_TARGET_CLONE_VERSION
> (string_slice @var{str}, location_t *@var{loc})
> +This hook is used to check if a version specified in a @code{target_clones}
> +annotation is valid. @var{str} is the version to be considered.
> +If @var{loc} is not @code{NULL} then emit warnings for invalid errors at
> +that location.
"invalid errors" doesn't sound right. Maybe "emit diagnostics for
invalid vversions". Probably worth adding ", otherwise emit no
diagnostics" to the end.
OK with those changes, thanks.
Richard
> +Returns @code{true} if @var{str} is a valid version string, and @code{false}
> +otherwise
> +@end deftypefn
> +
> @deftypefn {Target Hook} bool TARGET_PREDICT_DOLOOP_P (class loop
> *@var{loop})
> Return true if we can predict it is possible to use a low-overhead loop
> for a particular loop. The parameter @var{loop} is a pointer to the loop.
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 12b8ed660a0..c2e7921192c 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -7897,6 +7897,8 @@ to by @var{ce_info}.
>
> @hook TARGET_GENERATE_VERSION_DISPATCHER_BODY
>
> +@hook TARGET_CHECK_TARGET_CLONE_VERSION
> +
> @hook TARGET_PREDICT_DOLOOP_P
>
> @hook TARGET_HAVE_COUNT_REG_DECR_P
> diff --git a/gcc/hooks.cc b/gcc/hooks.cc
> index 76cb593103d..865820d80b1 100644
> --- a/gcc/hooks.cc
> +++ b/gcc/hooks.cc
> @@ -585,3 +585,11 @@ hook_optmode_mode_uhwi_none (machine_mode, unsigned
> HOST_WIDE_INT)
> {
> return opt_machine_mode ();
> }
> +
> +/* Generic hook that takes a string_slice and a locations and returns false.
> */
> +
> +bool
> +hook_stringslice_locationtptr_true (string_slice, location_t *)
> +{
> + return true;
> +}
> diff --git a/gcc/hooks.h b/gcc/hooks.h
> index e95bd11aca8..08542d7a24e 100644
> --- a/gcc/hooks.h
> +++ b/gcc/hooks.h
> @@ -137,4 +137,7 @@ extern const char
> *hook_constcharptr_int_const_tree_const_tree_null (int, const_
>
> extern opt_machine_mode hook_optmode_mode_uhwi_none (machine_mode,
> unsigned HOST_WIDE_INT);
> +
> +extern bool hook_stringslice_locationtptr_true (string_slice, location_t *);
> +
> #endif
> diff --git a/gcc/target.def b/gcc/target.def
> index 5dd8f253ef6..c4912622f67 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -2606,6 +2606,19 @@ version at run-time. @var{decl} is one version from a
> set of semantically\n\
> identical versions.",
> tree, (void *decl), NULL)
>
> +/* Target hook is used to ignore certain versions specified in a
> target_clones
> + annoration. STR is the version to be considered. */
> +DEFHOOK
> +(check_target_clone_version ,
> + "This hook is used to check if a version specified in a
> @code{target_clones}\n\
> +annotation is valid. @var{str} is the version to be considered.\n\
> +If @var{loc} is not @code{NULL} then emit warnings for invalid errors at\n\
> +that location.\n\
> +Returns @code{true} if @var{str} is a valid version string, and
> @code{false}\n\
> +otherwise",
> + bool, (string_slice str, location_t *loc),
> + hook_stringslice_locationtptr_true)
> +
> /* Returns a code for a target-specific builtin that implements
> reciprocal of a target-specific function, or NULL_TREE if not available.
> */
> DEFHOOK
> diff --git a/gcc/tree.cc b/gcc/tree.cc
> index ad381091820..4e1d522827e 100644
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -15398,10 +15398,13 @@ get_attr_nonstring_decl (tree expr, tree *ref)
> }
>
> /* Returns an auto_vec of string_slices containing the version strings from
> - ARGLIST. DEFAULT_COUNT is incremented for each default version found. */
> + ARGLIST. DEFAULT_COUNT is incremented for each default version found.
> + If FILTER is true then any invalid versions strings are not included. */
>
> auto_vec<string_slice>
> -get_clone_attr_versions (const tree arglist, int *default_count)
> +get_clone_attr_versions (const tree arglist,
> + int *default_count,
> + bool filter)
> {
> gcc_assert (TREE_CODE (arglist) == TREE_LIST);
> auto_vec<string_slice> versions;
> @@ -15417,6 +15420,9 @@ get_clone_attr_versions (const tree arglist, int
> *default_count)
> string_slice attr = string_slice::tokenize (&str, separators);
> attr = attr.strip ();
>
> + if (filter && !targetm.check_target_clone_version (attr, NULL))
> + continue;
> +
> if (attr == "default" && default_count)
> (*default_count)++;
> versions.safe_push (attr);
> @@ -15427,15 +15433,16 @@ get_clone_attr_versions (const tree arglist, int
> *default_count)
>
> /* Returns an auto_vec of string_slices containing the version strings from
> the target_clone attribute from DECL. DEFAULT_COUNT is incremented for
> each
> - default version found. */
> + default version found. If FILTER is true then any invalid versions
> strings
> + are not included. */
> auto_vec<string_slice>
> -get_clone_versions (const tree decl, int *default_count)
> +get_clone_versions (const tree decl, int *default_count, bool filter)
> {
> tree attr = lookup_attribute ("target_clones", DECL_ATTRIBUTES (decl));
> if (!attr)
> return auto_vec<string_slice> ();
> tree arglist = TREE_VALUE (attr);
> - return get_clone_attr_versions (arglist, default_count);
> + return get_clone_attr_versions (arglist, default_count, filter);
> }
>
> /* If DECL has a target_version attribute, returns a string_slice containing
> the
> diff --git a/gcc/tree.h b/gcc/tree.h
> index b67331aef74..f773ea1fef1 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -7101,10 +7101,15 @@ extern tree get_attr_nonstring_decl (tree, tree * =
> NULL);
> Returns an invalid string_slice if no attribute is present. */
> extern string_slice get_target_version (const tree);
> /* Returns a vector of the version strings from a target_clones attribute on
> - a decl. Can also record the number of default versions found. */
> -extern auto_vec<string_slice> get_clone_versions (const tree, int * = NULL);
> + a decl. Can also record the number of default versions found.
> + Use bool to control whether or not the results should
> + be filtered with TARGET_CHECK_TARGET_CLONE_VERSION. */
> +extern auto_vec<string_slice> get_clone_versions
> + (const tree,int * = NULL, bool = true);
> /* Returns a vector of the version strings from a target_clones attribute
> - directly. */
> -extern auto_vec<string_slice> get_clone_attr_versions (const tree, int *);
> + directly. Additionally takes a bool to control whether or not the results
> + should be filtered with TARGET_CHECK_TARGET_CLONE_VERSION. */
> +extern auto_vec<string_slice> get_clone_attr_versions
> + (const tree, int *, bool = true);
>
> #endif /* GCC_TREE_H */
