Alfie Richards <alfie.richa...@arm.com> writes:
> This patch is an overhaul of how FMV name mangling works. Previously
> mangling logic was duplicated in several places across both target
> specific and independent code. This patch changes this such that all
> mangling is done in targetm.mangle_decl_assembler_name (including for the
> dispatched symbol and dispatcher resolver).
>
> This allows for the removing of previous hacks, such as where the default
> mangled decl's assembler name was unmangled to then remangle all versions
> and the resolver and dispatched symbol.
>
> This does introduce a change though (shown in test changes) where
> previously x86 for target annotated FMV sets set the function name to
> the assembler name and remangled this. This was hard to reproduce without
> resorting to hacks I wasn't comfortable with so the mangling is changed
> to append ".ifunc" which matches clang.
>
> This change also refactors expand_target_clone using
> targetm.mangle_decl_assembler_name for mangling and get_clone_versions.
>
> gcc/ChangeLog:
>
> * attribs.cc (make_dispatcher_decl): Refactor to use
> targetm.mangle_decl_assembler_name for mangling.
> * config/aarch64/aarch64.cc (aarch64_parse_fmv_features): Change to
> support string_slice.
> (aarch64_process_target_version_attr): Ditto.
> (get_feature_mask_for_version): Ditto.
> (aarch64_mangle_decl_assembler_name): Refactor to handle FMV dispatched
> symbol and resolver.
> (get_suffixed_assembler_name): Removed.
> (make_resolver_func): Refactor to use
> aarch64_mangle_decl_assembler_name for mangling.
> (aarch64_generate_version_dispatcher_body): Ditto.
> * config/i386/i386-features.cc (is_valid_asm_symbol): Moved from
> multiple_target.cc.
> (create_new_asm_name): Moved from gcc/multiple_target.cc.
> (ix86_mangle_function_version_assembler_name): Refactor to handle FMV
> dispatched symbol and resolver.
> (ix86_mangle_decl_assembler_name): Ditto.
> (ix86_get_function_versions_dispatcher): Refactor to use
> ix86_mangle_decl_assembler_name for mangling.
> (make_resolver_func): Ditto.
> * config/riscv/riscv.cc (riscv_mangle_decl_assembler_name): Refactor to
> handle FMV dispatched symbol and resolver.
> (get_suffixed_assembler_name): Removed.
> (make_resolver_func): Refactor to use riscv_mangle_decl_assembler_name
> for mangling.
> (riscv_generate_version_dispatcher_body): Ditto.
> * config/rs6000/rs6000.cc (rs6000_mangle_decl_assembler_name): Refactor
> to handle FMV dispatched symbol and resolver.
> (make_resolver_func): Refactor to use
> rs6000_mangle_function_version_assembler_name for mangling.
> (is_valid_asm_symbol): Moved from gcc/multiple_target.cc.
> (create_new_asm_name): Ditto.
> (rs6000_mangle_function_version_assembler_name): Refactor to handle FMV
> dispatched symbol and resolver.
> * multiple_target.cc (create_dispatcher_calls): Refactored to use
> targetm.mangle_decl_assembler_name for mangling.
> (is_valid_asm_symbol): Moved to target specific code.
> (create_new_asm_name): Ditto.
> (expand_target_clones): Refactored to use
> targetm.mangle_decl_assembler_name for mangling.
>
> gcc/cp/ChangeLog:
>
> * decl.cc (duplicate_decls): Added logic to remangle FMV decls when
> merging.
>
> gcc/testsuite/ChangeLog:
>
> * g++.target/i386/mv-symbols1.C: Change FMV mangling.
> * g++.target/i386/mv-symbols3.C: Ditto.
> * g++.target/i386/mv-symbols4.C: Ditto.
> * g++.target/i386/mv-symbols5.C: Ditto.
> ---
> gcc/attribs.cc | 25 +++-
> gcc/config/aarch64/aarch64.cc | 141 ++++++++-----------
> gcc/config/i386/i386-features.cc | 90 +++++++++---
> gcc/config/riscv/riscv.cc | 95 ++++++-------
> gcc/config/rs6000/rs6000.cc | 104 +++++++++++++-
> gcc/cp/decl.cc | 13 ++
> gcc/multiple_target.cc | 146 +++++++++-----------
> gcc/testsuite/g++.target/i386/mv-symbols1.C | 12 +-
> gcc/testsuite/g++.target/i386/mv-symbols3.C | 10 +-
> gcc/testsuite/g++.target/i386/mv-symbols4.C | 10 +-
> gcc/testsuite/g++.target/i386/mv-symbols5.C | 10 +-
> 11 files changed, 375 insertions(+), 281 deletions(-)
The attribs.cc part looks good to me. Some comments about the aarch64
part below. I'm not really qualified to review the rest.
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 15dd7dda48a..420bbba9be2 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -19862,7 +19862,7 @@ static aarch64_fmv_feature_datum
> aarch64_fmv_feature_data[] = {
> #include "config/aarch64/aarch64-option-extensions.def"
> };
>
> -/* Parse a function multiversioning feature string STR, as found in a
> +/* Parse a function multiversioning feature string_slice STR, as found in a
> target_version or target_clones attribute.
>
> If ISA_FLAGS is nonnull, then update it with the specified architecture
> @@ -19874,37 +19874,33 @@ static aarch64_fmv_feature_datum
> aarch64_fmv_feature_data[] = {
> the extension string is created and stored to INVALID_EXTENSION. */
>
> static enum aarch_parse_opt_result
> -aarch64_parse_fmv_features (const char *str, aarch64_feature_flags
> *isa_flags,
> +aarch64_parse_fmv_features (string_slice str, aarch64_feature_flags
> *isa_flags,
> aarch64_fmv_feature_mask *feature_mask,
> std::string *invalid_extension)
> {
> if (feature_mask)
> *feature_mask = 0ULL;
>
> - if (strcmp (str, "default") == 0)
> + if (strcmp (str, string_slice ("default")) == 0)
Hopefully just str == "default", but at least not more than
str == string_slice ("default")
> return AARCH_PARSE_OK;
>
> - while (str != NULL && *str != 0)
> + while (str.is_valid ())
> {
> - const char *ext;
> - size_t len;
> + string_slice ext;
>
> - ext = strchr (str, '+');
> + ext = string_slice::strtok (&str, string_slice ("+"));
>
> - if (ext != NULL)
> - len = ext - str;
> - else
> - len = strlen (str);
> -
> - if (len == 0)
> + // This doesnt seem to actually work :(
> + if (!ext.is_valid () || ext.empty ())
> return AARCH_PARSE_MISSING_ARG;
>
> int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
> int i;
> for (i = 0; i < num_features; i++)
> {
> - if (strlen (aarch64_fmv_feature_data[i].name) == len
> - && strncmp (aarch64_fmv_feature_data[i].name, str, len) == 0)
> + if (strlen (aarch64_fmv_feature_data[i].name) == ext.size ()
> + && strcmp (string_slice (aarch64_fmv_feature_data[i].name), ext)
> + == 0)
Hopefully this could just be:
if (ext == aarch64_fmv_feature_data[i].name)
> {
> if (isa_flags)
> *isa_flags |= aarch64_fmv_feature_data[i].opt_flags;
> @@ -19916,7 +19912,8 @@ aarch64_parse_fmv_features (const char *str,
> aarch64_feature_flags *isa_flags,
> {
> /* Duplicate feature. */
> if (invalid_extension)
> - *invalid_extension = std::string (str, len);
> + *invalid_extension
> + = std::string (ext.front (), ext.size ());
> return AARCH_PARSE_DUPLICATE_FEATURE;
> }
> }
> @@ -19928,14 +19925,9 @@ aarch64_parse_fmv_features (const char *str,
> aarch64_feature_flags *isa_flags,
> {
> /* Feature not found in list. */
> if (invalid_extension)
> - *invalid_extension = std::string (str, len);
> + *invalid_extension = std::string (str.front (), str.size ());
> return AARCH_PARSE_INVALID_FEATURE;
> }
> -
> - str = ext;
> - if (str)
> - /* Skip over the next '+'. */
> - str++;
> }
>
> return AARCH_PARSE_OK;
> @@ -19978,7 +19970,7 @@ aarch64_process_target_version_attr (tree args)
> auto isa_flags = aarch64_asm_isa_flags;
>
> std::string invalid_extension;
> - parse_res = aarch64_parse_fmv_features (str, &isa_flags, NULL,
> + parse_res = aarch64_parse_fmv_features (string_slice (str), &isa_flags,
> NULL,
> &invalid_extension);
I suppose this is personal preference, but: IMO it would be cleaner
without the explicit string_slice constructor. Same elsewhere.
>
> if (parse_res == AARCH_PARSE_OK)
> @@ -20079,8 +20071,8 @@ get_feature_mask_for_version (tree decl)
> enum aarch_parse_opt_result parse_res;
> aarch64_fmv_feature_mask feature_mask;
>
> - parse_res = aarch64_parse_fmv_features (version_string, NULL,
> &feature_mask,
> - NULL);
> + parse_res = aarch64_parse_fmv_features (string_slice (version_string),
> NULL,
> + &feature_mask, NULL);
>
> /* We should have detected any errors before getting here. */
> gcc_assert (parse_res == AARCH_PARSE_OK);
> @@ -20175,36 +20167,39 @@ tree
> aarch64_mangle_decl_assembler_name (tree decl, tree id)
> {
> /* For function version, add the target suffix to the assembler name. */
> - if (TREE_CODE (decl) == FUNCTION_DECL
> - && DECL_FUNCTION_VERSIONED (decl))
> + if (TREE_CODE (decl) == FUNCTION_DECL)
> {
> - aarch64_fmv_feature_mask feature_mask = get_feature_mask_for_version
> (decl);
> -
> - std::string name = IDENTIFIER_POINTER (id);
> -
> - /* For the default version, append ".default". */
> - if (feature_mask == 0ULL)
> + if (cgraph_node::get_create (decl)->dispatcher_function)
Similarly to a comment in an earlier patch in the series: this is
worth caching, rather than calling multiple times.
> + return id;
> + else if (cgraph_node::get_create (decl)->dispatcher_resolver_function)
> + id = clone_identifier (id, "resolver");
> + else if (DECL_FUNCTION_VERSIONED (decl))
> {
> - name += ".default";
> - return get_identifier (name.c_str());
> - }
> -
> - name += "._";
> + aarch64_fmv_feature_mask feature_mask
> + = get_feature_mask_for_version (decl);
>
> - int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
> - for (int i = 0; i < num_features; i++)
> - {
> - if (feature_mask & aarch64_fmv_feature_data[i].feature_mask)
> + if (feature_mask == 0ULL) // && not implemented!
Could you explain the // comment?
> {
> - name += "M";
> - name += aarch64_fmv_feature_data[i].name;
> + if (!DECL_INITIAL (decl))
> + return id;
> + return clone_identifier (id, "default");
This different handling of defined vs. undefined functions feels a
bit weird. It's not clear to me whether:
(1) The .default version is effectively internal to the translation unit,
and therefore other translation units cannot assume it exists.
(2) Other translation units can assume that the .default version exists
if they can see a target_version("default") or target_clones definition.
(3) Other translation units can assume that the .default version exists
if they can see a non-default target_version or a target_clones definition.
(4) Something else.
(2) would create a difference between implicit and explicit defaults
and doesn't seem to be what the series implements (mv-symbols6.C from
patch 14). (3) seems indirect, and IMO it's dangerous to assume that
the mere existence of a non-default version X in one TU implies that
version X will be visible in the TU that contains the resolver. I would
have expected that it would be valid for version X not to be visible
in the TU that contains the resolver, with the consequence that the
resolver won't use it.
(1) seems more appealing on the face of it, so that .default is more
like .resolver. But that doesn't seem to be what the spec says.
> }
> - }
>
> - if (DECL_ASSEMBLER_NAME_SET_P (decl))
> - SET_DECL_RTL (decl, NULL);
> + std::string suffix = "_";
> +
> + int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
> + for (int i = 0; i < num_features; i++)
> + if (feature_mask & aarch64_fmv_feature_data[i].feature_mask)
> + {
> + suffix += "M";
> + suffix += aarch64_fmv_feature_data[i].name;
> + }
> +
> + if (DECL_ASSEMBLER_NAME_SET_P (decl))
> + SET_DECL_RTL (decl, NULL);
Why is it necessary to conditionally reset the DECL_RTL for the
non-default case but not necessary when adding ".default"?
Thanks,
Richard
>
> - id = get_identifier (name.c_str());
> + id = clone_identifier (id, suffix.c_str ());
> + }
> }
> return id;
> }
> @@ -20213,18 +20208,6 @@ aarch64_mangle_decl_assembler_name (tree decl, tree
> id)
> This is computed by taking the default version's assembler name, and
> stripping off the ".default" suffix if it's already been appended. */
>
> -static tree
> -get_suffixed_assembler_name (tree default_decl, const char *suffix)
> -{
> - std::string name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (default_decl));
> -
> - auto size = name.size ();
> - if (size >= 8 && name.compare (size - 8, 8, ".default") == 0)
> - name.resize (size - 8);
> - name += suffix;
> - return get_identifier (name.c_str());
> -}
> -
> /* Make the resolver function decl to dispatch the versions of
> a multi-versioned function, DEFAULT_DECL. IFUNC_ALIAS_DECL is
> ifunc alias that will point to the created resolver. Create an
> @@ -20240,8 +20223,6 @@ make_resolver_func (const tree default_decl,
>
> /* Create resolver function name based on default_decl. We need to remove
> an
> existing ".default" suffix if this has already been appended. */
> - tree decl_name = get_suffixed_assembler_name (default_decl, ".resolver");
> - const char *resolver_name = IDENTIFIER_POINTER (decl_name);
>
> /* The resolver function should have signature
> (void *) resolver (uint64_t, const __ifunc_arg_t *) */
> @@ -20250,10 +20231,17 @@ make_resolver_func (const tree default_decl,
> build_ifunc_arg_type (),
> NULL_TREE);
>
> - decl = build_fn_decl (resolver_name, type);
> - SET_DECL_ASSEMBLER_NAME (decl, decl_name);
> + cgraph_node *node = cgraph_node::get (default_decl);
> + gcc_assert (node && node->function_version ());
> +
> + decl = build_fn_decl (IDENTIFIER_POINTER (DECL_NAME (default_decl)), type);
> + cgraph_node *resolver_node = cgraph_node::get_create (decl);
> + resolver_node->dispatcher_resolver_function = true;
> +
> + tree id = aarch64_mangle_decl_assembler_name (
> + decl, node->function_version ()->assembler_name);
> + symtab->change_decl_assembler_name (decl, id);
>
> - DECL_NAME (decl) = decl_name;
> TREE_USED (decl) = 1;
> DECL_ARTIFICIAL (decl) = 1;
> DECL_IGNORED_P (decl) = 1;
> @@ -20318,7 +20306,7 @@ make_resolver_func (const tree default_decl,
> gcc_assert (ifunc_alias_decl != NULL);
> /* Mark ifunc_alias_decl as "ifunc" with resolver as resolver_name. */
> DECL_ATTRIBUTES (ifunc_alias_decl)
> - = make_attribute ("ifunc", resolver_name,
> + = make_attribute ("ifunc", IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME
> (decl)),
> DECL_ATTRIBUTES (ifunc_alias_decl));
>
> /* Create the alias for dispatch to resolver here. */
> @@ -20595,27 +20583,6 @@ aarch64_generate_version_dispatcher_body (void
> *node_p)
> cgraph_edge::rebuild_edges ();
> pop_cfun ();
>
> - /* Fix up symbol names. First we need to obtain the base name, which may
> - have already been mangled. */
> - tree base_name = get_suffixed_assembler_name (default_ver_decl, "");
> -
> - /* We need to redo the version mangling on the non-default versions for the
> - target_clones case. Redoing the mangling for the target_version case is
> - redundant but does no harm. We need to skip the default version,
> because
> - expand_clones will append ".default" later; fortunately that suffix is
> the
> - one we want anyway. */
> - for (versn_info = node_version_info->next->next; versn_info;
> - versn_info = versn_info->next)
> - {
> - tree version_decl = versn_info->this_node->decl;
> - tree name = aarch64_mangle_decl_assembler_name (version_decl,
> - base_name);
> - symtab->change_decl_assembler_name (version_decl, name);
> - }
> -
> - /* We also need to use the base name for the ifunc declaration. */
> - symtab->change_decl_assembler_name (node->decl, base_name);
> -
> return resolver_decl;
> }
>
