Re: [PATCH v4 16/20] Refactor FMV frontend conflict and merging logic and hooks.

[Alfie Richards]

On 20/05/2025 03:28, Jason Merrill wrote:
On 4/15/25 6:31 AM, Alfie Richards wrote:
This change refactors FMV handling in the frontend to allows greater
reasoning about versions in shared code.

Looking at the cp/ changes:

diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index c28d9e5b3ab..4f195ae06cd 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3.  If not see
  #include "gimplify.h"
  #include "intl.h"
  #include "asan.h"
+#include "attribs.h"

This seems unneeded since distinct_ is declared in tree.h.

Ah yes, thank you.


  /* Id for dumping the class hierarchy.  */
  int class_dump_id;
@@ -8999,8 +9000,7 @@ resolve_address_of_overloaded_function (tree 
target_type,
       decls_match will return false as they are different.  */
        for (match = TREE_CHAIN (matches); match; match = TREE_CHAIN 
(match))
      if (!decls_match (fn, TREE_PURPOSE (match))
-        && !targetm.target_option.function_versions
-           (fn, TREE_PURPOSE (match)))
+        && !distinct_version_decls (fn, TREE_PURPOSE (match)))
            break;
        if (match)
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 898054c2891..ccd9547c2c8 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7121,7 +7121,7 @@ extern void note_iteration_stmt_body_end    (bool);
  extern void determine_local_discriminator    (tree, tree = NULL_TREE);
  extern bool member_like_constrained_friend_p    (tree);
  extern bool fns_correspond            (tree, tree);
-extern int decls_match                (tree, tree, bool = true);
+extern int decls_match                (tree, tree, bool = true, 
string_slice* = NULL);

I'm a bit uncomfortable with adding yet another parameter to decls_match 
for versioning.  Do we really need to pass back the version for 
incorporating in the diagnostics?  Isn't it enough to print the two 
declarations?  If not, you could call a function to give more 
information about the version mismatch without needing to store it in 
duplicate_decls.

I had a feeling this was too much. The solutions you have given are a 
better balance, I'll get a new version to you soon.

  extern bool maybe_version_functions        (tree, tree);
  extern bool validate_constexpr_redeclaration    (tree, tree);
  extern bool merge_default_template_args        (tree, tree, bool);
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index ec4b1755741..4a374fa29e3 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -1120,7 +1120,10 @@ fns_correspond (tree newdecl, tree olddecl)
     `const int&'.  */
  int
-decls_match (tree newdecl, tree olddecl, bool record_versions /* = 
true */)
+decls_match (tree newdecl,
+         tree olddecl,
+         bool record_versions /* = true */,
+         string_slice *conflicting_version)
  {
    int types_match;
@@ -1213,7 +1216,7 @@ decls_match (tree newdecl, tree olddecl, bool 
record_versions /* = true */)
        if (types_match
        && !DECL_EXTERN_C_P (newdecl)
        && !DECL_EXTERN_C_P (olddecl)
-      && targetm.target_option.function_versions (newdecl, olddecl))
+      && distinct_version_decls (newdecl, olddecl, conflicting_version))
      {
        if (record_versions)
          maybe_version_functions (newdecl, olddecl);
@@ -1296,7 +1299,7 @@ maybe_mark_function_versioned (tree decl)
  bool
  maybe_version_functions (tree newdecl, tree olddecl)
  {
-  if (!targetm.target_option.function_versions (newdecl, olddecl))
+  if (!distinct_version_decls (newdecl, olddecl))
      return false;
    maybe_mark_function_versioned (olddecl);
@@ -1686,11 +1689,12 @@ duplicate_decls (tree newdecl, tree olddecl, 
bool hiding, bool was_hidden)
    tree new_template_info;
    location_t olddecl_loc = DECL_SOURCE_LOCATION (olddecl);
    location_t newdecl_loc = DECL_SOURCE_LOCATION (newdecl);
+  string_slice conflicting_version = string_slice::invalid ();
    if (newdecl == olddecl)
      return olddecl;
-  types_match = decls_match (newdecl, olddecl);
+  types_match = decls_match (newdecl, olddecl, true, 
&conflicting_version);
    /* If either the type of the new decl or the type of the old decl 
is an
       error_mark_node, then that implies that we have already issued an
@@ -2106,6 +2110,16 @@ duplicate_decls (tree newdecl, tree olddecl, 
bool hiding, bool was_hidden)
        /* Leave it to update_binding to merge or report error.  */
        return NULL_TREE;
      }
+  else if (!TARGET_HAS_FMV_TARGET_ATTRIBUTE
+       && !mergeable_version_decls (newdecl, olddecl))
+    {
+      /* newdecl defines an overlapping FMV version with olddecl but 
they
+     cannot be merged so are conflicting.  */
+      gcc_assert (conflicting_version.is_valid ());
+      error_at (newdecl_loc, "conflicting %qB versions", 
&conflicting_version);
+      inform (olddecl_loc, "previous definition");

That is, here you could call an explain_nonmergeable_versions function.

+      return error_mark_node;
+    }
    else
      {
        const char *errmsg = redeclaration_error_message (newdecl, 
olddecl);
@@ -2114,10 +2128,23 @@ duplicate_decls (tree newdecl, tree olddecl, 
bool hiding, bool was_hidden)
        auto_diagnostic_group d;
        error_at (newdecl_loc, errmsg, newdecl);
        if (DECL_NAME (olddecl) != NULL_TREE)
-        inform (olddecl_loc,
-            (DECL_INITIAL (olddecl) && namespace_bindings_p ())
-            ? G_("%q#D previously defined here")
-            : G_("%q#D previously declared here"), olddecl);
+        {
+          /* If conflicting_version is set then this collision is 
between
+         two FMV annotated functions.  */
+          if (conflicting_version.is_valid ())
+        inform (olddecl_loc,
+            (DECL_INITIAL (olddecl) && namespace_bindings_p ())
+            ? G_("%qB version of %q#D previously defined here")
+            : G_("%qB version of %q#D previously declared here"),

...and this seems redundant.  Isn't the version printed by %D?  If not, 
that would be good to do.

%D currently doesn't show the target_version/target_clone if its on a 
different line, which is the issue I'm trying to solve with this.

i.e. currently for

```c++
__attribute((target_version("sve"))
int foo () {return 1;}

__attribute((target_version("sve"))
int foo () {return 2;}
```

(or similar) you end up with something like:

```
test.cpp:5:5: error: redefinition of ‘int foo()’
    5 | int foo () {return 2;}
      |     ^~~
test.cpp:2:5: note: ‘int foo()’ previously defined here
    2 | int foo () {return 1;}
```

I agree changing %D is a better solution, thank you.


+            &conflicting_version,
+            olddecl);
+          else
+        inform (olddecl_loc,
+            (DECL_INITIAL (olddecl) && namespace_bindings_p ())
+            ? G_("%q#D previously defined here")
+            : G_("%q#D previously declared here"),
+            olddecl);
+        }
        if (cxx_dialect >= cxx26
            && DECL_NAME (newdecl)
            && id_equal (DECL_NAME (newdecl), "_")