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.
> +
> + 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;
> }
