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
