Victor Do Nascimento <victor.donascime...@arm.com> writes: > On 10/18/23 22:30, Richard Sandiford wrote: >> Victor Do Nascimento <victor.donascime...@arm.com> writes: >>> Add a build-time test to check whether system register data, as >>> imported from `aarch64-sys-reg.def' has any duplicate entries. >>> >>> Duplicate entries are defined as any two SYSREG entries in the .def >>> file which share the same encoding values (as specified by its `CPENC' >>> field) and where the relationship amongst the two does not fit into >>> one of the following categories: >>> >>> * Simple aliasing: In some cases, it is observed that one >>> register name serves as an alias to another. One example of >>> this is where TRCEXTINSELR aliases TRCEXTINSELR0. >>> * Expressing intent: It is possible that when a given register >>> serves two distinct functions depending on how it is used, it >>> is given two distinct names whose use should match the context >>> under which it is being used. Example: Debug Data Transfer >>> Register. When used to receive data, it should be accessed as >>> DBGDTRRX_EL0 while when transmitting data it should be >>> accessed via DBGDTRTX_EL0. >>> * Register depreciation: Some register names have been >>> deprecated and should no longer be used, but backwards- >>> compatibility requires that such names continue to be >>> recognized, as is the case for the SPSR_EL1 register, whose >>> access via the SPSR_SVC name is now deprecated. >>> * Same encoding different target: Some encodings are given >>> different meaning depending on the target architecture and, as >>> such, are given different names in each of theses contexts. >>> We see an example of this for CPENC(3,4,2,0,0), which >>> corresponds to TTBR0_EL2 for Armv8-A targets and VSCTLR_EL2 >>> in Armv8-R targets. >>> >>> A consequence of these observations is that `CPENC' duplication is >>> acceptable iff at least one of the `properties' or `arch_reqs' fields >>> of the `sysreg_t' structs associated with the two registers in >>> question differ and it's this condition that is checked by the new >>> `aarch64_test_sysreg_encoding_clashes' function. >>> >>> gcc/ChangeLog: >>> >>> * gcc/config/aarch64/aarch64.cc >>> (aarch64_test_sysreg_encoding_clashes): New. >>> (aarch64_run_selftests): add call to >>> aarch64_test_sysreg_encoding_clashes selftest. >>> --- >>> gcc/config/aarch64/aarch64.cc | 53 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 53 insertions(+) >>> >>> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc >>> index d187e171beb..e0be2877ede 100644 >>> --- a/gcc/config/aarch64/aarch64.cc >>> +++ b/gcc/config/aarch64/aarch64.cc >>> @@ -22,6 +22,7 @@ >>> >>> #define INCLUDE_STRING >>> #define INCLUDE_ALGORITHM >>> +#define INCLUDE_VECTOR >>> #include "config.h" >>> #include "system.h" >>> #include "coretypes.h" >>> @@ -28332,6 +28333,57 @@ aarch64_test_fractional_cost () >>> ASSERT_EQ (cf (1, 2).as_double (), 0.5); >>> } >>> >>> +/* Calculate whether our system register data, as imported from >>> + `aarch64-sys-reg.def' has any duplicate entries. */ >>> +static void >>> +aarch64_test_sysreg_encoding_clashes (void) >>> +{ >>> + using dup_counters_t = hash_map<nofree_string_hash, unsigned>; >>> + using dup_instances_t = hash_map<nofree_string_hash, >>> + std::vector<const sysreg_t*>>; >>> + >>> + dup_counters_t duplicate_counts; >>> + dup_instances_t duplicate_instances; >>> + >>> + /* Every time an encoding is established to come up more than once >>> + we add it to a "clash-analysis queue", which is then used to extract >>> + necessary information from our hash map when establishing whether >>> + repeated encodings are valid. */ >> >> Formatting nit, sorry, but second and subsequent lines should be >> indented to line up with the "E". >> >>> + >>> + /* 1) Collect recurrence information. */ >>> + std::vector<const char*> testqueue; >>> + >>> + for (unsigned i = 0; i < nsysreg; i++) >>> + { >>> + const sysreg_t *reg = sysreg_structs + i; >>> + >>> + unsigned *tbl_entry = &duplicate_counts.get_or_insert >>> (reg->encoding); >>> + *tbl_entry += 1; >>> + >>> + std::vector<const sysreg_t*> *tmp >>> + = &duplicate_instances.get_or_insert (reg->encoding); >>> + >>> + tmp->push_back (reg); >>> + if (*tbl_entry > 1) >>> + testqueue.push_back (reg->encoding); >>> + } >> >> Do we need two hash maps here? It looks like the length of the vector >> is always equal to the count. Also... >> > > You're right. Addressed in next iteration of patch series. > >>> + >>> + /* 2) Carry out analysis on collected data. */ >>> + for (auto enc : testqueue) >> >> ...hash_map itself is iterable. We could iterate over that instead, >> which would avoid the need for the queue. >> > > My rationale here is that I prefer to take up the extra little bit of > memory to save on execution time. > > `duplicate_instances' is an iterable of vectors, with one such vector > for each encountered encoding value, irrespective of whether or not that > encoding is duplicated. Thus to iterate over this, we'd have to 1. > iterate through every possible vector and 2. check each one's length. > By having our `testqueue', we know immediately which encodings have > duplicate sysreg entries and thus we can jump immediately to analyzing > those and only those.
Yeah, but creating testqueue has its own overhead, with its own memory allocations (which aren't free). This is also run-once self-test code, iterating over a fairly small, fixed-size, static array. So I'm not sure the extra code is really paying for itself in practice. I won't push it though, since I agree the code is correct. Thanks, Richard