Thanks for the updates.
Victor Do Nascimento <victor.donascime...@arm.com> writes:
> On 10/18/23 22:07, Richard Sandiford wrote:
>> 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.
>
> 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
