<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 and location argument.
> (get_clone_versions): Add filter argument.
> * tree.h (get_clone_attr_versions): Add filter and location argument.
> (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_FUNCTION_CLONE_VERSION): New define.
> * doc/tm.texi: Regenerated.
> * doc/tm.texi.in: Add documentation for new hook.
> * hooks.h (hook_stringslice_locationt_boolean_false): New function.
> * hooks.cc (hook_stringslice_locationt_boolean_false): New function.
>
> gcc/c-family/ChangeLog:
>
> * c-attribs.cc (handle_target_clones_attribute): Update to use new hook.
> ---
> gcc/c-family/c-attribs.cc | 24 ++++++++++++----
> gcc/config/aarch64/aarch64.cc | 53 +++++++++++++++++++++++++++++++++++
> gcc/config/riscv/riscv.cc | 22 +++++++++++++++
> gcc/doc/tm.texi | 5 ++++
> gcc/doc/tm.texi.in | 2 ++
> gcc/hooks.cc | 8 ++++++
> gcc/hooks.h | 3 ++
> gcc/target.def | 9 ++++++
> gcc/tree.cc | 15 ++++++++--
> gcc/tree.h | 15 +++++++---
> 10 files changed, 144 insertions(+), 12 deletions(-)
>
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index a0d832b5e05..ce31fa2fcaf 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -6238,12 +6238,26 @@ 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,
> + DECL_SOURCE_LOCATION (*node),
> + false);
> +
> + for (auto v : versions)
> + targetm.check_target_clone_version
> + (v, DECL_SOURCE_LOCATION (*node), true);
> +
> + /* 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 44b14fc6f45..2029a851396 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -31807,6 +31807,56 @@ 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. */
> +
> +bool
> +aarch64_check_target_clone_version (string_slice str,
> + location_t loc,
> + bool emit_diag)
Sorry to be awkward, but how about passing a location_t * instead
of separate location_t and bool parameters, with nullptr meaning no
diagnostic? I know that isn't what I suggested last time, but seeing
the interface in action, it seemed a bit clunky that a location was
required even when a diagnostic wasn't.
I realise that'll involve more changes to the RISC-V code. I'm happy
to do that if you'd prefer, since it's a sort of feature creep.
> +{
> + 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 (!emit_diag)
> + 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;
> + default:
> + gcc_unreachable ();
> + }
> + return false;
I think for this kind of thing it's better to make the cases return false,
to remove the default, and to put the gcc_unreachable () after the switch
statement. That way we'll get a warning if there's a missing case.
> +}
> +
> /* Target-specific selftests. */
>
> #if CHECKING_P
> @@ -32664,6 +32714,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 38fce5519da..90fd7d95c57 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -14153,6 +14153,25 @@ 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 false. */
> +
> +bool
> +riscv_verify_target_clone_version (string_slice str, location_t loc)
> +{
> + struct riscv_feature_bits mask;
> + int prio;
> +
> + /* 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);
> +
> + return true;
> +}
> +
> /* Implement TARGET_MANGLE_DECL_ASSEMBLER_NAME, to add function
> multiversioning
> suffixes. */
>
> @@ -15785,6 +15804,9 @@ synthesize_and (rtx operands[3])
> #undef TARGET_COMPARE_VERSION_PRIORITY
> #define TARGET_COMPARE_VERSION_PRIORITY riscv_compare_version_priority
>
> +#undef TARGET_VERIFY_TARGET_CLONE_VERSION
> +#define TARGET_VERIFY_TARGET_CLONE_VERSION riscv_check_target_clone_version
> +
s/VERIFY/CHECK/ in the macro name and s/verify/check/ in the function
definition. The interface above is missing the bool parameter.
> #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 4d4e676aadf..e91a1550ff8 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -12279,6 +12279,11 @@ 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}, @var{bool})
> +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.
> +@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 1a51ad54817..eaab62652a7 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -7887,6 +7887,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..640759881d3 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_locationt_bool_true (string_slice, location_t, bool)
> +{
> + return true;
> +}
> diff --git a/gcc/hooks.h b/gcc/hooks.h
> index e95bd11aca8..038598499a6 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_locationt_bool_true (string_slice, location_t,
> bool);
> +
> #endif
> diff --git a/gcc/target.def b/gcc/target.def
> index 5dd8f253ef6..96643612fb5 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -2606,6 +2606,15 @@ 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.",
The other parameters should be documented too, as well as the return value.
> + bool, (string_slice str, location_t loc, bool),
> + hook_stringslice_locationt_bool_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 2335a6993b9..a50c2f5163b 100644
> --- a/gcc/tree.cc
> +++ b/gcc/tree.cc
> @@ -15398,7 +15398,10 @@ get_attr_nonstring_decl (tree expr, tree *ref)
> ARGLIST. DEFAULT_COUNT is incremented for each default version found. */
>
> auto_vec<string_slice>
> -get_clone_attr_versions (const tree arglist, int *default_count)
> +get_clone_attr_versions (const tree arglist,
> + int *default_count,
> + location_t loc,
> + bool filter)
The function comment should document FILTER.
> {
> gcc_assert (TREE_CODE (arglist) == TREE_LIST);
> auto_vec<string_slice> versions;
> @@ -15414,6 +15417,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, loc, false))
> + continue;
> +
> if (attr == "default" && default_count)
> (*default_count)++;
> versions.safe_push (attr);
> @@ -15426,13 +15432,16 @@ get_clone_attr_versions (const tree arglist, int
> *default_count)
> the target_clone attribute from DECL. DEFAULT_COUNT is incremented for
> each
> default version found. */
> auto_vec<string_slice>
> -get_clone_versions (const tree decl, int *default_count)
> +get_clone_versions (const tree decl, int *default_count, bool filter)
Same here.
Thanks,
Richard
> {
> 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,
> + DECL_SOURCE_LOCATION (decl),
> + 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 eadf0603066..c239b3505e5 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -7091,10 +7091,17 @@ 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 location for potential diagnostics to be
> + emitted for and 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 *,
> + location_t loc,
> + bool = true);
>
> #endif /* GCC_TREE_H */
