[V2,2/7] aarch64: Add support for aarch64-sys-regs.def

  This patch defines the structure of a new .def file used for
representing the aarch64 system registers, what information it should
hold and the basic framework in GCC to process this file.

Entries in the aarch64-system-regs.def file should be as follows:

  SYSREG (NAME, CPENC (sn,op1,cn,cm,op2), FLAG1 | ... | FLAGn, ARCH)

Where the arguments to SYSREG correspond to:
  - NAME:  The system register name, as used in the assembly language.
  - CPENC: The system register encoding, mapping to:

    	       s<sn>_<op1>_c<cn>_c<cm>_<op2>

  - FLAG: The entries in the FLAGS field are bitwise-OR'd together to
    	  encode extra information required to ensure proper use of
	  the system register.  For example, a read-only system
	  register will have the flag F_REG_READ, while write-only
	  registers will be labeled F_REG_WRITE.  Such flags are
	  tested against at compile-time.
  - ARCH: The architectural features the system register is associated
    	  with.  This is encoded via one of three possible macros:
	  1. When a system register is universally implemented, we say
	  it has no feature requirements, so we tag it with the
	  AARCH64_NO_FEATURES macro.
	  2. When a register is only implemented for a single
	  architectural extension EXT, the AARCH64_FEATURE (EXT), is
	  used.
	  3. When a given system register is made available by any of N
	  possible architectural extensions, the AARCH64_FEATURES(N, ...)
	  macro is used to combine them accordingly.

In order to enable proper interpretation of the SYSREG entries by the
compiler, flags defining system register behavior such as `F_REG_READ'
and `F_REG_WRITE' are also defined here, so they can later be used for
the validation of system register properties.

Finally, any architectural feature flags from Binutils missing from GCC
have appropriate aliases defined here so as to ensure
cross-compatibility of SYSREG entries across the toolchain.

gcc/ChangeLog:

	* gcc/config/aarch64/aarch64.cc (sysreg_t): New.
	(sysreg_structs): Likewise.
	(nsysreg): Likewise.
	(AARCH64_FEATURE): Likewise.
	(AARCH64_FEATURES): Likewise.
	(AARCH64_NO_FEATURES): Likewise.
	* gcc/config/aarch64/aarch64.h (AARCH64_ISA_V8A): Add missing
	ISA flag.
	(AARCH64_ISA_V8_1A): Likewise.
	(AARCH64_ISA_V8_7A): Likewise.
	(AARCH64_ISA_V8_8A): Likewise.
	(AARCH64_NO_FEATURES): Likewise.
	(AARCH64_FL_RAS): New ISA flag alias.
	(AARCH64_FL_LOR): Likewise.
	(AARCH64_FL_PAN): Likewise.
	(AARCH64_FL_AMU): Likewise.
	(AARCH64_FL_SCXTNUM): Likewise.
	(AARCH64_FL_ID_PFR2): Likewise.
	(F_DEPRECATED): New.
	(F_REG_READ): Likewise.
	(F_REG_WRITE): Likewise.
	(F_ARCHEXT): Likewise.
	(F_REG_ALIAS): Likewise.
---
 gcc/config/aarch64/aarch64.cc | 38 +++++++++++++++++++++++++++++++++++
 gcc/config/aarch64/aarch64.h  | 36 +++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
  

Victor Do Nascimento <victor.donascimento@arm.com> writes:
> This patch defines the structure of a new .def file used for
> representing the aarch64 system registers, what information it should
> hold and the basic framework in GCC to process this file.
>
> Entries in the aarch64-system-regs.def file should be as follows:
>
>   SYSREG (NAME, CPENC (sn,op1,cn,cm,op2), FLAG1 | ... | FLAGn, ARCH)
>
> Where the arguments to SYSREG correspond to:
>   - NAME:  The system register name, as used in the assembly language.
>   - CPENC: The system register encoding, mapping to:
>
>     	       s<sn>_<op1>_c<cn>_c<cm>_<op2>
>
>   - FLAG: The entries in the FLAGS field are bitwise-OR'd together to
>     	  encode extra information required to ensure proper use of
> 	  the system register.  For example, a read-only system
> 	  register will have the flag F_REG_READ, while write-only
> 	  registers will be labeled F_REG_WRITE.  Such flags are
> 	  tested against at compile-time.
>   - ARCH: The architectural features the system register is associated
>     	  with.  This is encoded via one of three possible macros:
> 	  1. When a system register is universally implemented, we say
> 	  it has no feature requirements, so we tag it with the
> 	  AARCH64_NO_FEATURES macro.
> 	  2. When a register is only implemented for a single
> 	  architectural extension EXT, the AARCH64_FEATURE (EXT), is
> 	  used.
> 	  3. When a given system register is made available by any of N
> 	  possible architectural extensions, the AARCH64_FEATURES(N, ...)
> 	  macro is used to combine them accordingly.
>
> In order to enable proper interpretation of the SYSREG entries by the
> compiler, flags defining system register behavior such as `F_REG_READ'
> and `F_REG_WRITE' are also defined here, so they can later be used for
> the validation of system register properties.
>
> Finally, any architectural feature flags from Binutils missing from GCC
> have appropriate aliases defined here so as to ensure
> cross-compatibility of SYSREG entries across the toolchain.
>
> gcc/ChangeLog:
>
> 	* gcc/config/aarch64/aarch64.cc (sysreg_t): New.
> 	(sysreg_structs): Likewise.
> 	(nsysreg): Likewise.
> 	(AARCH64_FEATURE): Likewise.
> 	(AARCH64_FEATURES): Likewise.
> 	(AARCH64_NO_FEATURES): Likewise.
> 	* gcc/config/aarch64/aarch64.h (AARCH64_ISA_V8A): Add missing
> 	ISA flag.
> 	(AARCH64_ISA_V8_1A): Likewise.
> 	(AARCH64_ISA_V8_7A): Likewise.
> 	(AARCH64_ISA_V8_8A): Likewise.
> 	(AARCH64_NO_FEATURES): Likewise.
> 	(AARCH64_FL_RAS): New ISA flag alias.
> 	(AARCH64_FL_LOR): Likewise.
> 	(AARCH64_FL_PAN): Likewise.
> 	(AARCH64_FL_AMU): Likewise.
> 	(AARCH64_FL_SCXTNUM): Likewise.
> 	(AARCH64_FL_ID_PFR2): Likewise.
> 	(F_DEPRECATED): New.
> 	(F_REG_READ): Likewise.
> 	(F_REG_WRITE): Likewise.
> 	(F_ARCHEXT): Likewise.
> 	(F_REG_ALIAS): Likewise.
> ---
>  gcc/config/aarch64/aarch64.cc | 38 +++++++++++++++++++++++++++++++++++
>  gcc/config/aarch64/aarch64.h  | 36 +++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 9fbfc548a89..69de2366424 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -2807,6 +2807,44 @@ static const struct processor all_cores[] =
>    {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, NULL}
>  };
>  
> +typedef struct {
> +  const char* name;
> +  const char* encoding;

Formatting nit, but GCC style is:

  const char *foo

rather than:

  const char* foo;

> +  const unsigned properties;
> +  const unsigned long long arch_reqs;

I don't think these two should be const.  There's no reason in principle
why a sysreg_t can't be created and modified dynamically.

It would be useful to have some comments above the fields to say what
they represent.  E.g. the definition on its own doesn't make clear what
"properties" refers to.

arch_reqs should use aarch64_feature_flags rather than unsigned long long.
We're running out of feature flags in GCC too, so aarch64_feature_flags
is soon likely to be a C++ class.

> +} sysreg_t;
> +
> +/* An aarch64_feature_set initializer for a single feature,
> +   AARCH64_FEATURE_<FEAT>.  */
> +#define AARCH64_FEATURE(FEAT) AARCH64_FL_##FEAT
> +
> +/* Used by AARCH64_FEATURES.  */
> +#define AARCH64_OR_FEATURES_1(X, F1) \
> +  AARCH64_FEATURE (F1)
> +#define AARCH64_OR_FEATURES_2(X, F1, F2) \
> +  (AARCH64_FEATURE (F1) | AARCH64_OR_FEATURES_1 (X, F2))
> +#define AARCH64_OR_FEATURES_3(X, F1, ...) \
> +  (AARCH64_FEATURE (F1) | AARCH64_OR_FEATURES_2 (X, __VA_ARGS__))
> +
> +/* An aarch64_feature_set initializer for the N features listed in "...".  */
> +#define AARCH64_FEATURES(N, ...) \
> +  AARCH64_OR_FEATURES_##N (0, __VA_ARGS__)
> +
> +/* Database of system registers, their encodings and architectural
> +   requirements.  */
> +const sysreg_t sysreg_structs[] =
> +{
> +#define CPENC(SN, OP1, CN, CM, OP2) "s"#SN"_"#OP1"_c"#CN"_c"#CM"_"#OP2
> +#define SYSREG(NAME, ENC, FLAGS, ARCH) \
> +  { NAME, ENC, FLAGS, ARCH },
> +#include "aarch64-sys-regs.def"
> +#undef CPENC
> +};
> +
> +#define TOTAL_ITEMS (sizeof sysreg_structs / sizeof sysreg_structs[0])
> +const unsigned nsysreg = TOTAL_ITEMS;

There's an ARRAY_SIZE macro for this.

> +#undef TOTAL_ITEMS
> +
>  /* The current tuning set.  */
>  struct tune_params aarch64_tune_params = generic_tunings;
>  
> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
> index d74e9116fc5..cf3969a11aa 100644
> --- a/gcc/config/aarch64/aarch64.h
> +++ b/gcc/config/aarch64/aarch64.h
> @@ -179,6 +179,8 @@ enum class aarch64_feature : unsigned char {
>  
>  /* Macros to test ISA flags.  */
>  
> +#define AARCH64_ISA_V8A		   (aarch64_isa_flags & AARCH64_FL_V8A)
> +#define AARCH64_ISA_V8_1A	   (aarch64_isa_flags & AARCH64_FL_V8_1A)
>  #define AARCH64_ISA_CRC            (aarch64_isa_flags & AARCH64_FL_CRC)
>  #define AARCH64_ISA_CRYPTO         (aarch64_isa_flags & AARCH64_FL_CRYPTO)
>  #define AARCH64_ISA_FP             (aarch64_isa_flags & AARCH64_FL_FP)
> @@ -215,6 +217,8 @@ enum class aarch64_feature : unsigned char {
>  #define AARCH64_ISA_SB		   (aarch64_isa_flags & AARCH64_FL_SB)
>  #define AARCH64_ISA_V8R		   (aarch64_isa_flags & AARCH64_FL_V8R)
>  #define AARCH64_ISA_PAUTH	   (aarch64_isa_flags & AARCH64_FL_PAUTH)
> +#define AARCH64_ISA_V8_7A	   (aarch64_isa_flags & AARCH64_FL_V8_7A)
> +#define AARCH64_ISA_V8_8A	   (aarch64_isa_flags & AARCH64_FL_V8_8A)
>  #define AARCH64_ISA_V9A		   (aarch64_isa_flags & AARCH64_FL_V9A)
>  #define AARCH64_ISA_V9_1A          (aarch64_isa_flags & AARCH64_FL_V9_1A)
>  #define AARCH64_ISA_V9_2A          (aarch64_isa_flags & AARCH64_FL_V9_2A)
> @@ -223,6 +227,38 @@ enum class aarch64_feature : unsigned char {
>  #define AARCH64_ISA_LS64	   (aarch64_isa_flags & AARCH64_FL_LS64)
>  #define AARCH64_ISA_CSSC	   (aarch64_isa_flags & AARCH64_FL_CSSC)
>  
> +/* AARCH64_FL options necessary for system register implementation.  */
> +
> +/* Mask for core system registers.  System registers requiring no architectural
> +   extensions set up a feature-checking mask which returns any passed flags
> +   unchanged when ANDd with.  */
> +#define AARCH64_NO_FEATURES	   (uint64_t)-1

I think this one should only be defined before including the .def file,
and undefined afterwards.  It's not something that general GCC code
should use.

Is -1 right though?  3/7 uses:

  if (aarch64_isa_flags & sysreg->arch_reqs)

which requires at least one of the features in sysreg->arch_reqs to be
available.  But binutils uses AARCH64_CPU_HAS_ALL_FEATURES, which requires
all of the features to be available.

At least, it seems odd that AARCH64_NO_FEATURES is all-ones here
but all-zeros in binutils.

> +
> +/* Define AARCH64_FL aliases for architectural features which are protected
> +   by -march flags in binutils but which receive no special treatment by GCC.
> +
> +   Such flags are inherited from the Binutils definition of system registers
> +   and are mapped to the architecture in which the feature is implemented.  */
> +#define AARCH64_FL_RAS		   AARCH64_FL_V8A
> +#define AARCH64_FL_LOR		   AARCH64_FL_V8_1A
> +#define AARCH64_FL_PAN		   AARCH64_FL_V8_1A
> +#define AARCH64_FL_AMU		   AARCH64_FL_V8_4A
> +#define AARCH64_FL_SCXTNUM	   AARCH64_FL_V8_5A
> +#define AARCH64_FL_ID_PFR2	   AARCH64_FL_V8_5A
> +
> +/* Define AARCH64_FL aliases for features note yet implemented in GCC.
> +   Accept them unconditionally.  */
> +#define AARCH64_FL_SME		   -1

I think disallowing them would be better.  Otherwise we'd go from
accepting a register without +sme to not accepting it, which might
seem like a regression to users.

Of course, in the particular case of SME, this problem should go
away very soon :)  And once this sysreg work goes in, it should
probably become a principle that GCC should accept feature flags as
soon as it knows about associated system registers (as a sufficient
but not necessary condition).

So we can probably leave this as-is on the basis that it will only
be around for a few weeks.

> +
> +/* Flags associated with the properties of system registers.  It mainly serves
> +   to mark particular registers as read or write only.  */
> +#define F_DEPRECATED		   (1 << 1)
> +#define F_REG_READ		   (1 << 2)
> +#define F_REG_WRITE		   (1 << 3)
> +#define F_ARCHEXT		   (1 << 4)
> +/* Flag indicating register name is alias for another system register.  */
> +#define F_REG_ALIAS		   (1 << 5)
> +

Since the definition of sysreg_t is local to aarch64.cc (good!),
could these flags be defined there rather than in aarch64.h?

Thanks,
Richard

>  /* Crypto is an optional extension to AdvSIMD.  */
>  #define TARGET_CRYPTO (AARCH64_ISA_CRYPTO)
  

On 10/18/23 22:07, Richard Sandiford wrote:
> Victor Do Nascimento <victor.donascimento@arm.com> writes:
>> This patch defines the structure of a new .def file used for
>> representing the aarch64 system registers, what information it should
>> hold and the basic framework in GCC to process this file.
>>
>> Entries in the aarch64-system-regs.def file should be as follows:
>>
>>    SYSREG (NAME, CPENC (sn,op1,cn,cm,op2), FLAG1 | ... | FLAGn, ARCH)
>>
>> Where the arguments to SYSREG correspond to:
>>    - NAME:  The system register name, as used in the assembly language.
>>    - CPENC: The system register encoding, mapping to:
>>
>>      	       s<sn>_<op1>_c<cn>_c<cm>_<op2>
>>
>>    - FLAG: The entries in the FLAGS field are bitwise-OR'd together to
>>      	  encode extra information required to ensure proper use of
>> 	  the system register.  For example, a read-only system
>> 	  register will have the flag F_REG_READ, while write-only
>> 	  registers will be labeled F_REG_WRITE.  Such flags are
>> 	  tested against at compile-time.
>>    - ARCH: The architectural features the system register is associated
>>      	  with.  This is encoded via one of three possible macros:
>> 	  1. When a system register is universally implemented, we say
>> 	  it has no feature requirements, so we tag it with the
>> 	  AARCH64_NO_FEATURES macro.
>> 	  2. When a register is only implemented for a single
>> 	  architectural extension EXT, the AARCH64_FEATURE (EXT), is
>> 	  used.
>> 	  3. When a given system register is made available by any of N
>> 	  possible architectural extensions, the AARCH64_FEATURES(N, ...)
>> 	  macro is used to combine them accordingly.
>>
>> In order to enable proper interpretation of the SYSREG entries by the
>> compiler, flags defining system register behavior such as `F_REG_READ'
>> and `F_REG_WRITE' are also defined here, so they can later be used for
>> the validation of system register properties.
>>
>> Finally, any architectural feature flags from Binutils missing from GCC
>> have appropriate aliases defined here so as to ensure
>> cross-compatibility of SYSREG entries across the toolchain.
>>
>> gcc/ChangeLog:
>>
>> 	* gcc/config/aarch64/aarch64.cc (sysreg_t): New.
>> 	(sysreg_structs): Likewise.
>> 	(nsysreg): Likewise.
>> 	(AARCH64_FEATURE): Likewise.
>> 	(AARCH64_FEATURES): Likewise.
>> 	(AARCH64_NO_FEATURES): Likewise.
>> 	* gcc/config/aarch64/aarch64.h (AARCH64_ISA_V8A): Add missing
>> 	ISA flag.
>> 	(AARCH64_ISA_V8_1A): Likewise.
>> 	(AARCH64_ISA_V8_7A): Likewise.
>> 	(AARCH64_ISA_V8_8A): Likewise.
>> 	(AARCH64_NO_FEATURES): Likewise.
>> 	(AARCH64_FL_RAS): New ISA flag alias.
>> 	(AARCH64_FL_LOR): Likewise.
>> 	(AARCH64_FL_PAN): Likewise.
>> 	(AARCH64_FL_AMU): Likewise.
>> 	(AARCH64_FL_SCXTNUM): Likewise.
>> 	(AARCH64_FL_ID_PFR2): Likewise.
>> 	(F_DEPRECATED): New.
>> 	(F_REG_READ): Likewise.
>> 	(F_REG_WRITE): Likewise.
>> 	(F_ARCHEXT): Likewise.
>> 	(F_REG_ALIAS): Likewise.
>> ---
>>   gcc/config/aarch64/aarch64.cc | 38 +++++++++++++++++++++++++++++++++++
>>   gcc/config/aarch64/aarch64.h  | 36 +++++++++++++++++++++++++++++++++
>>   2 files changed, 74 insertions(+)
>>
>> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
>> index 9fbfc548a89..69de2366424 100644
>> --- a/gcc/config/aarch64/aarch64.cc
>> +++ b/gcc/config/aarch64/aarch64.cc
>> @@ -2807,6 +2807,44 @@ static const struct processor all_cores[] =
>>     {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, NULL}
>>   };
>>   
>> +typedef struct {
>> +  const char* name;
>> +  const char* encoding;
> 
> Formatting nit, but GCC style is:
> 
>    const char *foo
> 
> rather than:
> 
>    const char* foo;
> 
>> +  const unsigned properties;
>> +  const unsigned long long arch_reqs;
> 
> I don't think these two should be const.  There's no reason in principle
> why a sysreg_t can't be created and modified dynamically.
> 
> It would be useful to have some comments above the fields to say what
> they represent.  E.g. the definition on its own doesn't make clear what
> "properties" refers to.
> 
> arch_reqs should use aarch64_feature_flags rather than unsigned long long.
> We're running out of feature flags in GCC too, so aarch64_feature_flags
> is soon likely to be a C++ class.
> 
>> +} sysreg_t;
>> +
>> +/* An aarch64_feature_set initializer for a single feature,
>> +   AARCH64_FEATURE_<FEAT>.  */
>> +#define AARCH64_FEATURE(FEAT) AARCH64_FL_##FEAT
>> +
>> +/* Used by AARCH64_FEATURES.  */
>> +#define AARCH64_OR_FEATURES_1(X, F1) \
>> +  AARCH64_FEATURE (F1)
>> +#define AARCH64_OR_FEATURES_2(X, F1, F2) \
>> +  (AARCH64_FEATURE (F1) | AARCH64_OR_FEATURES_1 (X, F2))
>> +#define AARCH64_OR_FEATURES_3(X, F1, ...) \
>> +  (AARCH64_FEATURE (F1) | AARCH64_OR_FEATURES_2 (X, __VA_ARGS__))
>> +
>> +/* An aarch64_feature_set initializer for the N features listed in "...".  */
>> +#define AARCH64_FEATURES(N, ...) \
>> +  AARCH64_OR_FEATURES_##N (0, __VA_ARGS__)
>> +
>> +/* Database of system registers, their encodings and architectural
>> +   requirements.  */
>> +const sysreg_t sysreg_structs[] =
>> +{
>> +#define CPENC(SN, OP1, CN, CM, OP2) "s"#SN"_"#OP1"_c"#CN"_c"#CM"_"#OP2
>> +#define SYSREG(NAME, ENC, FLAGS, ARCH) \
>> +  { NAME, ENC, FLAGS, ARCH },
>> +#include "aarch64-sys-regs.def"
>> +#undef CPENC
>> +};
>> +
>> +#define TOTAL_ITEMS (sizeof sysreg_structs / sizeof sysreg_structs[0])
>> +const unsigned nsysreg = TOTAL_ITEMS;
> 
> There's an ARRAY_SIZE macro for this.
> 
>> +#undef TOTAL_ITEMS
>> +
>>   /* The current tuning set.  */
>>   struct tune_params aarch64_tune_params = generic_tunings;
>>   
>> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
>> index d74e9116fc5..cf3969a11aa 100644
>> --- a/gcc/config/aarch64/aarch64.h
>> +++ b/gcc/config/aarch64/aarch64.h
>> @@ -179,6 +179,8 @@ enum class aarch64_feature : unsigned char {
>>   
>>   /* Macros to test ISA flags.  */
>>   
>> +#define AARCH64_ISA_V8A		   (aarch64_isa_flags & AARCH64_FL_V8A)
>> +#define AARCH64_ISA_V8_1A	   (aarch64_isa_flags & AARCH64_FL_V8_1A)
>>   #define AARCH64_ISA_CRC            (aarch64_isa_flags & AARCH64_FL_CRC)
>>   #define AARCH64_ISA_CRYPTO         (aarch64_isa_flags & AARCH64_FL_CRYPTO)
>>   #define AARCH64_ISA_FP             (aarch64_isa_flags & AARCH64_FL_FP)
>> @@ -215,6 +217,8 @@ enum class aarch64_feature : unsigned char {
>>   #define AARCH64_ISA_SB		   (aarch64_isa_flags & AARCH64_FL_SB)
>>   #define AARCH64_ISA_V8R		   (aarch64_isa_flags & AARCH64_FL_V8R)
>>   #define AARCH64_ISA_PAUTH	   (aarch64_isa_flags & AARCH64_FL_PAUTH)
>> +#define AARCH64_ISA_V8_7A	   (aarch64_isa_flags & AARCH64_FL_V8_7A)
>> +#define AARCH64_ISA_V8_8A	   (aarch64_isa_flags & AARCH64_FL_V8_8A)
>>   #define AARCH64_ISA_V9A		   (aarch64_isa_flags & AARCH64_FL_V9A)
>>   #define AARCH64_ISA_V9_1A          (aarch64_isa_flags & AARCH64_FL_V9_1A)
>>   #define AARCH64_ISA_V9_2A          (aarch64_isa_flags & AARCH64_FL_V9_2A)
>> @@ -223,6 +227,38 @@ enum class aarch64_feature : unsigned char {
>>   #define AARCH64_ISA_LS64	   (aarch64_isa_flags & AARCH64_FL_LS64)
>>   #define AARCH64_ISA_CSSC	   (aarch64_isa_flags & AARCH64_FL_CSSC)
>>   
>> +/* AARCH64_FL options necessary for system register implementation.  */
>> +
>> +/* Mask for core system registers.  System registers requiring no architectural
>> +   extensions set up a feature-checking mask which returns any passed flags
>> +   unchanged when ANDd with.  */
>> +#define AARCH64_NO_FEATURES	   (uint64_t)-1
> 
> I think this one should only be defined before including the .def file,
> and undefined afterwards.  It's not something that general GCC code
> should use.
> 
> Is -1 right though?  3/7 uses:
> 
>    if (aarch64_isa_flags & sysreg->arch_reqs)
> 
> which requires at least one of the features in sysreg->arch_reqs to be
> available.  But binutils uses AARCH64_CPU_HAS_ALL_FEATURES, which requires
> all of the features to be available.
> 
> At least, it seems odd that AARCH64_NO_FEATURES is all-ones here
> but all-zeros in binutils.

Good point.

Functionally, this worked. I could not see where `aarch64_isa_flags' 
would ever be all zeros, so an all-ones's mask would cause the above 
expression to always evaluate to true and thus allow for any 
`aarch64_isa_flags' configuration to be accepted.

Nonetheless, I very much agree with your point that all-ones is more 
indicative of AARCH64_CPU_HAS_ALL_FEATURES and, as such, having -1 map 
onto AARCH64_NO_FEATURES was misleading insofar as signifying intent and 
felt like bad code.

I've modified things such that we now have AARCH64_NO_FEATURES mapping 
to 0 and, where appropriate, replacing

   if (aarch64_isa_flags & sysreg->arch_reqs)

with something to the effect of:

   if (sysreg->arch_reqs
       && (aarch64_isa_flags & sysreg->arch_reqs)),

thus saying "if the system register has any special architectural 
requirements, then and only then compare those requirement with 
`aarch64_isa_flags'".

> 
>> +
>> +/* Define AARCH64_FL aliases for architectural features which are protected
>> +   by -march flags in binutils but which receive no special treatment by GCC.
>> +
>> +   Such flags are inherited from the Binutils definition of system registers
>> +   and are mapped to the architecture in which the feature is implemented.  */
>> +#define AARCH64_FL_RAS		   AARCH64_FL_V8A
>> +#define AARCH64_FL_LOR		   AARCH64_FL_V8_1A
>> +#define AARCH64_FL_PAN		   AARCH64_FL_V8_1A
>> +#define AARCH64_FL_AMU		   AARCH64_FL_V8_4A
>> +#define AARCH64_FL_SCXTNUM	   AARCH64_FL_V8_5A
>> +#define AARCH64_FL_ID_PFR2	   AARCH64_FL_V8_5A
>> +
>> +/* Define AARCH64_FL aliases for features note yet implemented in GCC.
>> +   Accept them unconditionally.  */
>> +#define AARCH64_FL_SME		   -1
> 
> I think disallowing them would be better.  Otherwise we'd go from
> accepting a register without +sme to not accepting it, which might
> seem like a regression to users.
> 
Naturally, this makes sense.
Agreed.

Thanks,
V.

> Of course, in the particular case of SME, this problem should go
> away very soon :)  And once this sysreg work goes in, it should
> probably become a principle that GCC should accept feature flags as
> soon as it knows about associated system registers (as a sufficient
> but not necessary condition).
> 
> So we can probably leave this as-is on the basis that it will only
> be around for a few weeks.
> 
>> +
>> +/* Flags associated with the properties of system registers.  It mainly serves
>> +   to mark particular registers as read or write only.  */
>> +#define F_DEPRECATED		   (1 << 1)
>> +#define F_REG_READ		   (1 << 2)
>> +#define F_REG_WRITE		   (1 << 3)
>> +#define F_ARCHEXT		   (1 << 4)
>> +/* Flag indicating register name is alias for another system register.  */
>> +#define F_REG_ALIAS		   (1 << 5)
>> +
> 
> Since the definition of sysreg_t is local to aarch64.cc (good!),
> could these flags be defined there rather than in aarch64.h?
> 
> Thanks,
> Richard
> 
>>   /* Crypto is an optional extension to AdvSIMD.  */
>>   #define TARGET_CRYPTO (AARCH64_ISA_CRYPTO)
  

Thanks for the updates.

Victor Do Nascimento <Victor.DoNascimento@arm.com> writes:
> On 10/18/23 22:07, Richard Sandiford wrote:
>> Victor Do Nascimento <victor.donascimento@arm.com> writes:
>>> This patch defines the structure of a new .def file used for
>>> representing the aarch64 system registers, what information it should
>>> hold and the basic framework in GCC to process this file.
>>>
>>> Entries in the aarch64-system-regs.def file should be as follows:
>>>
>>>    SYSREG (NAME, CPENC (sn,op1,cn,cm,op2), FLAG1 | ... | FLAGn, ARCH)
>>>
>>> Where the arguments to SYSREG correspond to:
>>>    - NAME:  The system register name, as used in the assembly language.
>>>    - CPENC: The system register encoding, mapping to:
>>>
>>>      	       s<sn>_<op1>_c<cn>_c<cm>_<op2>
>>>
>>>    - FLAG: The entries in the FLAGS field are bitwise-OR'd together to
>>>      	  encode extra information required to ensure proper use of
>>> 	  the system register.  For example, a read-only system
>>> 	  register will have the flag F_REG_READ, while write-only
>>> 	  registers will be labeled F_REG_WRITE.  Such flags are
>>> 	  tested against at compile-time.
>>>    - ARCH: The architectural features the system register is associated
>>>      	  with.  This is encoded via one of three possible macros:
>>> 	  1. When a system register is universally implemented, we say
>>> 	  it has no feature requirements, so we tag it with the
>>> 	  AARCH64_NO_FEATURES macro.
>>> 	  2. When a register is only implemented for a single
>>> 	  architectural extension EXT, the AARCH64_FEATURE (EXT), is
>>> 	  used.
>>> 	  3. When a given system register is made available by any of N
>>> 	  possible architectural extensions, the AARCH64_FEATURES(N, ...)
>>> 	  macro is used to combine them accordingly.
>>>
>>> In order to enable proper interpretation of the SYSREG entries by the
>>> compiler, flags defining system register behavior such as `F_REG_READ'
>>> and `F_REG_WRITE' are also defined here, so they can later be used for
>>> the validation of system register properties.
>>>
>>> Finally, any architectural feature flags from Binutils missing from GCC
>>> have appropriate aliases defined here so as to ensure
>>> cross-compatibility of SYSREG entries across the toolchain.
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* gcc/config/aarch64/aarch64.cc (sysreg_t): New.
>>> 	(sysreg_structs): Likewise.
>>> 	(nsysreg): Likewise.
>>> 	(AARCH64_FEATURE): Likewise.
>>> 	(AARCH64_FEATURES): Likewise.
>>> 	(AARCH64_NO_FEATURES): Likewise.
>>> 	* gcc/config/aarch64/aarch64.h (AARCH64_ISA_V8A): Add missing
>>> 	ISA flag.
>>> 	(AARCH64_ISA_V8_1A): Likewise.
>>> 	(AARCH64_ISA_V8_7A): Likewise.
>>> 	(AARCH64_ISA_V8_8A): Likewise.
>>> 	(AARCH64_NO_FEATURES): Likewise.
>>> 	(AARCH64_FL_RAS): New ISA flag alias.
>>> 	(AARCH64_FL_LOR): Likewise.
>>> 	(AARCH64_FL_PAN): Likewise.
>>> 	(AARCH64_FL_AMU): Likewise.
>>> 	(AARCH64_FL_SCXTNUM): Likewise.
>>> 	(AARCH64_FL_ID_PFR2): Likewise.
>>> 	(F_DEPRECATED): New.
>>> 	(F_REG_READ): Likewise.
>>> 	(F_REG_WRITE): Likewise.
>>> 	(F_ARCHEXT): Likewise.
>>> 	(F_REG_ALIAS): Likewise.
>>> ---
>>>   gcc/config/aarch64/aarch64.cc | 38 +++++++++++++++++++++++++++++++++++
>>>   gcc/config/aarch64/aarch64.h  | 36 +++++++++++++++++++++++++++++++++
>>>   2 files changed, 74 insertions(+)
>>>
>>> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
>>> index 9fbfc548a89..69de2366424 100644
>>> --- a/gcc/config/aarch64/aarch64.cc
>>> +++ b/gcc/config/aarch64/aarch64.cc
>>> @@ -2807,6 +2807,44 @@ static const struct processor all_cores[] =
>>>     {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, NULL}
>>>   };
>>>   
>>> +typedef struct {
>>> +  const char* name;
>>> +  const char* encoding;
>> 
>> Formatting nit, but GCC style is:
>> 
>>    const char *foo
>> 
>> rather than:
>> 
>>    const char* foo;
>> 
>>> +  const unsigned properties;
>>> +  const unsigned long long arch_reqs;
>> 
>> I don't think these two should be const.  There's no reason in principle
>> why a sysreg_t can't be created and modified dynamically.
>> 
>> It would be useful to have some comments above the fields to say what
>> they represent.  E.g. the definition on its own doesn't make clear what
>> "properties" refers to.
>> 
>> arch_reqs should use aarch64_feature_flags rather than unsigned long long.
>> We're running out of feature flags in GCC too, so aarch64_feature_flags
>> is soon likely to be a C++ class.
>> 
>>> +} sysreg_t;
>>> +
>>> +/* An aarch64_feature_set initializer for a single feature,
>>> +   AARCH64_FEATURE_<FEAT>.  */
>>> +#define AARCH64_FEATURE(FEAT) AARCH64_FL_##FEAT
>>> +
>>> +/* Used by AARCH64_FEATURES.  */
>>> +#define AARCH64_OR_FEATURES_1(X, F1) \
>>> +  AARCH64_FEATURE (F1)
>>> +#define AARCH64_OR_FEATURES_2(X, F1, F2) \
>>> +  (AARCH64_FEATURE (F1) | AARCH64_OR_FEATURES_1 (X, F2))
>>> +#define AARCH64_OR_FEATURES_3(X, F1, ...) \
>>> +  (AARCH64_FEATURE (F1) | AARCH64_OR_FEATURES_2 (X, __VA_ARGS__))
>>> +
>>> +/* An aarch64_feature_set initializer for the N features listed in "...".  */
>>> +#define AARCH64_FEATURES(N, ...) \
>>> +  AARCH64_OR_FEATURES_##N (0, __VA_ARGS__)
>>> +
>>> +/* Database of system registers, their encodings and architectural
>>> +   requirements.  */
>>> +const sysreg_t sysreg_structs[] =
>>> +{
>>> +#define CPENC(SN, OP1, CN, CM, OP2) "s"#SN"_"#OP1"_c"#CN"_c"#CM"_"#OP2
>>> +#define SYSREG(NAME, ENC, FLAGS, ARCH) \
>>> +  { NAME, ENC, FLAGS, ARCH },
>>> +#include "aarch64-sys-regs.def"
>>> +#undef CPENC
>>> +};
>>> +
>>> +#define TOTAL_ITEMS (sizeof sysreg_structs / sizeof sysreg_structs[0])
>>> +const unsigned nsysreg = TOTAL_ITEMS;
>> 
>> There's an ARRAY_SIZE macro for this.
>> 
>>> +#undef TOTAL_ITEMS
>>> +
>>>   /* The current tuning set.  */
>>>   struct tune_params aarch64_tune_params = generic_tunings;
>>>   
>>> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
>>> index d74e9116fc5..cf3969a11aa 100644
>>> --- a/gcc/config/aarch64/aarch64.h
>>> +++ b/gcc/config/aarch64/aarch64.h
>>> @@ -179,6 +179,8 @@ enum class aarch64_feature : unsigned char {
>>>   
>>>   /* Macros to test ISA flags.  */
>>>   
>>> +#define AARCH64_ISA_V8A		   (aarch64_isa_flags & AARCH64_FL_V8A)
>>> +#define AARCH64_ISA_V8_1A	   (aarch64_isa_flags & AARCH64_FL_V8_1A)
>>>   #define AARCH64_ISA_CRC            (aarch64_isa_flags & AARCH64_FL_CRC)
>>>   #define AARCH64_ISA_CRYPTO         (aarch64_isa_flags & AARCH64_FL_CRYPTO)
>>>   #define AARCH64_ISA_FP             (aarch64_isa_flags & AARCH64_FL_FP)
>>> @@ -215,6 +217,8 @@ enum class aarch64_feature : unsigned char {
>>>   #define AARCH64_ISA_SB		   (aarch64_isa_flags & AARCH64_FL_SB)
>>>   #define AARCH64_ISA_V8R		   (aarch64_isa_flags & AARCH64_FL_V8R)
>>>   #define AARCH64_ISA_PAUTH	   (aarch64_isa_flags & AARCH64_FL_PAUTH)
>>> +#define AARCH64_ISA_V8_7A	   (aarch64_isa_flags & AARCH64_FL_V8_7A)
>>> +#define AARCH64_ISA_V8_8A	   (aarch64_isa_flags & AARCH64_FL_V8_8A)
>>>   #define AARCH64_ISA_V9A		   (aarch64_isa_flags & AARCH64_FL_V9A)
>>>   #define AARCH64_ISA_V9_1A          (aarch64_isa_flags & AARCH64_FL_V9_1A)
>>>   #define AARCH64_ISA_V9_2A          (aarch64_isa_flags & AARCH64_FL_V9_2A)
>>> @@ -223,6 +227,38 @@ enum class aarch64_feature : unsigned char {
>>>   #define AARCH64_ISA_LS64	   (aarch64_isa_flags & AARCH64_FL_LS64)
>>>   #define AARCH64_ISA_CSSC	   (aarch64_isa_flags & AARCH64_FL_CSSC)
>>>   
>>> +/* AARCH64_FL options necessary for system register implementation.  */
>>> +
>>> +/* Mask for core system registers.  System registers requiring no architectural
>>> +   extensions set up a feature-checking mask which returns any passed flags
>>> +   unchanged when ANDd with.  */
>>> +#define AARCH64_NO_FEATURES	   (uint64_t)-1
>> 
>> I think this one should only be defined before including the .def file,
>> and undefined afterwards.  It's not something that general GCC code
>> should use.
>> 
>> Is -1 right though?  3/7 uses:
>> 
>>    if (aarch64_isa_flags & sysreg->arch_reqs)
>> 
>> which requires at least one of the features in sysreg->arch_reqs to be
>> available.  But binutils uses AARCH64_CPU_HAS_ALL_FEATURES, which requires
>> all of the features to be available.
>> 
>> At least, it seems odd that AARCH64_NO_FEATURES is all-ones here
>> but all-zeros in binutils.
>
> Good point.
>
> Functionally, this worked. I could not see where `aarch64_isa_flags' 
> would ever be all zeros, so an all-ones's mask would cause the above 
> expression to always evaluate to true and thus allow for any 
> `aarch64_isa_flags' configuration to be accepted.
>
> Nonetheless, I very much agree with your point that all-ones is more 
> indicative of AARCH64_CPU_HAS_ALL_FEATURES and, as such, having -1 map 
> onto AARCH64_NO_FEATURES was misleading insofar as signifying intent and 
> felt like bad code.
>
> I've modified things such that we now have AARCH64_NO_FEATURES mapping 
> to 0 and, where appropriate, replacing
>
>    if (aarch64_isa_flags & sysreg->arch_reqs)
>
> with something to the effect of:
>
>    if (sysreg->arch_reqs
>        && (aarch64_isa_flags & sysreg->arch_reqs)),
>
> thus saying "if the system register has any special architectural 
> requirements, then and only then compare those requirement with 
> `aarch64_isa_flags'".

I think it should instead be:

  if ((aarch64_isa_flags & sysreg->arch_reqs) == sysreg->arch_reqs)

or (equivalently)

  if ((~aarch64_isa_flags & sysreg->arch_reqs) == 0)

which requires all of the flags in arch_reqs to be present.
That matches what AARCH64_CPU_HAS_ALL_FEATURES does, and is always
true when sysreg->arch_reqs is zero.

Thanks,
Richard