Re: [PATCH V3 2/6] aarch64: Add support for aarch64-sys-regs.def

[Richard Sandiford <richard.sandiford@arm.com>]

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:
>
> 	* config/aarch64/aarch64.cc (sysreg_t): New.
> 	(sysreg_structs): Likewise.
> 	(nsysreg): Likewise.
> 	(AARCH64_FEATURE): Likewise.
> 	(AARCH64_FEATURES): Likewise.
> 	(AARCH64_NO_FEATURES): Likewise.
> 	* 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 | 53 +++++++++++++++++++++++++++++++++++
>  gcc/config/aarch64/aarch64.h  | 22 +++++++++++++++
>  2 files changed, 75 insertions(+)
>
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 5fd7063663c..a4a9e2e51ea 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -2806,6 +2806,59 @@ static const struct processor all_cores[] =
>     feature_deps::V8A ().enable, &generic_tunings},
>    {NULL, aarch64_none, aarch64_none, aarch64_no_arch, 0, NULL}
>  };
> +/* Internal representation of system registers.  */
> +typedef struct {
> +  const char *name;
> +  /* Stringified sysreg encoding values, represented as
> +     s<sn>_<op1>_c<cn>_c<cm>_<op2>.  */
> +  const char *encoding;
> +  /* Flags affecting sysreg usage, such as read/write-only.  */
> +  unsigned properties;
> +  /* Architectural features implied by sysreg.  */
> +  aarch64_feature_flags arch_reqs;
> +} 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__)
> +
> +#define AARCH64_NO_FEATURES	   0
> +
> +/* 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)
> +
> +/* Database of system registers, their encodings and architectural
> +   requirements.  */
> +const sysreg_t sysreg_structs[] =

How about "aarch64_sysregs" instead, to keep the CPU prefix?

> +{
> +#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
> +};
> +
> +#undef AARCH64_NO_FEATURES
> +
> +const unsigned nsysreg = ARRAY_SIZE (sysreg_structs);

I think it's clearer to drop this and instead use ARRAY_SIZE directly.

OK with those changes, thanks -- no need to repost unless you want to.

Richard

>  
>  /* 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 2f0777a37ac..84e6f79ca83 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,24 @@ 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.  */
> +
> +/* 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		   0
> +
>  /* Crypto is an optional extension to AdvSIMD.  */
>  #define TARGET_CRYPTO (AARCH64_ISA_CRYPTO)