On 18/02/2025 12:11, Richard Sandiford wrote:
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.
I see your point here, which I think relies on the assumption that
functions get
added to the structure one by one rather than in a fractal pattern.
This assumption is already used here subtly so that makes sense.
I added this logic to at least try make this work in a slightly more
general case as
to tell if a structure contains the default we should check the first
element
of that structure, but it is unnecessary given that knowledge.
I would prefer to change this to make that more explicit and change this
to be
"add_decl_to_version_into" taking a cgraph_function_version_info for the
existing structure and a decl for the version to add to make this
explicit. Would that change work for you?
[...]
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.
Apologies for this, I meant to change this and must have missed it.
I agree with your reasoning.
OK for GCC 16 with those changes, if you agree with them.
Thanks,
Richard
