diff mbox series

Add warning for use of non-spec FMV in Aarch64

Message ID 20250109095833.2455898-1-alfie.richards@arm.com
State New
Headers
Series Add warning for use of non-spec FMV in Aarch64 |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed

Commit Message

Alfie Richards Jan. 9, 2025, 9:58 a.m. UTC 
  This patch adds a warning whenever 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
future compatability issues.

There is a patch for and ACLE compliant version of target_version and
target_clone coming eventually 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                   | 3 +++
 gcc/config/aarch64/aarch64.opt                  | 4 ++++
 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 +
 13 files changed, 26 insertions(+)
 create mode 100644 gcc/testsuite/g++.target/aarch64/mv-warning1.C

Comments

Kyrylo Tkachov Jan. 9, 2025, 10:41 a.m. UTC | #1 
Hi Alfie,

> On 9 Jan 2025, at 10:58, alfie.richards@arm.com wrote:
> 
> This patch adds a warning whenever 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
> future compatability issues.
> 
> There is a patch for and ACLE compliant version of target_version and
> target_clone coming eventually 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                   | 3 +++
> gcc/config/aarch64/aarch64.opt                  | 4 ++++
> 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 +
> 13 files changed, 26 insertions(+)
> create mode 100644 gcc/testsuite/g++.target/aarch64/mv-warning1.C
> 
> <0001-Add-warning-for-use-of-non-spec-FMV-in-Aarch64.patch>

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 91de13159cb..afc0749fd67 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -20347,6 +20347,9 @@ 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");
+
Some wording nits.

Space before the “(“.
I think there should be no ‘-‘ here to keep consistent with the ACLE wording. Not sure whether Function Multi Versioning should be capitalised. What do you think?

diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt
index 36bc719b822..55670eeb74f 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

Should this have aarch64 in the name somehow? It feels awkward to have aarch64 in the name, but the option is not generic.
In any case, this should be documented in invoke.texi.
I also think that from a user experience POV if they get this warning they may ask what the recommendation is.
Should they change their source? Be prepared that the spec will change in future releases??
The documentation should give some guidance

Thanks,
Kyrill
Alfie Richards Jan. 9, 2025, 1:01 p.m. UTC | #2 
Hi Kyrylo,

(resending due to missing CC)

On 09/01/2025 10:41, Kyrylo Tkachov wrote:
> diff --git a/gcc/config/aarch64/aarch64.cc 
> b/gcc/config/aarch64/aarch64.cc
> index 91de13159cb..afc0749fd67 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -20347,6 +20347,9 @@ 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");
> +
> Some wording nits.
>
> Space before the “(“.
Thank you, I will fix.
> I think there should be no ‘-‘ here to keep consistent with the ACLE 
> wording. Not sure whether Function Multi Versioning should be 
> capitalised. What do you think?
Ah yes, best to follow the ACLE for formatting I suppose. I will 
capitalise and remove the hyphen to match.
> diff --git a/gcc/config/aarch64/aarch64.opt 
> b/gcc/config/aarch64/aarch64.opt
> index 36bc719b822..55670eeb74f 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
>
> Should this have aarch64 in the name somehow? It feels awkward to have 
> aarch64 in the name, but the option is not generic.
I was following the Wopenacc-dims in designing this which is a target 
specific option for gcn
that isn't named overly specifically.

I also don't think this necessarily has to be limited to Aarch64. I'm 
aware other ports have
specified the behaviour of target_version like Aarch64 has which GCC 
currently doesn't conform to
and so may want to issue the same warning.
> In any case, this should be documented in invoke.texi.
Will send a patch adding this.
> I also think that from a user experience POV if they get this warning 
> they may ask what the recommendation is.
> Should they change their source? Be prepared that the spec will change 
> in future releases??
> The documentation should give some guidance

I have added to the warning (in upcoming patch) that the behaviour is 
likely to change.
I will also add similar to the documentation.

Thank you for the feedback! I will get an updated patch to you shortly.
Alfie Richards
>
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 91de13159cb..afc0749fd67 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -20347,6 +20347,9 @@  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");
+
       aarch64_fmv_feature_mask feature_mask = get_feature_mask_for_version (decl);
 
       std::string name = IDENTIFIER_POINTER (id);
diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt
index 36bc719b822..55670eeb74f 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/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..f1a9ecf8ed5
--- /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" } */
+
+__attribute__((target_version("rng")))
+int foo () { return 1; }/* { dg-warning "function multi-versioning support is experimental" } */
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 ();