Victor Do Nascimento <victor.donascime...@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)
