On 04/02/2025 15:11, Richard Sandiford wrote:
Richard Sandiford <richard.sandif...@arm.com> writes:
Alfie Richards <alfie.richa...@arm.com> writes:
---
gcc/cgraph.cc | 39 +++++++++++++++++++++++-------
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, 58 insertions(+), 127 deletions(-)
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index d0b19ad850e..1ea38d16e56 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -236,37 +236,58 @@ cgraph_node::delete_function_version_by_decl (tree decl)
void
cgraph_node::record_function_versions (tree decl1, tree decl2)
{
- cgraph_node *decl1_node = cgraph_node::get_create (decl1);
- cgraph_node *decl2_node = cgraph_node::get_create (decl2);
+ cgraph_node *decl1_node;
+ cgraph_node *decl2_node;
cgraph_function_version_info *decl1_v = NULL;
cgraph_function_version_info *decl2_v = NULL;
cgraph_function_version_info *before;
cgraph_function_version_info *after;
+ cgraph_function_version_info *temp_node;
+
+ decl1_node = cgraph_node::get_create (decl1);
+ decl2_node = cgraph_node::get_create (decl2);
gcc_assert (decl1_node != NULL && decl2_node != NULL);
decl1_v = decl1_node->function_version ();
decl2_v = decl2_node->function_version ();
- if (decl1_v != NULL && decl2_v != NULL)
- return;
-
Could you go into more detail about why this return needs to be removed?
It seems like the assumption was that, if the two decls were already
versioned, they were already versions of the same thing. For example,
we wouldn't create a set of 4 versions and a set of 2 versions and
only then merge them into a single set of 6 versions. Is that not
the case with the new scheme?
This was removed because function versions are populated when the assembler
name is set (in the commit #9 of this series). So I removed this check and
added the later check which exits if the first node of each series is equal.
I've come up with a better way of handling assembler names though that
doesn't require initializing the function version members so early and
can add this simpler check back in.
You're right that we never join two concurrently built version sets,
though this function is constructed in a way to make that mostly possible.
If we could keep the return, then we could add:
if (is_function_default_version (decl2)
|| (!decl1_v && !is_function_default_version (decl1)))
On second thoughts, a slightly cleaner alternative might be:
if (decl2_v
? !is_function_default_version (decl1)
: is_function_default_version (decl2))
Yeah this is nice, I will switch to this for the next version.
{
std::swap (decl1, decl2);
std::swap (decl1_v, decl2_v);
}
after it and then proceed as before, on the basis that (a) decl1_v and
decl2_v are individually canonical and (b) after the swap, any default
must be decl1 or earlier in decl1_v. That would avoid a bit of extra
pointer chasing.
Thank you for the feedback!
