Re: [PATCH v7 02/13] fmv: Refactor FMV name mangling.

[Alfie Richards]

On 01/08/2025 11:46, Richard Sandiford wrote:
Sorry, I think I missed the multiple_targets.cc changes in my
previous review.
Alfie Richards <alfie.richa...@arm.com> writes:
diff --git a/gcc/multiple_target.cc b/gcc/multiple_target.cc
index d25277c0a93..44340cbc6a4 100644
--- a/gcc/multiple_target.cc
+++ b/gcc/multiple_target.cc
@@ -313,7 +216,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));
@@ -321,11 +223,12 @@ 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_defaults = 0;
+  auto_vec<string_slice> attr_list = get_clone_versions (node->decl,
+                                                        &num_defaults);
  
    /* 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");
@@ -352,95 +255,114 @@ 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_defaults > 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_defaults == 0)
      {
-      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");
-  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 ();
-  before = decl1_v;
-  DECL_FUNCTION_VERSIONED (node->decl) = 1;
-
-  for (i = 0; i < attrnum; i++)
+  /* Disallow any empty values in the clone attr.  */
+  for (string_slice attr : attr_list)
+    if (attr.empty () || !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 = TARGET_HAS_FMV_TARGET_ATTRIBUTE
+                              ? "target"
+                              : "target_version";
+
+  cgraph_function_version_info *node_v = node->function_version ();
+
+  if (!node_v)
+    node_v = node->insert_new_function_version ();
+
+  /* If this target_clones contains a default, then convert this node to the
+     default.  If this node does not contain default (this is only possible
+     in target_version semantics) then remove the node.  This is safe at the
+     point as only target_clones declarations containing default version is

s/the point/this point/
s/default versions is/a default version are/

+     resolvable so this decl will have no calls/refrences.  */

Typo: references

+
+  tree attrs =  remove_attribute ("target_clones",
+                                 DECL_ATTRIBUTES (node->decl));
+  tree assembler_name = node_v->assembler_name;
+
+  /* Change the current node into the default node.  */
+  if (num_defaults == 1)
      {
-      char *attr = attrs[i];
+      /* Setting new attribute to initial function.  */
+      tree attributes = make_attribute (new_attr_name, "default", attrs);
+      DECL_ATTRIBUTES (node->decl) = attributes;
+      DECL_FUNCTION_VERSIONED (node->decl) = true;
+
+      node->is_target_clone = true;
+      node->local = false;
+
+      /* Remangle base node after new target version string set.  */
+      tree id = targetm.mangle_decl_assembler_name (node->decl, 
assembler_name);
+      symtab->change_decl_assembler_name (node->decl, id);
+    }
+  else
+    {
+      /* Target clones without a default are only allowed for target_version
+        semantics where we can have target_clones/target_version mixing.  */
+      gcc_assert (!TARGET_HAS_FMV_TARGET_ATTRIBUTE);
+
+      /* If there isn't a default version, can safely remove this version.
+        The node itself gets removed after the other versions are created.  */
+      cgraph_function_version_info *temp = node_v;
+      node_v = node_v->next ? node_v->next : node_v->prev;
+      cgraph_node::delete_function_version (temp);
+    }

In terms of staging, would it be worth putting the else block in patch 8
instead, along with the corresponding !node_v and num_defaults == 0
handling?  As it stands, this code can't be reached.

I don't mind keeping it as-is if you prefer though.  I just found it a
little hard to follow in this context.
You're right that this and patch 8 are really intertwined. I developed 
them simultaneously and didn't want to split up this logic as it would 
have involved writing new logic to delete later.

I can do that if you would like. But I'd rather not personally :)>
+
+  for (string_slice attr : attr_list)
+    {
+      /* Skip default nodes.  */
+      if (attr == "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);
+      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 ();
-      if (decl2_v != NULL)
-        continue;
-      decl2_v = new_node->insert_new_function_version ();
-
-      /* Chain decl2_v and decl1_v.  All semantically identical versions
-        will be chained together.  */
-      after = decl2_v;
-      while (before->next != NULL)
-       before = before->next;
-      while (after->prev != NULL)
-       after = after->prev;
-
-      before->next = after;
-      after->prev = before;
-      DECL_FUNCTION_VERSIONED (new_node->decl) = 1;
+      DECL_FUNCTION_VERSIONED (new_node->decl) = true;
+      if (!node_v)
+       node_v = new_node->insert_new_function_version ();
+      else
+       cgraph_node::add_function_version (node_v, new_node->decl);
+
+      /* Use the base nodes assembler name for all created nodes.  */

node's

Thanks,
Richard

+      new_node->function_version ()->assembler_name = 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);
      }
  
-  XDELETEVEC (attrs);
-  XDELETEVEC (attr_str);
+  /* If there are no default versions in the target_clones, this node is not
+     reused, so can delete this node.  */
+  if (num_defaults == 0)
+    node->remove ();
  
-  /* Setting new attribute to initial function.  */
-  tree attributes = make_attribute (new_attr_name, "default",
-                                   DECL_ATTRIBUTES (node->decl));
-  DECL_ATTRIBUTES (node->decl) = attributes;
-  node->local = false;
    return true;
  }