Re: [PATCH v2] Add warning for non-spec compliant FMV in Aarch64

[Alfie Richards]

Thank you both for feedback.

On 10/01/2025 10:47, Kyrylo Tkachov wrote:
On 10 Jan 2025, at 11:22, Richard Sandiford <richard.sandif...@arm.com> wrote:
<alfie.richa...@arm.com> writes:
This patch adds a warning when FMV is used for Aarch64.

The reasoning for this is the ACLE [1] spec for FMV has diverged
significantly from the current implementation and we want to prevent
potential future compatability issues.

There is a patch for an ACLE compliant version of target_version and
target_clone in progress but it won't make gcc-15.

This has been bootstrap and regression tested for Aarch64.
Is this okay for master and packport to gcc-14?

[1] 
https://github.com/ARM-software/acle/blob/main/main/acle.md#function-multi-versioning

gcc/ChangeLog:

* config/aarch64/aarch64.cc
(aarch64_mangle_decl_assembler_name): Add experimental warning.
* config/aarch64/aarch64.opt: Add command line option to disable
warning.

gcc/testsuite/ChangeLog:

* g++.target/aarch64/mv-1.C: Add CLI flag
* g++.target/aarch64/mv-symbols1.C: Add CLI flag
* g++.target/aarch64/mv-symbols2.C: Add CLI flag
* g++.target/aarch64/mv-symbols3.C: Add CLI flag
* g++.target/aarch64/mv-symbols4.C: Add CLI flag
* g++.target/aarch64/mv-symbols5.C: Add CLI flag
* g++.target/aarch64/mvc-symbols1.C: Add CLI flag
* g++.target/aarch64/mvc-symbols2.C: Add CLI flag
* g++.target/aarch64/mvc-symbols3.C: Add CLI flag
* g++.target/aarch64/mvc-symbols4.C: Add CLI flag
* g++.target/aarch64/mv-warning1.C: New test.
---
gcc/config/aarch64/aarch64.cc                   |  4 ++++
gcc/config/aarch64/aarch64.opt                  |  4 ++++
gcc/doc/invoke.texi                             | 11 ++++++++++-
gcc/testsuite/g++.target/aarch64/mv-1.C         |  1 +
gcc/testsuite/g++.target/aarch64/mv-symbols1.C  |  1 +
gcc/testsuite/g++.target/aarch64/mv-symbols2.C  |  1 +
gcc/testsuite/g++.target/aarch64/mv-symbols3.C  |  1 +
gcc/testsuite/g++.target/aarch64/mv-symbols4.C  |  1 +
gcc/testsuite/g++.target/aarch64/mv-symbols5.C  |  1 +
gcc/testsuite/g++.target/aarch64/mv-warning1.C  |  9 +++++++++
gcc/testsuite/g++.target/aarch64/mvc-symbols1.C |  1 +
gcc/testsuite/g++.target/aarch64/mvc-symbols2.C |  1 +
gcc/testsuite/g++.target/aarch64/mvc-symbols3.C |  1 +
gcc/testsuite/g++.target/aarch64/mvc-symbols4.C |  1 +
14 files changed, 37 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.target/aarch64/mv-warning1.C

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 91de13159cb..7d64e99b76b 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -20347,6 +20347,10 @@ aarch64_mangle_decl_assembler_name (tree decl, tree id)
   if (TREE_CODE (decl) == FUNCTION_DECL
       && DECL_FUNCTION_VERSIONED (decl))
     {
+      warning_at (DECL_SOURCE_LOCATION(decl),  OPT_Wexperimental_fmv_target,
+  "Function Multi Versioning support is experimental, and the "
+  "behavior is likely to change");
+
       aarch64_fmv_feature_mask feature_mask = get_feature_mask_for_version 
(decl);
Did you consider doing this in aarch64_option_valid_version_attribute_p
instead?  That hook is called directly by the frontend and is something
that already produces diagnostics for invalid versions.
I have tried this, as it does feel like a more natural fit. However, 
when debugging on an
example programmy break point for 
aarch64_option_valid_version_attribute_p is never reached and the 
warning not issued.
OK with that change from POV if it works, and if you agree.
Please give others until Monday to comment though.
Sounds good.
I wonder do we want to make it a once-only warning similar to 
aarch64_report_sve_required ?
If the user has multiple multiversioned functions in their code maybe warning 
only once is okay.
On the other hand, maybe pointing out all of them at once is more helpful.
I don’t feel strongly about it, so happy with either.
I don't have strong feelings about this either.
I’m happy to change it but lean towards leaving it as is for now unless 
anyone else has
a preference.
Thanks,
Kyrill
Thanks,
Richard

       std::string name = IDENTIFIER_POINTER (id);
diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt
index 36bc719b822..2a8dd8ea66c 100644
--- a/gcc/config/aarch64/aarch64.opt
+++ b/gcc/config/aarch64/aarch64.opt
@@ -431,3 +431,7 @@ handling.  One means we try to form pairs involving one or 
more existing
individual writeback accesses where possible.  A value of two means we
also try to opportunistically form writeback opportunities by folding in
trailing destructive updates of the base register used by a pair.
+
+Wexperimental-fmv-target
+Target Var(warn_experimental_fmv) Warning Init(1)
+Warn about usage of experimental Function Multi Versioning.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 51dc871e6bc..bdf9ee1bc0c 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -822,7 +822,8 @@ Objective-C and Objective-C++ Dialects}.
-moverride=@var{string}  -mverbose-cost-dump
-mstack-protector-guard=@var{guard} -mstack-protector-guard-reg=@var{sysreg}
-mstack-protector-guard-offset=@var{offset} -mtrack-speculation
--moutline-atomics -mearly-ldp-fusion -mlate-ldp-fusion}
+-moutline-atomics -mearly-ldp-fusion -mlate-ldp-fusion
+-Wexperimental-fmv-target}

@emph{Adapteva Epiphany Options}
@gccoptlist{-mhalf-reg-file  -mprefer-short-insn-regs
@@ -22087,6 +22088,14 @@ which specify use of that register as a fixed register,
and @samp{none}, which means that no register is used for this
purpose.  The default is @option{-m1reg-none}.

+@opindex Wexperimental-fmv-target
+@opindex Wno-experimental-fmv-target
+@item -Wexperimental-fmv-target
+Warn about use of experimental Function Multi Versioning.
+The Arm C Language Extension specification for Function Multi Versioning
+is beta and subject to change. Any usage of FMV is caveated that future
+behavior change and incompatibility is likely.
+
@end table

@node AMD GCN Options
diff --git a/gcc/testsuite/g++.target/aarch64/mv-1.C 
b/gcc/testsuite/g++.target/aarch64/mv-1.C
index b4b0e5e3fea..b10037f1b9b 100644
--- a/gcc/testsuite/g++.target/aarch64/mv-1.C
+++ b/gcc/testsuite/g++.target/aarch64/mv-1.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_version("default")))
int foo ()
diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols1.C 
b/gcc/testsuite/g++.target/aarch64/mv-symbols1.C
index 53e0abcd9b4..73cde42fa34 100644
--- a/gcc/testsuite/g++.target/aarch64/mv-symbols1.C
+++ b/gcc/testsuite/g++.target/aarch64/mv-symbols1.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

int foo ()
{
diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols2.C 
b/gcc/testsuite/g++.target/aarch64/mv-symbols2.C
index f0c7967a97a..6da88ddfb48 100644
--- a/gcc/testsuite/g++.target/aarch64/mv-symbols2.C
+++ b/gcc/testsuite/g++.target/aarch64/mv-symbols2.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_version("default")))
int foo ()
diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols3.C 
b/gcc/testsuite/g++.target/aarch64/mv-symbols3.C
index 3d30e27deb8..5dd7b49be2a 100644
--- a/gcc/testsuite/g++.target/aarch64/mv-symbols3.C
+++ b/gcc/testsuite/g++.target/aarch64/mv-symbols3.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_version("default")))
int foo ();
diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols4.C 
b/gcc/testsuite/g++.target/aarch64/mv-symbols4.C
index 73e3279ec31..4b25d17cc15 100644
--- a/gcc/testsuite/g++.target/aarch64/mv-symbols4.C
+++ b/gcc/testsuite/g++.target/aarch64/mv-symbols4.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_version("default")))
int foo ()
diff --git a/gcc/testsuite/g++.target/aarch64/mv-symbols5.C 
b/gcc/testsuite/g++.target/aarch64/mv-symbols5.C
index 05d1379f53e..fac00b20313 100644
--- a/gcc/testsuite/g++.target/aarch64/mv-symbols5.C
+++ b/gcc/testsuite/g++.target/aarch64/mv-symbols5.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_version("default")))
int foo ();
diff --git a/gcc/testsuite/g++.target/aarch64/mv-warning1.C 
b/gcc/testsuite/g++.target/aarch64/mv-warning1.C
new file mode 100644
index 00000000000..0addd0c9772
--- /dev/null
+++ b/gcc/testsuite/g++.target/aarch64/mv-warning1.C
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-require-ifunc "" } */
+/* { dg-options "-O0" } */
+
+__attribute__((target_version("default")))
+int foo () { return 1; }/* { dg-warning "Function Multi Versioning support is 
experimental, and the behavior is likely to change" } */
+
+__attribute__((target_version("rng")))
+int foo () { return 1; }/* { dg-warning "Function Multi Versioning support is 
experimental, and the behavior is likely to change" } */
diff --git a/gcc/testsuite/g++.target/aarch64/mvc-symbols1.C 
b/gcc/testsuite/g++.target/aarch64/mvc-symbols1.C
index 2dd7c79f16c..983194d74af 100644
--- a/gcc/testsuite/g++.target/aarch64/mvc-symbols1.C
+++ b/gcc/testsuite/g++.target/aarch64/mvc-symbols1.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_clones("default", "dotprod", "sve+sve2")))
int foo ()
diff --git a/gcc/testsuite/g++.target/aarch64/mvc-symbols2.C 
b/gcc/testsuite/g++.target/aarch64/mvc-symbols2.C
index 75b9c126dd8..58a797947ce 100644
--- a/gcc/testsuite/g++.target/aarch64/mvc-symbols2.C
+++ b/gcc/testsuite/g++.target/aarch64/mvc-symbols2.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_clones("default", "dotprod", "sve+sve2")))
int foo ()
diff --git a/gcc/testsuite/g++.target/aarch64/mvc-symbols3.C 
b/gcc/testsuite/g++.target/aarch64/mvc-symbols3.C
index 82e777c8fc6..350a5586643 100644
--- a/gcc/testsuite/g++.target/aarch64/mvc-symbols3.C
+++ b/gcc/testsuite/g++.target/aarch64/mvc-symbols3.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_clones("default", "dotprod", "sve+sve2")))
int foo ();
diff --git a/gcc/testsuite/g++.target/aarch64/mvc-symbols4.C 
b/gcc/testsuite/g++.target/aarch64/mvc-symbols4.C
index 6c86ae61e5f..9c8a7bd37f2 100644
--- a/gcc/testsuite/g++.target/aarch64/mvc-symbols4.C
+++ b/gcc/testsuite/g++.target/aarch64/mvc-symbols4.C
@@ -1,6 +1,7 @@
/* { dg-do compile } */
/* { dg-require-ifunc "" } */
/* { dg-options "-O0" } */
+/* { dg-additional-options "-Wno-experimental-fmv-target" } */

__attribute__((target_clones("default", "dotprod", "sve+sve2")))
int foo ();