[PATCH 0/5] aarch64: FMV feature list fixes

[PATCH 0/5] aarch64: FMV feature list fixes

[Andrew Carlotti]

The first three patches are trivial changes to the feature list to reflect
recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
features that don't work at the moment, and should be entirely uncontroversial.

Patch 5 handles the remaining cases, where there's an inconsistency in how
features are named in the current FMV specification compared to the existing
command line options.  It might be better to instead preserve the "memtag2",
"ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
version.

Bootstrapped and regression tested on aarch64. Ok for master?

[PATCH 1/5] aarch64: Reorder FMV feature priorities

[Andrew Carlotti]

Some higher priority FMV features were dependent subsets of lower
priority features.  Fix this, using the new priorities specified in
https://github.com/ARM-software/acle/pull/279.

gcc/ChangeLog:

	* config/aarch64/aarch64-option-extensions.def: Reorder FMV entries.


diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index aa3cd99f791c83c5b15291503f3375a7cf2732cd..0078dd092884a94d2a339b5238b8d19747ff9fa1 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -99,17 +99,17 @@ AARCH64_OPT_EXTENSION(NAME, IDENT, REQUIRES, EXPLICIT_ON, EXPLICIT_OFF,	\
 AARCH64_FMV_FEATURE(NAME, IDENT, (IDENT))
 
 
-AARCH64_OPT_EXTENSION("fp", FP, (), (), (), "fp")
-
-AARCH64_OPT_EXTENSION("simd", SIMD, (FP), (), (), "asimd")
-
 AARCH64_OPT_FMV_EXTENSION("rng", RNG, (), (), (), "rng")
 
 AARCH64_OPT_FMV_EXTENSION("flagm", FLAGM, (), (), (), "flagm")
 
 AARCH64_FMV_FEATURE("flagm2", FLAGM2, (FLAGM))
 
-AARCH64_FMV_FEATURE("fp16fml", FP16FML, (F16FML))
+AARCH64_OPT_FMV_EXTENSION("lse", LSE, (), (), (), "atomics")
+
+AARCH64_OPT_FMV_EXTENSION("fp", FP, (), (), (), "fp")
+
+AARCH64_OPT_FMV_EXTENSION("simd", SIMD, (FP), (), (), "asimd")
 
 AARCH64_OPT_FMV_EXTENSION("dotprod", DOTPROD, (SIMD), (), (), "asimddp")
 
@@ -121,12 +121,6 @@ AARCH64_OPT_EXTENSION("rdma", RDMA, (), (SIMD), (), "asimdrdm")
 
 AARCH64_FMV_FEATURE("rmd", RDM, (RDMA))
 
-AARCH64_OPT_FMV_EXTENSION("lse", LSE, (), (), (), "atomics")
-
-AARCH64_FMV_FEATURE("fp", FP, (FP))
-
-AARCH64_FMV_FEATURE("simd", SIMD, (SIMD))
-
 AARCH64_OPT_FMV_EXTENSION("crc", CRC, (), (), (), "crc32")
 
 AARCH64_FMV_FEATURE("sha1", SHA1, ())
@@ -160,6 +154,8 @@ AARCH64_FMV_FEATURE("fp16", FP16, (F16))
    -march=armv8.4-a+nofp16+fp16 enables F16 but not F16FML.  */
 AARCH64_OPT_EXTENSION("fp16fml", F16FML, (), (F16), (), "asimdfhm")
 
+AARCH64_FMV_FEATURE("fp16fml", FP16FML, (F16FML))
+
 AARCH64_FMV_FEATURE("dit", DIT, ())
 
 AARCH64_FMV_FEATURE("dpb", DPB, ())

[PATCH 2/5] aarch64: Don't use FEAT_MAX as array length

[Andrew Carlotti]

There was an assumption in some places that the aarch64_fmv_feature_data
array contained FEAT_MAX elements.  While this assumption held up till
now, it is safer and more flexible to use the array size directly.

gcc/ChangeLog:

	* config/aarch64/aarch64.cc (compare_feature_masks):
	Use ARRAY_SIZE to determine iteration bounds.
	(aarch64_mangle_decl_assembler_name): Ditto.


diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 1ea84c8bd7386e399f6ffa3a5e36408cf8831fc6..5de842fcc212c78beba1fa99639e79562d718579 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -19899,7 +19899,8 @@ compare_feature_masks (aarch64_fmv_feature_mask mask1,
   auto diff_mask = mask1 ^ mask2;
   if (diff_mask == 0ULL)
     return 0;
-  for (int i = FEAT_MAX - 1; i > 0; i--)
+  static const int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
+  for (int i = num_features - 1; i > 0; i--)
     {
       auto bit_mask = aarch64_fmv_feature_data[i].feature_mask;
       if (diff_mask & bit_mask)
@@ -19982,7 +19983,8 @@ aarch64_mangle_decl_assembler_name (tree decl, tree id)
 
       name += "._";
 
-      for (int i = 0; i < FEAT_MAX; i++)
+      static const int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
+      for (int i = 0; i < num_features; i++)
 	{
 	  if (feature_mask & aarch64_fmv_feature_data[i].feature_mask)
 	    {

[PATCH 3/5] aarch64: Fix typo and make rdma/rdm alias for FMV

[Andrew Carlotti]

gcc/ChangeLog:

	* config/aarch64/aarch64-option-extensions.def:
	Fix "rmd"->"rdm", and add FMV to "rdma".
	* config/aarch64/aarch64.cc (FEAT_RDMA): Define as FEAT_RDM.


diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index 0078dd092884a94d2a339b5238b8d19747ff9fa1..b7b307b24eadd83a6d083955f5b30814b7212712 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -117,9 +117,10 @@ AARCH64_OPT_FMV_EXTENSION("sm4", SM4, (SIMD), (), (), "sm3 sm4")
 
 /* An explicit +rdma implies +simd, but +rdma+nosimd still enables scalar
    RDMA instructions.  */
-AARCH64_OPT_EXTENSION("rdma", RDMA, (), (SIMD), (), "asimdrdm")
+AARCH64_OPT_FMV_EXTENSION("rdma", RDMA, (), (SIMD), (), "asimdrdm")
 
-AARCH64_FMV_FEATURE("rmd", RDM, (RDMA))
+/* rdm is an alias for rdma.  */
+AARCH64_FMV_FEATURE("rdm", RDM, (RDMA))
 
 AARCH64_OPT_FMV_EXTENSION("crc", CRC, (), (), (), "crc32")
 
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 5de842fcc212c78beba1fa99639e79562d718579..b5c51b2cf1dd2f15e0ee1ac21d7959507c05c298 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -19657,6 +19657,10 @@ typedef struct
 #define AARCH64_FMV_FEATURE(NAME, FEAT_NAME, C) \
   {NAME, 1ULL << FEAT_##FEAT_NAME, ::feature_deps::fmv_deps_##FEAT_NAME},
 
+/* The "rdma" alias uses a different FEAT_NAME to avoid a duplicate
+   feature_deps name.  */
+#define FEAT_RDMA FEAT_RDM
+
 /* FMV features are listed in priority order, to make it easier to sort target
    strings.  */
 static aarch64_fmv_feature_datum aarch64_fmv_feature_data[] = {

[PATCH 4/5] aarch64: Remove unsupported FMV features

[Andrew Carlotti]

It currently isn't possible to support function multiversioning features
properly in GCC without also enabling the extension in the command line
options (with the exception of features such as "rpres" that do not
require assembler support).  We therefore remove unsupported features
from GCC's list of FMV features.

Some of these features ("fcma", "jscvt", "frintts", "flagm2", "wfxt",
"rcpc2", and perhaps "dpb" and "dpb2") will be added back in the future
once support for the command line option has been added.

The rest of the removed features I have proposed removing from the ACLE
specification as well, since it doesn't seem worthwhile to include support
for them; see the ACLE pull request for more detailed justification:
https://github.com/ARM-software/acle/pull/315

gcc/ChangeLog:

	* config/aarch64/aarch64-option-extensions.def:
	Remove "flagm2", "sha1", "pmull", "dit", "dpb", "dpb2", "jscvt",
	"fcma", "rcpc2", "frintts", "dgh", "ebf16", "sve-bf16",
	"sve-ebf16", "sve-i8mm", "sve2-pmull128", "memtag3", "bti" and
	"wfxt" entries.


diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index b7b307b24eadd83a6d083955f5b30814b7212712..54bbf9c41e794786dffd69dd103fcbbca0a49f1f 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -103,8 +103,6 @@ AARCH64_OPT_FMV_EXTENSION("rng", RNG, (), (), (), "rng")
 
 AARCH64_OPT_FMV_EXTENSION("flagm", FLAGM, (), (), (), "flagm")
 
-AARCH64_FMV_FEATURE("flagm2", FLAGM2, (FLAGM))
-
 AARCH64_OPT_FMV_EXTENSION("lse", LSE, (), (), (), "atomics")
 
 AARCH64_OPT_FMV_EXTENSION("fp", FP, (), (), (), "fp")
@@ -124,16 +122,12 @@ AARCH64_FMV_FEATURE("rdm", RDM, (RDMA))
 
 AARCH64_OPT_FMV_EXTENSION("crc", CRC, (), (), (), "crc32")
 
-AARCH64_FMV_FEATURE("sha1", SHA1, ())
-
 AARCH64_OPT_FMV_EXTENSION("sha2", SHA2, (SIMD), (), (), "sha1 sha2")
 
 AARCH64_FMV_FEATURE("sha3", SHA3, (SHA3))
 
 AARCH64_OPT_FMV_EXTENSION("aes", AES, (SIMD), (), (), "aes")
 
-AARCH64_FMV_FEATURE("pmull", PMULL, ())
-
 /* +nocrypto disables AES, SHA2 and SM4, and anything that depends on them
    (such as SHA3 and the SVE2 crypto extensions).  */
 AARCH64_OPT_EXTENSION("crypto", CRYPTO, (AES, SHA2), (), (AES, SHA2, SM4),
@@ -157,44 +151,20 @@ AARCH64_OPT_EXTENSION("fp16fml", F16FML, (), (F16), (), "asimdfhm")
 
 AARCH64_FMV_FEATURE("fp16fml", FP16FML, (F16FML))
 
-AARCH64_FMV_FEATURE("dit", DIT, ())
-
-AARCH64_FMV_FEATURE("dpb", DPB, ())
-
-AARCH64_FMV_FEATURE("dpb2", DPB2, ())
-
-AARCH64_FMV_FEATURE("jscvt", JSCVT, ())
-
-AARCH64_FMV_FEATURE("fcma", FCMA, (SIMD))
-
 AARCH64_OPT_FMV_EXTENSION("rcpc", RCPC, (), (), (), "lrcpc")
 
-AARCH64_FMV_FEATURE("rcpc2", RCPC2, (RCPC))
-
 AARCH64_OPT_FMV_EXTENSION("rcpc3", RCPC3, (), (), (), "lrcpc3")
 
-AARCH64_FMV_FEATURE("frintts", FRINTTS, ())
-
-AARCH64_FMV_FEATURE("dgh", DGH, ())
-
 AARCH64_OPT_FMV_EXTENSION("i8mm", I8MM, (SIMD), (), (), "i8mm")
 
 /* An explicit +bf16 implies +simd, but +bf16+nosimd still enables scalar BF16
    instructions.  */
 AARCH64_OPT_FMV_EXTENSION("bf16", BF16, (FP), (SIMD), (), "bf16")
 
-AARCH64_FMV_FEATURE("ebf16", EBF16, (BF16))
-
 AARCH64_FMV_FEATURE("rpres", RPRES, ())
 
 AARCH64_OPT_FMV_EXTENSION("sve", SVE, (SIMD, F16), (), (), "sve")
 
-AARCH64_FMV_FEATURE("sve-bf16", SVE_BF16, (SVE, BF16))
-
-AARCH64_FMV_FEATURE("sve-ebf16", SVE_EBF16, (SVE, BF16))
-
-AARCH64_FMV_FEATURE("sve-i8mm", SVE_I8MM, (SVE, I8MM))
-
 AARCH64_OPT_EXTENSION("f32mm", F32MM, (SVE), (), (), "f32mm")
 
 AARCH64_FMV_FEATURE("f32mm", SVE_F32MM, (F32MM))
@@ -209,8 +179,6 @@ AARCH64_OPT_EXTENSION("sve2-aes", SVE2_AES, (SVE2, AES), (), (), "sveaes")
 
 AARCH64_FMV_FEATURE("sve2-aes", SVE_AES, (SVE2_AES))
 
-AARCH64_FMV_FEATURE("sve2-pmull128", SVE_PMULL128, (SVE2))
-
 AARCH64_OPT_EXTENSION("sve2-bitperm", SVE2_BITPERM, (SVE2), (), (),
 		      "svebitperm")
 
@@ -230,8 +198,6 @@ AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
 
 AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
 
-AARCH64_FMV_FEATURE("memtag3", MEMTAG3, (MEMTAG))
-
 AARCH64_OPT_FMV_EXTENSION("sb", SB, (), (), (), "sb")
 
 AARCH64_OPT_FMV_EXTENSION("predres", PREDRES, (), (), (), "")
@@ -240,8 +206,6 @@ AARCH64_OPT_FMV_EXTENSION("ssbs", SSBS, (), (), (), "ssbs")
 
 AARCH64_FMV_FEATURE("ssbs2", SSBS2, (SSBS))
 
-AARCH64_FMV_FEATURE("bti", BTI, ())
-
 AARCH64_OPT_EXTENSION("profile", PROFILE, (), (), (), "")
 
 AARCH64_OPT_EXTENSION("tme", TME, (), (), (), "")
@@ -256,8 +220,6 @@ AARCH64_FMV_FEATURE("ls64_v", LS64_V, ())
 
 AARCH64_FMV_FEATURE("ls64_accdata", LS64_ACCDATA, (LS64))
 
-AARCH64_FMV_FEATURE("wfxt", WFXT, ())
-
 AARCH64_OPT_EXTENSION("sme-f64f64", SME_F64F64, (SME), (), (), "")
 
 AARCH64_FMV_FEATURE("sme-f64f64", SME_F64, (SME_F64F64))

[PATCH 5/5] aarch64: Combine some FMV features

[Andrew Carlotti]

Some architecture features have been combined under a single command
line flag, but have been assigned multiple FMV feature names with the
command line flag name enabling only a subset of these features in
the FMV specification.  Remove the unsupported FMV subfeatures, and
rename the remaining features with the corresponding command line flag
names.  This change is also proposed in the specification:
https://github.com/ARM-software/acle/pull/315

gcc/ChangeLog:

	* config/aarch64/aarch64-option-extensions.def:
	Combine "memtag2" into "memtag", "ssbs2" into "ssbs", and
	"ls64_v and ls64_accdata" into "ls64".


diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index 54bbf9c41e794786dffd69dd103fcbbca0a49f1f..164ee3b8194396e66a61f43d45c199c523d2e7cf 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -194,17 +194,17 @@ AARCH64_FMV_FEATURE("sve2-sm4", SVE_SM4, (SVE2_SM4))
 
 AARCH64_OPT_FMV_EXTENSION("sme", SME, (BF16, SVE2), (), (), "sme")
 
-AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
+AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
 
-AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
+AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
 
 AARCH64_OPT_FMV_EXTENSION("sb", SB, (), (), (), "sb")
 
 AARCH64_OPT_FMV_EXTENSION("predres", PREDRES, (), (), (), "")
 
-AARCH64_OPT_FMV_EXTENSION("ssbs", SSBS, (), (), (), "ssbs")
+AARCH64_OPT_EXTENSION("ssbs", SSBS, (), (), (), "ssbs")
 
-AARCH64_FMV_FEATURE("ssbs2", SSBS2, (SSBS))
+AARCH64_FMV_FEATURE("ssbs", SSBS2, (SSBS))
 
 AARCH64_OPT_EXTENSION("profile", PROFILE, (), (), (), "")
 
@@ -214,11 +214,7 @@ AARCH64_OPT_EXTENSION("pauth", PAUTH, (), (), (), "paca pacg")
 
 AARCH64_OPT_EXTENSION("ls64", LS64, (), (), (), "")
 
-AARCH64_FMV_FEATURE("ls64", LS64, ())
-
-AARCH64_FMV_FEATURE("ls64_v", LS64_V, ())
-
-AARCH64_FMV_FEATURE("ls64_accdata", LS64_ACCDATA, (LS64))
+AARCH64_FMV_FEATURE("ls64", LS64_ACCDATA, (LS64))
 
 AARCH64_OPT_EXTENSION("sme-f64f64", SME_F64F64, (SME), (), (), "")
 

Re: [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length

[Richard Sandiford]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> There was an assumption in some places that the aarch64_fmv_feature_data
> array contained FEAT_MAX elements.  While this assumption held up till
> now, it is safer and more flexible to use the array size directly.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.cc (compare_feature_masks):
> 	Use ARRAY_SIZE to determine iteration bounds.
> 	(aarch64_mangle_decl_assembler_name): Ditto.
>
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 1ea84c8bd7386e399f6ffa3a5e36408cf8831fc6..5de842fcc212c78beba1fa99639e79562d718579 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -19899,7 +19899,8 @@ compare_feature_masks (aarch64_fmv_feature_mask mask1,
>    auto diff_mask = mask1 ^ mask2;
>    if (diff_mask == 0ULL)
>      return 0;
> -  for (int i = FEAT_MAX - 1; i > 0; i--)
> +  static const int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);

There doesn't seem any need for this to be static (or const).  Same for
the second hunk.

> +  for (int i = num_features - 1; i > 0; i--)

Pre-existing, but is > 0 rather than >= 0 deliberate?  Shouldn't we look
at index 0 as well?

LGTM otherwise.

Thanks,
Richard

>      {
>        auto bit_mask = aarch64_fmv_feature_data[i].feature_mask;
>        if (diff_mask & bit_mask)
> @@ -19982,7 +19983,8 @@ aarch64_mangle_decl_assembler_name (tree decl, tree id)
>  
>        name += "._";
>  
> -      for (int i = 0; i < FEAT_MAX; i++)
> +      static const int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
> +      for (int i = 0; i < num_features; i++)
>  	{
>  	  if (feature_mask & aarch64_fmv_feature_data[i].feature_mask)
>  	    {

Re: [PATCH 0/5] aarch64: FMV feature list fixes

[Richard Sandiford]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> The first three patches are trivial changes to the feature list to reflect
> recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> features that don't work at the moment, and should be entirely uncontroversial.
>
> Patch 5 handles the remaining cases, where there's an inconsistency in how
> features are named in the current FMV specification compared to the existing
> command line options.  It might be better to instead preserve the "memtag2",
> "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> version.

Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
since e.g.:

-AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
+AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
 
-AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
+AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))

seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
FEAT_MEMTAG2.  Is that right?

Apart from that and the comment on patch 2, the series looks good to me.

While rechecking aarch64-option-extensions.def against the ACLE list:
it seems that the .def doesn't treat mops as an FMV feature.  Is that
deliberate?

Thanks,
Richard

Re: [PATCH 0/5] aarch64: FMV feature list fixes

[Andrew Carlotti]

On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > The first three patches are trivial changes to the feature list to reflect
> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> > features that don't work at the moment, and should be entirely uncontroversial.
> >
> > Patch 5 handles the remaining cases, where there's an inconsistency in how
> > features are named in the current FMV specification compared to the existing
> > command line options.  It might be better to instead preserve the "memtag2",
> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> > version.
> 
> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
> since e.g.:
> 
> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
>  
> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
> 
> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
> FEAT_MEMTAG2.  Is that right?

That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
match the definition of FEAT_MTE in the architecture, and likewise for
FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
instructions can be generated from GCC without inline assembly).  The FMV
specification in the ACLE currently uses names "memtag" and "memtag2" that
match the architecture names, but arguably don't match the command line
extension names.  I'm advocating for that to change to match the extension
names in command line options.

The LS64 example is definitely an inconsistency, since GCC uses "+ls64" to
enable intrinsics for all of the FEAT_LS64/FEAT_LS64_V/FEAT_LS64_ACCDATA
intrinsics.

There were similar issues with "sha1", "pmull" and "sve2-pmull128", but in
these cases their presence architecturally is implied by the presence of the
features checked for "sha2", "aes" and "sve2-aes" so it's fine to just delete
the ones without command line flags.

> Apart from that and the comment on patch 2, the series looks good to me.
> 
> While rechecking aarch64-option-extensions.def against the ACLE list:
> it seems that the .def doesn't treat mops as an FMV feature.  Is that
> deliberate?

"mops" was added to the ACLE list later, and libgcc doesn't yet support
detecting it.  I didn't think it was sensible to add new FMV feature support at
this stage.

> Thanks,
> Richard

Re: [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length

[Andrew Carlotti]

On Tue, Apr 09, 2024 at 04:33:10PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > There was an assumption in some places that the aarch64_fmv_feature_data
> > array contained FEAT_MAX elements.  While this assumption held up till
> > now, it is safer and more flexible to use the array size directly.
> >
> > gcc/ChangeLog:
> >
> > 	* config/aarch64/aarch64.cc (compare_feature_masks):
> > 	Use ARRAY_SIZE to determine iteration bounds.
> > 	(aarch64_mangle_decl_assembler_name): Ditto.
> >
> >
> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> > index 1ea84c8bd7386e399f6ffa3a5e36408cf8831fc6..5de842fcc212c78beba1fa99639e79562d718579 100644
> > --- a/gcc/config/aarch64/aarch64.cc
> > +++ b/gcc/config/aarch64/aarch64.cc
> > @@ -19899,7 +19899,8 @@ compare_feature_masks (aarch64_fmv_feature_mask mask1,
> >    auto diff_mask = mask1 ^ mask2;
> >    if (diff_mask == 0ULL)
> >      return 0;
> > -  for (int i = FEAT_MAX - 1; i > 0; i--)
> > +  static const int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
> 
> There doesn't seem any need for this to be static (or const).  Same for
> the second hunk.

Agreed - I'll fix that, and the other instance I added in a previous patch.

I originally copied this pattern from my driver-aarch64.c:252, which was added
by Kyrill back in 2015.

> > +  for (int i = num_features - 1; i > 0; i--)
> 
> Pre-existing, but is > 0 rather than >= 0 deliberate?  Shouldn't we look
> at index 0 as well?

That was probably left over from when "default" was handled as part of the
list.  I think a different instance of this mistake was mentioned in a previous
review.  I'll fix this mistake and add a test.

> LGTM otherwise.
> 
> Thanks,
> Richard
> 
> >      {
> >        auto bit_mask = aarch64_fmv_feature_data[i].feature_mask;
> >        if (diff_mask & bit_mask)
> > @@ -19982,7 +19983,8 @@ aarch64_mangle_decl_assembler_name (tree decl, tree id)
> >  
> >        name += "._";
> >  
> > -      for (int i = 0; i < FEAT_MAX; i++)
> > +      static const int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
> > +      for (int i = 0; i < num_features; i++)
> >  	{
> >  	  if (feature_mask & aarch64_fmv_feature_data[i].feature_mask)
> >  	    {

Re: [PATCH 0/5] aarch64: FMV feature list fixes

[Richard Sandiford]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
>> Andrew Carlotti <andrew.carlotti@arm.com> writes:
>> > The first three patches are trivial changes to the feature list to reflect
>> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
>> > features that don't work at the moment, and should be entirely uncontroversial.
>> >
>> > Patch 5 handles the remaining cases, where there's an inconsistency in how
>> > features are named in the current FMV specification compared to the existing
>> > command line options.  It might be better to instead preserve the "memtag2",
>> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
>> > version.
>> 
>> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
>> since e.g.:
>> 
>> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
>> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
>>  
>> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
>> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
>> 
>> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
>> FEAT_MEMTAG2.  Is that right?
>
> That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
> match the definition of FEAT_MTE in the architecture, and likewise for
> FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
> both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
> instructions can be generated from GCC without inline assembly).  The FMV
> specification in the ACLE currently uses names "memtag" and "memtag2" that
> match the architecture names, but arguably don't match the command line
> extension names.  I'm advocating for that to change to match the extension
> names in command line options.

Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
the same names as the architecture or (b) use exactly the same names as the
command-line (mangled where necessary)?  It seems that we're instead
using a third convention that doesn't exactly match the other two.

That is, I can see the rationale for "memtag" => FEAT_MTE2 and
"memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
(where MEMTAG2 is an alias of MTE2).

How much leeway do we have to change the __aarch64_cpu_features names?
Is it supposed to be a public API (as opposed to ABI)?

> The LS64 example is definitely an inconsistency, since GCC uses "+ls64" to
> enable intrinsics for all of the FEAT_LS64/FEAT_LS64_V/FEAT_LS64_ACCDATA
> intrinsics.

Ok, thanks.  If we go for option (a) above then I agree that the ls64
change is correct.  If we go for option (b) then I suppose it should
stay as LS64.

> There were similar issues with "sha1", "pmull" and "sve2-pmull128", but in
> these cases their presence architecturally is implied by the presence of the
> features checked for "sha2", "aes" and "sve2-aes" so it's fine to just delete
> the ones without command line flags.
>
>> Apart from that and the comment on patch 2, the series looks good to me.
>> 
>> While rechecking aarch64-option-extensions.def against the ACLE list:
>> it seems that the .def doesn't treat mops as an FMV feature.  Is that
>> deliberate?
>
> "mops" was added to the ACLE list later, and libgcc doesn't yet support
> detecting it.  I didn't think it was sensible to add new FMV feature support at
> this stage.

Ah, ok, makes sense.

Richard

Re: [PATCH 0/5] aarch64: FMV feature list fixes

[Andrew Carlotti]

On Wed, Apr 10, 2024 at 05:42:05PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> >> > The first three patches are trivial changes to the feature list to reflect
> >> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> >> > features that don't work at the moment, and should be entirely uncontroversial.
> >> >
> >> > Patch 5 handles the remaining cases, where there's an inconsistency in how
> >> > features are named in the current FMV specification compared to the existing
> >> > command line options.  It might be better to instead preserve the "memtag2",
> >> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> >> > version.
> >> 
> >> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
> >> since e.g.:
> >> 
> >> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >>  
> >> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
> >> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
> >> 
> >> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
> >> FEAT_MEMTAG2.  Is that right?
> >
> > That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
> > match the definition of FEAT_MTE in the architecture, and likewise for
> > FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
> > both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
> > instructions can be generated from GCC without inline assembly).  The FMV
> > specification in the ACLE currently uses names "memtag" and "memtag2" that
> > match the architecture names, but arguably don't match the command line
> > extension names.  I'm advocating for that to change to match the extension
> > names in command line options.
> 
> Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
> the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
> the same names as the architecture or (b) use exactly the same names as the
> command-line (mangled where necessary)?  It seems that we're instead
> using a third convention that doesn't exactly match the other two.

I agree that the name isn't one I would choose now, but I don't think it matters much that it's inconsistent.

> That is, I can see the rationale for "memtag" => FEAT_MTE2 and
> "memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
> (where MEMTAG2 is an alias of MTE2).
> 
> How much leeway do we have to change the __aarch64_cpu_features names?
> Is it supposed to be a public API (as opposed to ABI)?

I think we're designing it to be capable of being a public API, but we haven't
yet made it one.  That's partly why I've kept the enum value names the same as
in LLVM so far.

> > The LS64 example is definitely an inconsistency, since GCC uses "+ls64" to
> > enable intrinsics for all of the FEAT_LS64/FEAT_LS64_V/FEAT_LS64_ACCDATA
> > intrinsics.
> 
> Ok, thanks.  If we go for option (a) above then I agree that the ls64
> change is correct.  If we go for option (b) then I suppose it should
> stay as LS64.
> 
> > There were similar issues with "sha1", "pmull" and "sve2-pmull128", but in
> > these cases their presence architecturally is implied by the presence of the
> > features checked for "sha2", "aes" and "sve2-aes" so it's fine to just delete
> > the ones without command line flags.
> >
> >> Apart from that and the comment on patch 2, the series looks good to me.
> >> 
> >> While rechecking aarch64-option-extensions.def against the ACLE list:
> >> it seems that the .def doesn't treat mops as an FMV feature.  Is that
> >> deliberate?
> >
> > "mops" was added to the ACLE list later, and libgcc doesn't yet support
> > detecting it.  I didn't think it was sensible to add new FMV feature support at
> > this stage.
> 
> Ah, ok, makes sense.
> 
> Richard

Re: [PATCH 0/5] aarch64: FMV feature list fixes

[Richard Sandiford]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> On Wed, Apr 10, 2024 at 05:42:05PM +0100, Richard Sandiford wrote:
>> Andrew Carlotti <andrew.carlotti@arm.com> writes:
>> > On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
>> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
>> >> > The first three patches are trivial changes to the feature list to reflect
>> >> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
>> >> > features that don't work at the moment, and should be entirely uncontroversial.
>> >> >
>> >> > Patch 5 handles the remaining cases, where there's an inconsistency in how
>> >> > features are named in the current FMV specification compared to the existing
>> >> > command line options.  It might be better to instead preserve the "memtag2",
>> >> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
>> >> > version.
>> >> 
>> >> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
>> >> since e.g.:
>> >> 
>> >> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
>> >> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
>> >>  
>> >> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
>> >> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
>> >> 
>> >> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
>> >> FEAT_MEMTAG2.  Is that right?
>> >
>> > That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
>> > match the definition of FEAT_MTE in the architecture, and likewise for
>> > FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
>> > both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
>> > instructions can be generated from GCC without inline assembly).  The FMV
>> > specification in the ACLE currently uses names "memtag" and "memtag2" that
>> > match the architecture names, but arguably don't match the command line
>> > extension names.  I'm advocating for that to change to match the extension
>> > names in command line options.
>> 
>> Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
>> the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
>> the same names as the architecture or (b) use exactly the same names as the
>> command-line (mangled where necessary)?  It seems that we're instead
>> using a third convention that doesn't exactly match the other two.
>
> I agree that the name isn't one I would choose now, but I don't think it matters much that it's inconsistent.

I kind-of think it does though.  Given...

>> That is, I can see the rationale for "memtag" => FEAT_MTE2 and
>> "memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
>> (where MEMTAG2 is an alias of MTE2).
>> 
>> How much leeway do we have to change the __aarch64_cpu_features names?
>> Is it supposed to be a public API (as opposed to ABI)?
>
> I think we're designing it to be capable of being a public API, but we haven't
> yet made it one.  That's partly why I've kept the enum value names the same as
> in LLVM so far.

...this, I don't want to sleep-walk into a situation where we have
one naming convention for the architecture, one for the attributes,
and a third one for the API.  If we're not in a position to commit
to a consistent naming scheme for the API by GCC 14 then it might be
better to remove the FMV features in 5/5 for GCC 14 and revisit in GCC 15.

A patch to do that is pre-approved if you agree (but please say
if you don't).

Thanks,
Richard

Re: [PATCH 0/5] aarch64: FMV feature list fixes

[Andrew Carlotti]

On Wed, Apr 10, 2024 at 07:51:44PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> > On Wed, Apr 10, 2024 at 05:42:05PM +0100, Richard Sandiford wrote:
> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> >> > On Tue, Apr 09, 2024 at 04:43:16PM +0100, Richard Sandiford wrote:
> >> >> Andrew Carlotti <andrew.carlotti@arm.com> writes:
> >> >> > The first three patches are trivial changes to the feature list to reflect
> >> >> > recent changes in the ACLE.  Patch 4 removes most of the FMV multiversioning
> >> >> > features that don't work at the moment, and should be entirely uncontroversial.
> >> >> >
> >> >> > Patch 5 handles the remaining cases, where there's an inconsistency in how
> >> >> > features are named in the current FMV specification compared to the existing
> >> >> > command line options.  It might be better to instead preserve the "memtag2",
> >> >> > "ssbs2" and "ls64_accdata" names for now; I'd be happy to commit either
> >> >> > version.
> >> >> 
> >> >> Yeah, I suppose patch 5 leaves things in a somewhat awkward state,
> >> >> since e.g.:
> >> >> 
> >> >> -AARCH64_OPT_FMV_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >> >> +AARCH64_OPT_EXTENSION("memtag", MEMTAG, (), (), (), "")
> >> >>  
> >> >> -AARCH64_FMV_FEATURE("memtag2", MEMTAG2, (MEMTAG))
> >> >> +AARCH64_FMV_FEATURE("memtag", MEMTAG2, (MEMTAG))
> >> >> 
> >> >> seems to drop "memtag2" and FEAT_MEMTAG, but keep "memtag" and
> >> >> FEAT_MEMTAG2.  Is that right?
> >> >
> >> > That's deliberate. The FEAT_MEMTAG bit in __aarch64_cpu_features is defined to
> >> > match the definition of FEAT_MTE in the architecture, and likewise for
> >> > FEAT_MEMTAG2/FEAT_MTE2.  However, in Binutils the "+memtag" extension enables
> >> > both FEAT_MTE and FEAT_MTE2 instructions (although none of the FEAT_MTE2
> >> > instructions can be generated from GCC without inline assembly).  The FMV
> >> > specification in the ACLE currently uses names "memtag" and "memtag2" that
> >> > match the architecture names, but arguably don't match the command line
> >> > extension names.  I'm advocating for that to change to match the extension
> >> > names in command line options.
> >> 
> >> Hmm, ok.  I agree it makes sense for the user-visible FMV namnes to match
> >> the command line.  But shouldn't __aarch64_cpu_features either (a) use exactly
> >> the same names as the architecture or (b) use exactly the same names as the
> >> command-line (mangled where necessary)?  It seems that we're instead
> >> using a third convention that doesn't exactly match the other two.
> >
> > I agree that the name isn't one I would choose now, but I don't think it matters much that it's inconsistent.
> 
> I kind-of think it does though.  Given...
> 
> >> That is, I can see the rationale for "memtag" => FEAT_MTE2 and
> >> "memtag" => FEAT_MEMTAG.  It just seems odd to have "memtag" => FEAT_MEMTAG2
> >> (where MEMTAG2 is an alias of MTE2).
> >> 
> >> How much leeway do we have to change the __aarch64_cpu_features names?
> >> Is it supposed to be a public API (as opposed to ABI)?
> >
> > I think we're designing it to be capable of being a public API, but we haven't
> > yet made it one.  That's partly why I've kept the enum value names the same as
> > in LLVM so far.
> 
> ...this, I don't want to sleep-walk into a situation where we have
> one naming convention for the architecture, one for the attributes,
> and a third one for the API.  If we're not in a position to commit
> to a consistent naming scheme for the API by GCC 14 then it might be
> better to remove the FMV features in 5/5 for GCC 14 and revisit in GCC 15.
> 
> A patch to do that is pre-approved if you agree (but please say
> if you don't).

I'm happy to remove those features for GCC 14 (pending agreement on the
attribute names in particular), but I don't think that does anything to solve
the enum names issue.  I'll remove the names from my FMV documentation patch as
well.

> Thanks,
> Richard