Alfie Richards <alfie.richa...@arm.com> writes:
> This changes function version structures to maintain the default version
> as the first declaration in the linked data structures by giving priority
> to the set containing the default when constructing the structure.
>
> This allows for removing logic for moving the default to the first
> position which was duplicated across target specific code and enables
> easier reasoning about function sets when checking for a default.
>
> gcc/ChangeLog:
>
> * cgraph.cc (cgraph_node::record_function_versions): Update to
> implicitly keep default first.
> * config/aarch64/aarch64.cc (aarch64_get_function_versions_dispatcher):
> Remove reordering.
> * config/i386/i386-features.cc (ix86_get_function_versions_dispatcher):
> Remove reordering.
> * config/riscv/riscv.cc (riscv_get_function_versions_dispatcher):
> Remove reordering.
> * config/rs6000/rs6000.cc (rs6000_get_function_versions_dispatcher):
> Remove reordering.
> ---
> gcc/cgraph.cc | 27 ++++++++++++++++-----
> gcc/config/aarch64/aarch64.cc | 37 +++++++---------------------
> gcc/config/i386/i386-features.cc | 33 ++++---------------------
> gcc/config/riscv/riscv.cc | 41 +++++++-------------------------
> gcc/config/rs6000/rs6000.cc | 35 +++++----------------------
> 5 files changed, 49 insertions(+), 124 deletions(-)
>
> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
> index d0b19ad850e..bf6b43d00db 100644
> --- a/gcc/cgraph.cc
> +++ b/gcc/cgraph.cc
> @@ -247,7 +247,9 @@ cgraph_node::record_function_versions (tree decl1, tree
> decl2)
> decl1_v = decl1_node->function_version ();
> decl2_v = decl2_node->function_version ();
>
> - if (decl1_v != NULL && decl2_v != NULL)
> + /* If the nodes are already linked, skip. */
> + if ((decl1_v != NULL && (decl1_v->next || decl1_v->prev))
> + && (decl2_v != NULL && (decl2_v->next || decl2_v->prev)))
> return;
>
> if (decl1_v == NULL)
> @@ -256,18 +258,31 @@ cgraph_node::record_function_versions (tree decl1, tree
> decl2)
> if (decl2_v == NULL)
> decl2_v = decl2_node->insert_new_function_version ();
>
> - /* Chain decl2_v and decl1_v. All semantically identical versions
> - will be chained together. */
> + gcc_assert (decl1_v);
> + gcc_assert (decl2_v);
>
> before = decl1_v;
> after = decl2_v;
>
> + /* Go to first after node. */
> + while (after->prev != NULL)
> + after = after->prev;
> +
> + while (before->prev != NULL)
> + before = before->prev;
> +
> + /* Potentially swap the nodes to maintain the default always being in the
> + first position. */
> + if (before->next
> + ? !is_function_default_version (before->this_node->decl)
> + : is_function_default_version (after->this_node->decl))
> + std::swap (before, after);
> +
> + /* Go to last node of before. */
> while (before->next != NULL)
> before = before->next;
>
> - while (after->prev != NULL)
> - after= after->prev;
> -
> + /* Chain decl2_v and decl1_v. */
I think this can be simplified to:
before = decl1_v;
after = decl2_v;
/* Potentially swap the nodes to maintain the default always being in the
first position. */
if (before->prev || before->next
? is_function_default_version (after->this_node->decl)
: !is_function_default_version (before->this_node->decl))
std::swap (before, after);
while (before->next != NULL)
before = before->next;
while (after->prev != NULL)
after = after->prev;
That is, if one decl is linked (and so the other is not), we only want
to put the other decl first if it is the default.
> [...]
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 9bf7713139f..e5aa99a4965 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -13726,7 +13726,6 @@ riscv_get_function_versions_dispatcher (void *decl)
> struct cgraph_node *node = NULL;
> struct cgraph_node *default_node = NULL;
> struct cgraph_function_version_info *node_v = NULL;
> - struct cgraph_function_version_info *first_v = NULL;
>
> tree dispatch_decl = NULL;
>
> @@ -13743,41 +13742,19 @@ riscv_get_function_versions_dispatcher (void *decl)
> if (node_v->dispatcher_resolver != NULL)
> return node_v->dispatcher_resolver;
>
> - /* Find the default version and make it the first node. */
> - first_v = node_v;
> - /* Go to the beginning of the chain. */
> - while (first_v->prev != NULL)
> - first_v = first_v->prev;
> - default_version_info = first_v;
> -
> - while (default_version_info != NULL)
> - {
> - struct riscv_feature_bits res;
> - int priority; /* Unused. */
> - parse_features_for_version (default_version_info->this_node->decl,
> - res, priority);
> - if (res.length == 0)
> - break;
> - default_version_info = default_version_info->next;
> - }
> + /* The default node is always the beginning of the chain. */
> + default_version_info = node_v;
> + while (default_version_info->prev)
> + default_version_info = default_version_info->prev;
> + default_node = default_version_info->this_node;
>
> /* If there is no default node, just return NULL. */
> - if (default_version_info == NULL)
> + struct riscv_feature_bits res;
> + int priority; /* Unused. */
> + parse_features_for_version (default_node->decl, res, priority);
> + if (res.length != 0)
> return NULL;
My comment from the previous review still stands:
I expect you were trying to avoid editorialising, but: how about
converting this to is_function_default_version at the same time?
That seems more logically consistent with the target-independent code
and wasn't an option open to the RISC-V code until patch 5.
To put it another way, not being able to use is_function_default_version
here would suggest that the "default-first" assumption doesn't in fact
hold for RISC-V.
I.e. I think this can just be:
if (!is_function_default_version (default_node->decl))
return NULL;
as for the others.
OK for GCC 16 with those changes, if you agree with them.
Thanks,
Richard
