public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH 0/5] aarch64: FMV feature list fixes
@ 2024-04-09 13:24 Andrew Carlotti
  2024-04-09 13:24 ` [PATCH 1/5] aarch64: Reorder FMV feature priorities Andrew Carlotti
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-09 13:24 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Sandiford, Richard Earnshaw

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?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] aarch64: Reorder FMV feature priorities
  2024-04-09 13:24 [PATCH 0/5] aarch64: FMV feature list fixes Andrew Carlotti
@ 2024-04-09 13:24 ` Andrew Carlotti
  2024-04-09 13:25 ` [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length Andrew Carlotti
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-09 13:24 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Sandiford, Richard Earnshaw

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, ())

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length
  2024-04-09 13:24 [PATCH 0/5] aarch64: FMV feature list fixes Andrew Carlotti
  2024-04-09 13:24 ` [PATCH 1/5] aarch64: Reorder FMV feature priorities Andrew Carlotti
@ 2024-04-09 13:25 ` Andrew Carlotti
  2024-04-09 15:33   ` Richard Sandiford
  2024-04-09 13:26 ` [PATCH 3/5] aarch64: Fix typo and make rdma/rdm alias for FMV Andrew Carlotti
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-09 13:25 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Sandiford, Richard Earnshaw

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)
 	    {

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/5] aarch64: Fix typo and make rdma/rdm alias for FMV
  2024-04-09 13:24 [PATCH 0/5] aarch64: FMV feature list fixes Andrew Carlotti
  2024-04-09 13:24 ` [PATCH 1/5] aarch64: Reorder FMV feature priorities Andrew Carlotti
  2024-04-09 13:25 ` [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length Andrew Carlotti
@ 2024-04-09 13:26 ` Andrew Carlotti
  2024-04-09 13:26 ` [PATCH 4/5] aarch64: Remove unsupported FMV features Andrew Carlotti
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-09 13:26 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Sandiford, Richard Earnshaw

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[] = {

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 4/5] aarch64: Remove unsupported FMV features
  2024-04-09 13:24 [PATCH 0/5] aarch64: FMV feature list fixes Andrew Carlotti
                   ` (2 preceding siblings ...)
  2024-04-09 13:26 ` [PATCH 3/5] aarch64: Fix typo and make rdma/rdm alias for FMV Andrew Carlotti
@ 2024-04-09 13:26 ` Andrew Carlotti
  2024-04-09 13:27 ` [PATCH 5/5] aarch64: Combine some " Andrew Carlotti
  2024-04-09 15:43 ` [PATCH 0/5] aarch64: FMV feature list fixes Richard Sandiford
  5 siblings, 0 replies; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-09 13:26 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Sandiford, Richard Earnshaw

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))

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 5/5] aarch64: Combine some FMV features
  2024-04-09 13:24 [PATCH 0/5] aarch64: FMV feature list fixes Andrew Carlotti
                   ` (3 preceding siblings ...)
  2024-04-09 13:26 ` [PATCH 4/5] aarch64: Remove unsupported FMV features Andrew Carlotti
@ 2024-04-09 13:27 ` Andrew Carlotti
  2024-04-09 15:43 ` [PATCH 0/5] aarch64: FMV feature list fixes Richard Sandiford
  5 siblings, 0 replies; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-09 13:27 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Sandiford, Richard Earnshaw

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), (), (), "")
 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length
  2024-04-09 13:25 ` [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length Andrew Carlotti
@ 2024-04-09 15:33   ` Richard Sandiford
  2024-04-10 12:33     ` Andrew Carlotti
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Sandiford @ 2024-04-09 15:33 UTC (permalink / raw)
  To: Andrew Carlotti; +Cc: gcc-patches, Richard Earnshaw

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)
>  	    {

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] aarch64: FMV feature list fixes
  2024-04-09 13:24 [PATCH 0/5] aarch64: FMV feature list fixes Andrew Carlotti
                   ` (4 preceding siblings ...)
  2024-04-09 13:27 ` [PATCH 5/5] aarch64: Combine some " Andrew Carlotti
@ 2024-04-09 15:43 ` Richard Sandiford
  2024-04-10 11:10   ` Andrew Carlotti
  5 siblings, 1 reply; 14+ messages in thread
From: Richard Sandiford @ 2024-04-09 15:43 UTC (permalink / raw)
  To: Andrew Carlotti; +Cc: gcc-patches, Richard Earnshaw

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] aarch64: FMV feature list fixes
  2024-04-09 15:43 ` [PATCH 0/5] aarch64: FMV feature list fixes Richard Sandiford
@ 2024-04-10 11:10   ` Andrew Carlotti
  2024-04-10 16:42     ` Richard Sandiford
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-10 11:10 UTC (permalink / raw)
  To: gcc-patches, Richard Earnshaw, richard.sandiford

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length
  2024-04-09 15:33   ` Richard Sandiford
@ 2024-04-10 12:33     ` Andrew Carlotti
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-10 12:33 UTC (permalink / raw)
  To: gcc-patches, Richard Earnshaw, richard.sandiford

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)
> >  	    {

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] aarch64: FMV feature list fixes
  2024-04-10 11:10   ` Andrew Carlotti
@ 2024-04-10 16:42     ` Richard Sandiford
  2024-04-10 17:11       ` Andrew Carlotti
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Sandiford @ 2024-04-10 16:42 UTC (permalink / raw)
  To: Andrew Carlotti; +Cc: gcc-patches, Richard Earnshaw

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] aarch64: FMV feature list fixes
  2024-04-10 16:42     ` Richard Sandiford
@ 2024-04-10 17:11       ` Andrew Carlotti
  2024-04-10 18:51         ` Richard Sandiford
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-10 17:11 UTC (permalink / raw)
  To: gcc-patches, Richard Earnshaw, richard.sandiford

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] aarch64: FMV feature list fixes
  2024-04-10 17:11       ` Andrew Carlotti
@ 2024-04-10 18:51         ` Richard Sandiford
  2024-04-10 19:03           ` Andrew Carlotti
  0 siblings, 1 reply; 14+ messages in thread
From: Richard Sandiford @ 2024-04-10 18:51 UTC (permalink / raw)
  To: Andrew Carlotti; +Cc: gcc-patches, Richard Earnshaw

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] aarch64: FMV feature list fixes
  2024-04-10 18:51         ` Richard Sandiford
@ 2024-04-10 19:03           ` Andrew Carlotti
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Carlotti @ 2024-04-10 19:03 UTC (permalink / raw)
  To: gcc-patches, Richard Earnshaw, richard.sandiford

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2024-04-10 19:03 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-09 13:24 [PATCH 0/5] aarch64: FMV feature list fixes Andrew Carlotti
2024-04-09 13:24 ` [PATCH 1/5] aarch64: Reorder FMV feature priorities Andrew Carlotti
2024-04-09 13:25 ` [PATCH 2/5] aarch64: Don't use FEAT_MAX as array length Andrew Carlotti
2024-04-09 15:33   ` Richard Sandiford
2024-04-10 12:33     ` Andrew Carlotti
2024-04-09 13:26 ` [PATCH 3/5] aarch64: Fix typo and make rdma/rdm alias for FMV Andrew Carlotti
2024-04-09 13:26 ` [PATCH 4/5] aarch64: Remove unsupported FMV features Andrew Carlotti
2024-04-09 13:27 ` [PATCH 5/5] aarch64: Combine some " Andrew Carlotti
2024-04-09 15:43 ` [PATCH 0/5] aarch64: FMV feature list fixes Richard Sandiford
2024-04-10 11:10   ` Andrew Carlotti
2024-04-10 16:42     ` Richard Sandiford
2024-04-10 17:11       ` Andrew Carlotti
2024-04-10 18:51         ` Richard Sandiford
2024-04-10 19:03           ` Andrew Carlotti

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).