Re: [PATCH v1 09/16] Add assembler_name to cgraph_function_version_info.

[Alfie Richards]

On 04/02/2025 08:41, Richard Sandiford wrote:

Alfie Richards <alfie.richa...@arm.com> writes:

This adds the assembler_name member to cgraph_function_version_info
to store the base assembler name for the function to be mangled. This is
used in later patches for refactoring FMV mangling.

gcc/c/ChangeLog:

        * c-decl.cc (start_decl): Record assembler_name.
        (start_function): Record assembler_name.

gcc/ChangeLog:

        * cgraph.cc (cgraph_node::record_function_versions): Record
        assembler_name.
        * cgraph.h (struct cgraph_function_version_info): Add assembler_name.

gcc/cp/ChangeLog:

        * decl.cc (maybe_mark_function_versioned): Record assember_name.
        (start_decl): Record assembler_name.
        (start_preparsed_function): Record assembler_name.

I'm not really qualified to review this part, but FWIW, my main question
would be: is it safe to cache the DECL_ASSEMBLER_NAME so early?  E.g. wrt
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117987 , it looks like the
C frontend applies the asm in finish_decl rather than start_decl.
I don't know whether the name could change even later, or whether
it is supposed to be final after finish_decl.

You're absolutely right, this is definitely the wrong time to do this. I
have found a better way and will send an updated version shortly.

This was also the reasons for the CI failures I didn't catch in my
testing, as it was subtly unsound.Sorry about this.

Also:

---
  gcc/c/c-decl.cc | 20 ++++++++++++++++++++
  gcc/cgraph.cc   | 10 ++++++++--
  gcc/cgraph.h    |  3 +++
  gcc/cp/decl.cc  | 34 ++++++++++++++++++++++++++++++++++
  4 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 0dcbae9b26f..daa19f360e6 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -5762,6 +5762,16 @@ start_decl (struct c_declarator *declarator, struct 
c_declspecs *declspecs,
        && VAR_OR_FUNCTION_DECL_P (decl))
        objc_check_global_decl (decl);

+ /* Store the base assembler name for mangling later. */

+  if (TREE_CODE (decl) == FUNCTION_DECL
+      && lookup_attribute ("target_clones", DECL_ATTRIBUTES (decl)))
+    {
+      cgraph_node *node = cgraph_node::get_create (decl);
+      if (!node->function_version ())
+       node->insert_new_function_version ();
+      node->function_version ()->assembler_name = DECL_ASSEMBLER_NAME (decl);
+    }
+
    /* Add this decl to the current scope.
       TEM may equal DECL or it may be a previous decl of the same name.  */
    if (do_push)
@@ -10863,6 +10873,16 @@ start_function (struct c_declspecs *declspecs, struct 
c_declarator *declarator,

current_function_decl = pushdecl (decl1); + /* Store the base assembler name for mangling later. */

+  if (TREE_CODE (decl1) == FUNCTION_DECL
+      && lookup_attribute ("target_clones", DECL_ATTRIBUTES (decl1)))
+    {
+      cgraph_node *node = cgraph_node::get_create (decl1);
+      if (!node->function_version ())
+       node->insert_new_function_version ();
+      node->function_version ()->assembler_name = DECL_ASSEMBLER_NAME (decl1);
+    }
+
    if (tree access = build_attr_access_from_parms (parms, false))
      decl_attributes (&current_function_decl, access, ATTR_FLAG_INTERNAL,
                     old_decl);
diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index 1ea38d16e56..c2038be4671 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -252,10 +252,16 @@ cgraph_node::record_function_versions (tree decl1, tree 
decl2)
    decl2_v = decl2_node->function_version ();

if (decl1_v == NULL)

-    decl1_v = decl1_node->insert_new_function_version ();
+    {
+      decl1_v = decl1_node->insert_new_function_version ();
+      decl1_v->assembler_name = DECL_ASSEMBLER_NAME (decl1);
+    }

if (decl2_v == NULL)

-    decl2_v = decl2_node->insert_new_function_version ();
+    {
+      decl2_v = decl2_node->insert_new_function_version ();
+      decl2_v->assembler_name = DECL_ASSEMBLER_NAME (decl2);
+    }

gcc_assert (decl1_v);

    gcc_assert (decl2_v);
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 065fcc742e8..d9177364b7a 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -856,6 +856,9 @@ struct GTY((for_user)) cgraph_function_version_info {
       dispatcher. The dispatcher decl is an alias to the resolver
       function decl.  */
    tree dispatcher_resolver;
+
+  /* The assmbly name of the function set before version mangling.  */
+  tree assembler_name;
  };

#define DEFCIFCODE(code, type, string) CIF_ ## code,

diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 3b3b4481964..fdef98f8062 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -1273,6 +1273,12 @@ maybe_mark_function_versioned (tree decl)
  {
    if (!DECL_FUNCTION_VERSIONED (decl))
      {
+      cgraph_node *node = cgraph_node::get_create (decl);
+      if (!node->function_version ())
+       node->insert_new_function_version ();
+      if (!node->function_version ()->assembler_name)
+       node->function_version ()->assembler_name = DECL_ASSEMBLER_NAME (decl);
+
        DECL_FUNCTION_VERSIONED (decl) = 1;
        /* If DECL_ASSEMBLER_NAME has already been set, re-mangle
         to include the version marker.  */
@@ -6155,6 +6161,20 @@ start_decl (const cp_declarator *declarator,

was_public = TREE_PUBLIC (decl); + /* Set the assembler string for any versioned function. */

+  if (TREE_CODE (decl) == FUNCTION_DECL
+      && (lookup_attribute (TARGET_HAS_FMV_TARGET_ATTRIBUTE ? "target"
+                                                           : "target_version",
+                           DECL_ATTRIBUTES (decl))
+         || lookup_attribute ("target_clones", DECL_ATTRIBUTES (decl))))
+    {
+      cgraph_node *node = cgraph_node::get_create (decl);
+      if (!node->function_version ())
+       node->insert_new_function_version ();
+      if (!node->function_version ()->assembler_name)
+       node->function_version ()->assembler_name = DECL_ASSEMBLER_NAME (decl);
+    }

function_version () is a hash lookup, so I think we should try to minimise
the number of calls.  E.g.:

     auto *decl_version = node->function_version ();
     if (!decl_version)
       decl_version = node->insert_new_function_version ();

It'd also be good avoid the cut-&-paste, in the sense that the C frontend
has two copies of the same code, and similarly the C++ frontend.

Thanks,
Richard

+
    if ((DECL_EXTERNAL (decl) || TREE_CODE (decl) == FUNCTION_DECL)
        && current_function_decl)
      {
@@ -18743,6 +18763,20 @@ start_preparsed_function (tree decl1, tree attrs, int 
flags)
    if (!DECL_OMP_DECLARE_REDUCTION_P (decl1))
      start_lambda_scope (decl1);

+ /* Set the assembler string for any versioned function. */

+  if (TREE_CODE (decl1) == FUNCTION_DECL
+      && (lookup_attribute (TARGET_HAS_FMV_TARGET_ATTRIBUTE ? "target"
+                                                           : "target_version",
+                           DECL_ATTRIBUTES (decl1))
+         || lookup_attribute ("target_clones", DECL_ATTRIBUTES (decl1))))
+    {
+      cgraph_node *node = cgraph_node::get_create (decl1);
+      if (!node->function_version ())
+       node->insert_new_function_version ();
+      if (!node->function_version ()->assembler_name)
+       node->function_version ()->assembler_name = DECL_ASSEMBLER_NAME (decl1);
+    }
+
    return true;
  }