[v1,12/16] Refactor FMV name mangling.

Message ID 20250203130421.2192732-14-alfie.richards@arm.com
State New
Headers
Series FMV refactor and ACLE compliance. |

Checks

Context Check Description
rivoscibot/toolchain-ci-rivos-apply-patch success Patch applied
rivoscibot/toolchain-ci-rivos-lint success Lint passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gcv-lp64d-multilib fail Build failed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gc_zba_zbb_zbc_zbs-lp64d-multilib fail Build failed
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gcv-lp64d-multilib fail Build failed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Build failed

Commit Message

Alfie Richards Feb. 3, 2025, 1:04 p.m. UTC
  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(-)
  

Comments

Richard Sandiford Feb. 4, 2025, 10:03 a.m. UTC | #1
Alfie Richards <alfie.richards@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;
>  }
>
  
Alfie Richards Feb. 5, 2025, 1:36 p.m. UTC | #2
On 04/02/2025 10:03, Richard Sandiford wrote:
> Alfie Richards <alfie.richards@arm.com> writes:
>> +	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?
Apologies, this was a temporary marker for myself that slipped through.
>>   	    {
>> -	      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.
I would say its (4) Something else.

This code is the result of that and a discussion we had where we decided we
can avoid mangling the default version symbol if it is external. As we 
know that
in the TU where the default is defined, the default
version will be mangled, and the dispatched symbol will take its name.

As the default is the only resolvable version, then all calls and 
references
will already have the correct target.

This allows us to avoid making the dispatched symbol and redirecting
default decl calls/references to the dispatched symbol.

It's a bit of a hack, and we can instead always mangle the default,
create the dispatched symbol? Apologies if I misunderstood the earlier
conversation.
>>   	    }
>> -	}
>>   
>> -      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
To be honest I don’t understand this code particularly well. This was here
previously, and I understood it to imply that the function needs to be
recompiled if it is non-default as it's architecture features will
have changed.I think it only really applies in the target_clones case 
where the new
decl from expand_clones could have carried over the RTL of the default?

Thanks,
Alfie
  
Richard Sandiford Feb. 5, 2025, 5:14 p.m. UTC | #3
Alfie Richards <alfie.richards@arm.com> writes:
> On 04/02/2025 10:03, Richard Sandiford wrote:
>> Alfie Richards <alfie.richards@arm.com> writes:
>>> +	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?
> Apologies, this was a temporary marker for myself that slipped through.
>>>   	    {
>>> -	      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.

Argh, sorry, I meant declaration rather than definition for both
(2) and (3).  My question doesn't make sense as written.

>> (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.
> I would say its (4) Something else.
>
> This code is the result of that and a discussion we had where we decided we
> can avoid mangling the default version symbol if it is external. As we 
> know that
> in the TU where the default is defined, the default
> version will be mangled, and the dispatched symbol will take its name.
>
> As the default is the only resolvable version, then all calls and 
> references
> will already have the correct target.
>
> This allows us to avoid making the dispatched symbol and redirecting
> default decl calls/references to the dispatched symbol.

That sounds like what I meant by (1): i.e. that the only TU that can
reference .default is the one that creates it (i.e. the one with the
definition).  A declaration with __attribute__((target_version("default"))
doesn't itself imply that a .default version exists.

The reason for asking is that the ACLE says:

  The "default" version is mangled with ".default" on top of the
  language-specific name mangling. All versioned functions with their
  mangled names are always resolvable.

which made it sound like other TUs could rely on ".default" existing.

> It's a bit of a hack, and we can instead always mangle the default,
> create the dispatched symbol? Apologies if I misunderstood the earlier
> conversation.
>
>>>   	    }
>>> -	}
>>>   
>>> -      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
> To be honest I don’t understand this code particularly well. This was here
> previously, and I understood it to imply that the function needs to be
> recompiled if it is non-default as it's architecture features will
> have changed.I think it only really applies in the target_clones case 
> where the new
> decl from expand_clones could have carried over the RTL of the default?

Resetting DECL_RTL doesn't AFAIK trigger any recompilation.
IIRC, the DECL_RTL for a function is usually:

  (mem (symbol_ref "<mangled name>"))

and so the purpose of clearing it out is to force a new symbol_ref
(with a new mangling) to be created.

It would be interesting to see whether any failures are visible with the
SET_DECL_RTL removed.  If so, it would be interesting to know whether
there's a particular reason why the same failure couldn't trigger for
the default version.  If not, then maybe the code can be removed.
(I'd expect a failure, but I'm certainly not 100% sure.)

Thanks,
Richard
  
Alfie Richards Feb. 6, 2025, 2:51 p.m. UTC | #4
Added missing CC

On 05/02/2025 17:14, Richard Sandiford wrote:
> Alfie Richards <alfie.richards@arm.com> writes:
>> On 04/02/2025 10:03, Richard Sandiford wrote:
>>> Alfie Richards <alfie.richards@arm.com> writes:
>>>> +    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?
>> Apologies, this was a temporary marker for myself that slipped through.
>>>>            {
>>>> -          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.
> Argh, sorry, I meant declaration rather than definition for both
> (2) and (3).  My question doesn't make sense as written.
>
>>> (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.
>> I would say its (4) Something else.
>>
>> This code is the result of that and a discussion we had where we 
>> decided we
>> can avoid mangling the default version symbol if it is external. As we
>> know that
>> in the TU where the default is defined, the default
>> version will be mangled, and the dispatched symbol will take its name.
>>
>> As the default is the only resolvable version, then all calls and
>> references
>> will already have the correct target.
>>
>> This allows us to avoid making the dispatched symbol and redirecting
>> default decl calls/references to the dispatched symbol.
> That sounds like what I meant by (1): i.e. that the only TU that can
> reference .default is the one that creates it (i.e. the one with the
> definition).  A declaration with 
> __attribute__((target_version("default"))
> doesn't itself imply that a .default version exists.
>
> The reason for asking is that the ACLE says:
>
>    The "default" version is mangled with ".default" on top of the
>    language-specific name mangling. All versioned functions with their
>    mangled names are always resolvable.
>
> which made it sound like other TUs could rely on ".default" existing.
Ahh that does help. I follow now.

My current understanding comes from following the clang implementation
and discussions regarding the spec.

But, I see your point though that this isn't what is specified.

I think a good correct solution would be that if there is
target_version ("default") then to always mangle the default. Then in the
case that there's a default and no other versions, emit a
non-mangled symbol alias to the default version?

Then we cover all cases well, follow the spec, and will be broadly 
compatible
with clang. (ie. in that case currently clang emits the default as if it
wasn't annotated, we would emit the default mangled, and include an alias
to the unmangled symbol)

>> It's a bit of a hack, and we can instead always mangle the default,
>> create the dispatched symbol? Apologies if I misunderstood the earlier
>> conversation.
>>
>>>>            }
>>>> -    }
>>>>    -      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
>> To be honest I don’t understand this code particularly well. This was 
>> here
>> previously, and I understood it to imply that the function needs to be
>> recompiled if it is non-default as it's architecture features will
>> have changed.I think it only really applies in the target_clones case
>> where the new
>> decl from expand_clones could have carried over the RTL of the default?
> Resetting DECL_RTL doesn't AFAIK trigger any recompilation.
> IIRC, the DECL_RTL for a function is usually:
>
>    (mem (symbol_ref "<mangled name>"))
>
> and so the purpose of clearing it out is to force a new symbol_ref
> (with a new mangling) to be created.
>
> It would be interesting to see whether any failures are visible with the
> SET_DECL_RTL removed.  If so, it would be interesting to know whether
> there's a particular reason why the same failure couldn't trigger for
> the default version.  If not, then maybe the code can be removed.
> (I'd expect a failure, but I'm certainly not 100% sure.)
Ah that's super helpful to know, thank you!
I will experimentfor what's needed.
> Thanks,
> Richard
  

Patch

diff --git a/gcc/attribs.cc b/gcc/attribs.cc
index cb25845715d..687e6d4143a 100644
--- a/gcc/attribs.cc
+++ b/gcc/attribs.cc
@@ -39,6 +39,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-pretty-print.h"
 #include "intl.h"
 #include "gcc-urlifier.h"
+#include "cgraph.h"
 
 /* Table of the tables of attributes (common, language, format, machine)
    searched.  */
@@ -1271,18 +1272,13 @@  common_function_versions (tree fn1, tree fn2)
 tree
 make_dispatcher_decl (const tree decl)
 {
-  tree func_decl;
-  char *func_name;
-  tree fn_type, func_type;
-
-  func_name = xstrdup (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)));
+  tree func_decl, fn_type, func_type;
 
   fn_type = TREE_TYPE (decl);
   func_type = build_function_type (TREE_TYPE (fn_type),
 				   TYPE_ARG_TYPES (fn_type));
 
-  func_decl = build_fn_decl (func_name, func_type);
-  XDELETEVEC (func_name);
+  func_decl = build_fn_decl (IDENTIFIER_POINTER (DECL_NAME (decl)), func_type);
   TREE_USED (func_decl) = 1;
   DECL_CONTEXT (func_decl) = NULL_TREE;
   DECL_INITIAL (func_decl) = error_mark_node;
@@ -1293,6 +1289,21 @@  make_dispatcher_decl (const tree decl)
   /* This will be of type IFUNCs have to be externally visible.  */
   TREE_PUBLIC (func_decl) = 1;
 
+  cgraph_node *node = cgraph_node::get_create (decl);
+
+  cgraph_node *func_node = cgraph_node::get_create (func_decl);
+  func_node->dispatcher_function = true;
+
+  /* If the default node is from a target_clone, mark the dispatcher as from
+     target_clone.  */
+  if (node->is_target_clone)
+    func_node->is_target_clone = true;
+
+  /* Get the assembler name by mangling with the base assembler name.  */
+  tree id = targetm.mangle_decl_assembler_name
+    (func_decl, node->function_version ()->assembler_name);
+  symtab->change_decl_assembler_name (func_decl, id);
+
   return func_decl;
 }
 
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)
     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)
 	    {
 	      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);
 
   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)
+	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!
 	    {
-	      name += "M";
-	      name += aarch64_fmv_feature_data[i].name;
+	      if (!DECL_INITIAL (decl))
+		return id;
+	      return clone_identifier (id, "default");
 	    }
-	}
 
-      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);
 
-      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;
 }
 
diff --git a/gcc/config/i386/i386-features.cc b/gcc/config/i386/i386-features.cc
index 2eb3a21bb5d..4c03a8acd4d 100644
--- a/gcc/config/i386/i386-features.cc
+++ b/gcc/config/i386/i386-features.cc
@@ -3888,6 +3888,37 @@  dispatch_function_versions (tree dispatch_decl,
   return 0;
 }
 
+/*  Return true if symbol is valid in assembler name.  */
+
+static bool
+is_valid_asm_symbol (char c)
+{
+  if ('a' <= c && c <= 'z')
+    return true;
+  if ('A' <= c && c <= 'Z')
+    return true;
+  if ('0' <= c && c <= '9')
+    return true;
+  if (c == '_')
+    return true;
+  return false;
+}
+
+/*  Replace all not valid assembler symbols with '_'.  */
+static void
+create_new_asm_name (char *old_asm_name, char *new_asm_name)
+{
+  int i;
+  int old_name_len = strlen (old_asm_name);
+  /* Replace all not valid assembler symbols with '_'.  */
+  for (i = 0; i < old_name_len; i++)
+    if (!is_valid_asm_symbol (old_asm_name[i]))
+      new_asm_name[i] = '_';
+    else
+      new_asm_name[i] = old_asm_name[i];
+  new_asm_name[old_name_len] = '\0';
+}
+
 /* This function changes the assembler name for functions that are
    versions.  If DECL is a function version and has a "target"
    attribute, it appends the attribute string to its assembler name.  */
@@ -3896,8 +3927,7 @@  static tree
 ix86_mangle_function_version_assembler_name (tree decl, tree id)
 {
   tree version_attr;
-  const char *orig_name, *version_string;
-  char *attr_str, *assembler_name;
+  char *attr_str;
 
   if (DECL_DECLARED_INLINE_P (decl)
       && lookup_attribute ("gnu_inline",
@@ -3915,25 +3945,24 @@  ix86_mangle_function_version_assembler_name (tree decl, tree id)
   /* target attribute string cannot be NULL.  */
   gcc_assert (version_attr != NULL_TREE);
 
-  orig_name = IDENTIFIER_POINTER (id);
-  version_string
-    = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (version_attr)));
-
-  if (strcmp (version_string, "default") == 0)
+  if (!cgraph_node::get_create (decl)->is_target_clone
+      && is_function_default_version (decl))
     return id;
 
   attr_str = sorted_attr_string (TREE_VALUE (version_attr));
-  assembler_name = XNEWVEC (char, strlen (orig_name) + strlen (attr_str) + 2);
 
-  sprintf (assembler_name, "%s.%s", orig_name, attr_str);
+  char *suffix = XNEWVEC (char, strlen (attr_str) + 1);
+  create_new_asm_name (attr_str, suffix);
 
   /* Allow assembler name to be modified if already set.  */
   if (DECL_ASSEMBLER_NAME_SET_P (decl))
     SET_DECL_RTL (decl, NULL);
 
-  tree ret = get_identifier (assembler_name);
+  tree ret = clone_identifier (id, suffix);
+
   XDELETEVEC (attr_str);
-  XDELETEVEC (assembler_name);
+  XDELETEVEC (suffix);
+
   return ret;
 }
 
@@ -3941,9 +3970,17 @@  tree
 ix86_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))
-    id = ix86_mangle_function_version_assembler_name (decl, id);
+  if (TREE_CODE (decl) == FUNCTION_DECL)
+    {
+      if (DECL_FUNCTION_VERSIONED (decl))
+	id = ix86_mangle_function_version_assembler_name (decl, id);
+      /* Mangle the dispatched symbol but only in the case of target clones.  */
+      else if (cgraph_node::get_create (decl)->dispatcher_function
+	       && !cgraph_node::get_create (decl)->is_target_clone)
+	id = clone_identifier (id, "ifunc");
+      else if (cgraph_node::get_create (decl)->dispatcher_resolver_function)
+	id = clone_identifier (id, "resolver");
+    }
 #ifdef SUBTARGET_MANGLE_DECL_ASSEMBLER_NAME
   id = SUBTARGET_MANGLE_DECL_ASSEMBLER_NAME (decl, id);
 #endif
@@ -3982,6 +4019,7 @@  ix86_get_function_versions_dispatcher (void *decl)
   default_version_info = node_v;
   while (default_version_info->prev != NULL)
     default_version_info = default_version_info->prev;
+  default_node = default_version_info->this_node;
 
   /* If there is no default node, just return NULL.  */
   if (!is_function_default_version (default_node->decl))
@@ -4038,17 +4076,25 @@  make_resolver_func (const tree default_decl,
 {
   tree decl, type, t;
 
-  /* Create resolver function name based on default_decl.  */
-  tree decl_name = clone_function_name (default_decl, "resolver");
-  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
-
   /* The resolver function should return a (void *). */
   type = build_function_type_list (ptr_type_node, 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;
+
+  if (node->is_target_clone)
+    resolver_node->is_target_clone = true;
+
+  tree id = ix86_mangle_decl_assembler_name
+    (decl, node->function_version ()->assembler_name);
+  SET_DECL_ASSEMBLER_NAME (decl, id);
 
-  DECL_NAME (decl) = decl_name;
+  DECL_NAME (decl) = DECL_NAME (default_decl);
   TREE_USED (decl) = 1;
   DECL_ARTIFICIAL (decl) = 1;
   DECL_IGNORED_P (decl) = 1;
@@ -4095,7 +4141,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.  */
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 69e47ec7e79..142f27b78f6 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -13204,32 +13204,39 @@  tree
 riscv_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)
     {
-      std::string name = IDENTIFIER_POINTER (id) + std::string (".");
-      tree target_attr = lookup_attribute ("target_version",
-					   DECL_ATTRIBUTES (decl));
-
-      if (target_attr == NULL_TREE)
+      if (cgraph_node::get_create (decl)->dispatcher_resolver_function)
+	return clone_identifier (id, "resolver");
+      /* Do not mangle the default if it is not implemented.
+	 If the default is external to the TU the ifunc resolver is not created
+	 and the dispatched symbol is not created, so the default version can
+	 be called unmangled.  */
+      else if (is_function_default_version (decl) && !DECL_INITIAL (decl))
+	return id;
+      else if (DECL_FUNCTION_VERSIONED (decl))
 	{
-	  name += "default";
-	  return get_identifier (name.c_str ());
-	}
+	  tree target_attr
+	    = lookup_attribute ("target_version", DECL_ATTRIBUTES (decl));
 
-      const char *version_string = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE
-							(target_attr)));
+	  if (target_attr == NULL_TREE)
+	    return clone_identifier (id, "default");
 
-      /* Replace non-alphanumeric characters with underscores as the suffix.  */
-      for (const char *c = version_string; *c; c++)
-	name += ISALNUM (*c) == 0 ? '_' : *c;
+	  const char *version_string
+	    = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (target_attr)));
 
-      if (DECL_ASSEMBLER_NAME_SET_P (decl))
-	SET_DECL_RTL (decl, NULL);
+	  /* Replace non-alphanumeric characters with underscores as the suffix.
+	   */
+	  std::string suffix = "";
+	  for (const char *c = version_string; *c; c++)
+	    suffix += ISALNUM (*c) == 0 ? '_' : *c;
 
-      id = get_identifier (name.c_str ());
-    }
+	  if (DECL_ASSEMBLER_NAME_SET_P (decl))
+	    SET_DECL_RTL (decl, NULL);
 
+	  id = clone_identifier (id, suffix.c_str ());
+	}
+    }
   return id;
 }
 
@@ -13510,22 +13517,6 @@  dispatch_function_versions (tree dispatch_decl,
   return 0;
 }
 
-/* Return an identifier for the base assembler name of a versioned function.
-   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
@@ -13539,10 +13530,8 @@  make_resolver_func (const tree default_decl,
 {
   tree decl, type, t;
 
-  /* 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);
+  cgraph_node *node = cgraph_node::get (default_decl);
+  gcc_assert (node && node->function_version ());
 
   /* The resolver function should have signature
      (void *) resolver (uint64_t, void *) */
@@ -13551,10 +13540,18 @@  make_resolver_func (const tree default_decl,
 				   ptr_type_node,
 				   NULL_TREE);
 
-  decl = build_fn_decl (resolver_name, type);
-  SET_DECL_ASSEMBLER_NAME (decl, decl_name);
+  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;
+
+  if (node->is_target_clone)
+    resolver_node->is_target_clone = true;
+
+  tree id = riscv_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;
@@ -13619,7 +13616,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.  */
@@ -13686,24 +13683,12 @@  riscv_generate_version_dispatcher_body (void *node_p)
 
   /* 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 = riscv_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;
 }
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index c59bd43c76d..6ef769a1084 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -88,6 +88,7 @@ 
 extern tree rs6000_builtin_mask_for_load (void);
 extern tree rs6000_builtin_md_vectorized_function (tree, tree, tree);
 extern tree rs6000_builtin_reciprocal (tree);
+static tree rs6000_mangle_decl_assembler_name (tree, tree);
 
   /* Set -mabi=ieeelongdouble on some old targets.  In the future, power server
      systems will also set long double to be IEEE 128-bit.  AIX and Darwin
@@ -25379,13 +25380,21 @@  make_resolver_func (const tree default_decl,
 {
   /* Make the resolver function static.  The resolver function returns
      void *.  */
-  tree decl_name = clone_function_name (default_decl, "resolver");
-  const char *resolver_name = IDENTIFIER_POINTER (decl_name);
   tree type = build_function_type_list (ptr_type_node, NULL_TREE);
-  tree decl = build_fn_decl (resolver_name, type);
-  SET_DECL_ASSEMBLER_NAME (decl, decl_name);
+  tree decl = build_fn_decl (IDENTIFIER_POINTER (DECL_NAME (default_decl)),
+			     type);
 
-  DECL_NAME (decl) = decl_name;
+  cgraph_node *node = cgraph_node::get (default_decl);
+  gcc_assert (node && node->function_version ());
+
+  cgraph_node *resolver_node = cgraph_node::get_create (decl);
+  resolver_node->dispatcher_resolver_function = true;
+
+  tree id = rs6000_mangle_decl_assembler_name
+    (decl, node->function_version ()->assembler_name);
+  symtab->change_decl_assembler_name (decl, id);
+
+  DECL_NAME (decl) = DECL_NAME (default_decl);
   TREE_USED (decl) = 1;
   DECL_ARTIFICIAL (decl) = 1;
   DECL_IGNORED_P (decl) = 0;
@@ -25431,7 +25440,8 @@  make_resolver_func (const tree default_decl,
 
   /* Mark dispatch_decl as "ifunc" with resolver as resolver_name.  */
   DECL_ATTRIBUTES (dispatch_decl)
-    = make_attribute ("ifunc", resolver_name, DECL_ATTRIBUTES (dispatch_decl));
+    = make_attribute ("ifunc", IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)),
+		      DECL_ATTRIBUTES (dispatch_decl));
 
   cgraph_node::create_same_body_alias (dispatch_decl, decl);
 
@@ -28471,6 +28481,81 @@  complex_divide_builtin_code (machine_mode mode)
   return (built_in_function) func;
 }
 
+static bool
+is_valid_asm_symbol (char c)
+{
+  if ('a' <= c && c <= 'z')
+    return true;
+  if ('A' <= c && c <= 'Z')
+    return true;
+  if ('0' <= c && c <= '9')
+    return true;
+  if (c == '_')
+    return true;
+  return false;
+}
+
+/*  Replace all not valid assembler symbols with '_'.  */
+static void
+create_new_asm_name (char *old_asm_name, char *new_asm_name)
+{
+  int i;
+  int old_name_len = strlen (old_asm_name);
+  /* Replace all not valid assembler symbols with '_'.  */
+  for (i = 0; i < old_name_len; i++)
+    if (!is_valid_asm_symbol (old_asm_name[i]))
+      new_asm_name[i] = '_';
+    else
+      new_asm_name[i] = old_asm_name[i];
+  new_asm_name[old_name_len] = '\0';
+}
+
+/* This function changes the assembler name for functions that are
+   versions.  If DECL is a function version and has a "target"
+   attribute, it appends the attribute string to its assembler name.  */
+
+static tree
+rs6000_mangle_function_version_assembler_name (tree decl, tree id)
+{
+  tree version_attr;
+  const char *version_string;
+  char *attr_str;
+
+  if (DECL_DECLARED_INLINE_P (decl)
+      && lookup_attribute ("gnu_inline", DECL_ATTRIBUTES (decl)))
+    error_at (DECL_SOURCE_LOCATION (decl),
+	      "function versions cannot be marked as %<gnu_inline%>,"
+	      " bodies have to be generated");
+
+  if (DECL_VIRTUAL_P (decl) || DECL_VINDEX (decl))
+    sorry ("virtual function multiversioning not supported");
+
+  version_attr = lookup_attribute ("target", DECL_ATTRIBUTES (decl));
+
+  /* target attribute string cannot be NULL.  */
+  gcc_assert (version_attr != NULL_TREE);
+
+  version_string = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (version_attr)));
+
+  if (strcmp (version_string, "default") == 0)
+    return clone_identifier (id, "default");
+
+  attr_str = sorted_attr_string (TREE_VALUE (version_attr));
+
+  char *suffix = XNEWVEC (char, strlen (attr_str) + 1);
+  create_new_asm_name (attr_str, suffix);
+
+  /* Allow assembler name to be modified if already set.  */
+  if (DECL_ASSEMBLER_NAME_SET_P (decl))
+    SET_DECL_RTL (decl, NULL);
+
+  tree ret = clone_identifier (id, suffix);
+
+  XDELETEVEC (attr_str);
+  XDELETEVEC (suffix);
+  return ret;
+}
+
 /* On 64-bit Linux and Freebsd systems, possibly switch the long double library
    function names from <foo>l to <foo>f128 if the default long double type is
    IEEE 128-bit.  Typically, with the C and C++ languages, the standard math.h
@@ -28656,6 +28741,13 @@  rs6000_mangle_decl_assembler_name (tree decl, tree id)
 	}
     }
 
+  if (TREE_CODE (decl) == FUNCTION_DECL)
+    {
+      if (cgraph_node::get_create (decl)->dispatcher_resolver_function)
+	id = clone_identifier (id, "resolver");
+      else if (DECL_FUNCTION_VERSIONED (decl))
+	id = rs6000_mangle_function_version_assembler_name (decl, id);
+    }
   return id;
 }
 
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index fdef98f8062..9e1a2e0a9bd 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -3375,6 +3375,19 @@  duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden)
 	}
     }
 
+  /* If merging versioned functions, it is possible olddecl needs to be
+     re-mangled.  */
+  if (TREE_CODE (olddecl) == FUNCTION_DECL && DECL_FUNCTION_VERSIONED (olddecl))
+    {
+      /* The function version and assembler name should have been initialized
+	 when DECL_FUNCTION_VERSIONED was set.  */
+      gcc_assert (cgraph_node::get (olddecl)->function_version ());
+      tree id = targetm.mangle_decl_assembler_name
+	(olddecl, cgraph_node::get (olddecl)->function_version ()
+					    ->assembler_name);
+      symtab->change_decl_assembler_name (olddecl, id);
+    }
+
   /* Remove the associated constraints for newdecl, if any, before
      reclaiming memory. */
   if (flag_concepts)
diff --git a/gcc/multiple_target.cc b/gcc/multiple_target.cc
index d8becf4d9a9..6aeceadbfd1 100644
--- a/gcc/multiple_target.cc
+++ b/gcc/multiple_target.cc
@@ -162,9 +162,6 @@  create_dispatcher_calls (struct cgraph_node *node)
 	}
     }
 
-  tree fname = clone_function_name (node->decl, "default");
-  symtab->change_decl_assembler_name (node->decl, fname);
-
   if (node->definition)
     {
       /* FIXME: copy of cgraph_node::make_local that should be cleaned up
@@ -241,39 +238,6 @@  separate_attrs (char *attr_str, char **attrs, int attrnum)
   return i;
 }
 
-/*  Return true if symbol is valid in assembler name.  */
-
-static bool
-is_valid_asm_symbol (char c)
-{
-  if ('a' <= c && c <= 'z')
-    return true;
-  if ('A' <= c && c <= 'Z')
-    return true;
-  if ('0' <= c && c <= '9')
-    return true;
-  if (c == '_')
-    return true;
-  return false;
-}
-
-/*  Replace all not valid assembler symbols with '_'.  */
-
-static void
-create_new_asm_name (char *old_asm_name, char *new_asm_name)
-{
-  int i;
-  int old_name_len = strlen (old_asm_name);
-
-  /* Replace all not valid assembler symbols with '_'.  */
-  for (i = 0; i < old_name_len; i++)
-    if (!is_valid_asm_symbol (old_asm_name[i]))
-      new_asm_name[i] = '_';
-    else
-      new_asm_name[i] = old_asm_name[i];
-  new_asm_name[old_name_len] = '\0';
-}
-
 /*  Creates target clone of NODE.  */
 
 static cgraph_node *
@@ -309,7 +273,6 @@  create_target_clone (cgraph_node *node, bool definition, char *name,
 static bool
 expand_target_clones (struct cgraph_node *node, bool definition)
 {
-  int i;
   /* Parsing target attributes separated by TARGET_CLONES_ATTR_SEPARATOR.  */
   tree attr_target = lookup_attribute ("target_clones",
 				       DECL_ATTRIBUTES (node->decl));
@@ -317,11 +280,11 @@  expand_target_clones (struct cgraph_node *node, bool definition)
   if (!attr_target)
     return false;
 
-  tree arglist = TREE_VALUE (attr_target);
-  int attr_len = get_target_clone_attr_len (arglist);
+  int num_def = 0;
+  auto_vec<string_slice> attr_list = get_clone_versions (node->decl, &num_def);
 
   /* No need to clone for 1 target attribute.  */
-  if (attr_len == -1)
+  if (attr_list.length () == 1)
     {
       warning_at (DECL_SOURCE_LOCATION (node->decl),
 		  0, "single %<target_clones%> attribute is ignored");
@@ -348,67 +311,70 @@  expand_target_clones (struct cgraph_node *node, bool definition)
       return false;
     }
 
-  char *attr_str = XNEWVEC (char, attr_len);
-  int attrnum = get_attr_str (arglist, attr_str);
-  char **attrs = XNEWVEC (char *, attrnum);
-
-  attrnum = separate_attrs (attr_str, attrs, attrnum);
-  switch (attrnum)
+  /* Disallow multiple defaults.  */
+  if (num_def > 1)
     {
-    case -1:
-      error_at (DECL_SOURCE_LOCATION (node->decl),
-		"%<default%> target was not set");
-      break;
-    case -2:
-      error_at (DECL_SOURCE_LOCATION (node->decl),
-		"an empty string cannot be in %<target_clones%> attribute");
-      break;
-    case -3:
       error_at (DECL_SOURCE_LOCATION (node->decl),
 		"multiple %<default%> targets were set");
-      break;
-    default:
-      break;
+      return false;
     }
-
-  if (attrnum < 0)
+  /* Disallow target clones with no defaults.  */
+  if (num_def == 0 && TARGET_HAS_FMV_TARGET_ATTRIBUTE)
     {
-      XDELETEVEC (attrs);
-      XDELETEVEC (attr_str);
+      error_at (DECL_SOURCE_LOCATION (node->decl),
+		"%<default%> target was not set");
       return false;
     }
 
-  const char *new_attr_name = (TARGET_HAS_FMV_TARGET_ATTRIBUTE
-			       ? "target" : "target_version");
+  /* Disallow any empty values in the clone attr.  */
+  for (string_slice attr : attr_list)
+    if (attr == string_slice ("") || !attr.is_valid ())
+      {
+	error_at (DECL_SOURCE_LOCATION (node->decl),
+		  "an empty string cannot be in %<target_clones%> attribute");
+	return false;
+      }
+
+  string_slice new_attr_name = string_slice
+    (TARGET_HAS_FMV_TARGET_ATTRIBUTE ? "target" : "target_version");
   cgraph_function_version_info *decl1_v = NULL;
   cgraph_function_version_info *decl2_v = NULL;
   cgraph_function_version_info *before = NULL;
   cgraph_function_version_info *after = NULL;
   decl1_v = node->function_version ();
-  if (decl1_v == NULL)
-    decl1_v = node->insert_new_function_version ();
+  /* Function version should have been initialized when the target_clone was
+     processed in the front-end.  */
+  gcc_assert (decl1_v);
   before = decl1_v;
   DECL_FUNCTION_VERSIONED (node->decl) = 1;
-
-  for (i = 0; i < attrnum; i++)
+  node->is_target_clone = true;
+
+  /* The existing decl is turned into one of the target versions.
+     If there is a default in the list then this decl is used for that version
+     as the calls are already to it so in the case where there is no
+     implementation and !TARGET_HAS_FMV_TARGET_ATTRIBUTE there is no redirection
+     necessary.
+     Otherwise, the first version listed in the attribute is used.  */
+  string_slice this_node_version = num_def ? string_slice ("default")
+					   : attr_list.pop ();
+
+  for (string_slice attr : attr_list)
     {
-      char *attr = attrs[i];
+      /* Skip default nodes.  */
+      if (attr == string_slice ("default"))
+	continue;
 
       /* Create new target clone.  */
       tree attributes = make_attribute (new_attr_name, attr,
 					DECL_ATTRIBUTES (node->decl));
 
-      char *suffix = XNEWVEC (char, strlen (attr) + 1);
-      create_new_asm_name (attr, suffix);
-      cgraph_node *new_node = create_target_clone (node, definition, suffix,
-						   attributes);
-      XDELETEVEC (suffix);
+      /* Remove the target_clones attribute, as this can confuse
+	 is_function_default_version.  */
+      remove_attribute ("target_clones", attributes);
+      cgraph_node *new_node
+	= create_target_clone (node, definition, NULL, attributes);
       if (new_node == NULL)
-	{
-	  XDELETEVEC (attrs);
-	  XDELETEVEC (attr_str);
-	  return false;
-	}
+	return false;
       new_node->local = false;
 
       decl2_v = new_node->function_version ();
@@ -427,16 +393,30 @@  expand_target_clones (struct cgraph_node *node, bool definition)
       before->next = after;
       after->prev = before;
       DECL_FUNCTION_VERSIONED (new_node->decl) = 1;
-    }
 
-  XDELETEVEC (attrs);
-  XDELETEVEC (attr_str);
+      /* Use the base nodes assembler name for all created nodes.  */
+      new_node->function_version ()->assembler_name
+	= node->function_version ()->assembler_name;
+      new_node->is_target_clone = true;
+
+      /* Mangle all new nodes.  */
+      tree id = targetm.mangle_decl_assembler_name (
+	new_node->decl, new_node->function_version ()->assembler_name);
+      symtab->change_decl_assembler_name (new_node->decl, id);
+    }
 
   /* Setting new attribute to initial function.  */
-  tree attributes = make_attribute (new_attr_name, "default",
+  tree attributes = make_attribute (new_attr_name, this_node_version,
 				    DECL_ATTRIBUTES (node->decl));
+  remove_attribute ("target_clones", attributes);
   DECL_ATTRIBUTES (node->decl) = attributes;
   node->local = false;
+
+  /* Remangle base node after new target version string set.  */
+  tree id = targetm.mangle_decl_assembler_name
+	      (node->decl, node->function_version ()->assembler_name);
+  symtab->change_decl_assembler_name (node->decl, id);
+
   return true;
 }
 
diff --git a/gcc/testsuite/g++.target/i386/mv-symbols1.C b/gcc/testsuite/g++.target/i386/mv-symbols1.C
index 1290299aea5..3163f03ddd8 100644
--- a/gcc/testsuite/g++.target/i386/mv-symbols1.C
+++ b/gcc/testsuite/g++.target/i386/mv-symbols1.C
@@ -55,14 +55,14 @@  int bar(int x)
 /* { dg-final { scan-assembler-times "\n_Z3foov\.arch_slm:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.sse4.2:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.resolver:\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\tcall\t_Z7_Z3foovv\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3foovv, @gnu_indirect_function\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3foovv,_Z3foov\.resolver\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\tcall\t_Z3foov.ifunc\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3foov.ifunc, @gnu_indirect_function\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3foov.ifunc,_Z3foov\.resolver\n" 1 } } */
 
 /* { dg-final { scan-assembler-times "\n_Z3fooi:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.arch_slm:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.sse4.2:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.resolver:\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\tcall\t_Z7_Z3fooii\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3fooii, @gnu_indirect_function\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3fooii,_Z3fooi\.resolver\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\tcall\t_Z3fooi.ifunc\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3fooi.ifunc, @gnu_indirect_function\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3fooi.ifunc,_Z3fooi\.resolver\n" 1 } } */
diff --git a/gcc/testsuite/g++.target/i386/mv-symbols3.C b/gcc/testsuite/g++.target/i386/mv-symbols3.C
index a5cf3445a43..67b27351143 100644
--- a/gcc/testsuite/g++.target/i386/mv-symbols3.C
+++ b/gcc/testsuite/g++.target/i386/mv-symbols3.C
@@ -32,13 +32,13 @@  int bar()
 /* { dg-final { scan-assembler-times "\n_Z3foov\.arch_slm:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.sse4.2:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.resolver:\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\tcall\t_Z7_Z3foovv\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3foovv, @gnu_indirect_function\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3foovv,_Z3foov\.resolver\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\tcall\t_Z3foov.ifunc\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3foov.ifunc, @gnu_indirect_function\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3foov.ifunc,_Z3foov\.resolver\n" 1 } } */
 
 /* { dg-final { scan-assembler-times "\n_Z3fooi:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.arch_slm:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.sse4.2:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.resolver:\n" 0 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3fooii, @gnu_indirect_function\n" 0 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3fooii,_Z3fooi\.resolver\n" 0 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3fooi.ifunc, @gnu_indirect_function\n" 0 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3fooi.ifunc,_Z3fooi\.resolver\n" 0 } } */
diff --git a/gcc/testsuite/g++.target/i386/mv-symbols4.C b/gcc/testsuite/g++.target/i386/mv-symbols4.C
index bb10f126f67..c82db70da35 100644
--- a/gcc/testsuite/g++.target/i386/mv-symbols4.C
+++ b/gcc/testsuite/g++.target/i386/mv-symbols4.C
@@ -38,13 +38,13 @@  int bar()
 /* { dg-final { scan-assembler-times "\n_Z3foov\.arch_slm:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.sse4.2:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.resolver:\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\tcall\t_Z7_Z3foovv\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3foovv, @gnu_indirect_function\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3foovv,_Z3foov\.resolver\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\tcall\t_Z3foov.ifunc\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3foov.ifunc, @gnu_indirect_function\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3foov.ifunc,_Z3foov\.resolver\n" 1 } } */
 
 /* { dg-final { scan-assembler-times "\n_Z3fooi:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.arch_slm:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.sse4.2:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.resolver:\n" 0 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3fooii, @gnu_indirect_function\n" 0 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3fooii,_Z3fooi\.resolver\n" 0 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3fooi.ifunc, @gnu_indirect_function\n" 0 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3fooi.ifunc,_Z3fooi\.resolver\n" 0 } } */
diff --git a/gcc/testsuite/g++.target/i386/mv-symbols5.C b/gcc/testsuite/g++.target/i386/mv-symbols5.C
index d36e4c304c2..7792f113f22 100644
--- a/gcc/testsuite/g++.target/i386/mv-symbols5.C
+++ b/gcc/testsuite/g++.target/i386/mv-symbols5.C
@@ -44,13 +44,13 @@  int bar()
 /* { dg-final { scan-assembler-times "\n_Z3foov\.arch_slm:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.sse4.2:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3foov\.resolver:\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\tcall\t_Z7_Z3foovv\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3foovv, @gnu_indirect_function\n" 1 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3foovv,_Z3foov\.resolver\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\tcall\t_Z3foov.ifunc\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3foov.ifunc, @gnu_indirect_function\n" 1 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3foov.ifunc,_Z3foov\.resolver\n" 1 } } */
 
 /* { dg-final { scan-assembler-times "\n_Z3fooi:\n" 0 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.arch_slm:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.sse4.2:\n" 1 } } */
 /* { dg-final { scan-assembler-times "\n_Z3fooi\.resolver:\n" 0 } } */
-/* { dg-final { scan-assembler-times "\n\t\.type\t_Z7_Z3fooii, @gnu_indirect_function\n" 0 } } */
-/* { dg-final { scan-assembler-times "\n\t\.set\t_Z7_Z3fooii,_Z3fooi\.resolver\n" 0 } } */
+/* { dg-final { scan-assembler-times "\n\t\.type\t_Z3fooi.ifunc, @gnu_indirect_function\n" 0 } } */
+/* { dg-final { scan-assembler-times "\n\t\.set\t_Z3fooi.ifunc,_Z3fooi\.resolver\n" 0 } } */