Victor Do Nascimento <[email protected]> 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...
> +
> + /* 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.
> + {
> + unsigned nrep = *duplicate_counts.get (enc);
> + for (unsigned i = 0; i < nrep; i++)
> + for (unsigned j = i+1; j < nrep; j++)
Formatting nit, but "i + 1" rather than "i+1".
Overall, it looks like really nice work. Thanks for doing this.
Richard
> + {
> + std::vector<const sysreg_t*> *tmp2 = duplicate_instances.get (enc);
> + const sysreg_t *a = (*tmp2)[i];
> + const sysreg_t *b = (*tmp2)[j];
> + ASSERT_TRUE ((a->properties != b->properties)
> + || (a->arch_reqs != b->arch_reqs));
> + }
> + }
> +}
> +
> /* Run all target-specific selftests. */
>
> static void
> @@ -28339,6 +28391,7 @@ aarch64_run_selftests (void)
> {
> aarch64_test_loading_full_dump ();
> aarch64_test_fractional_cost ();
> + aarch64_test_sysreg_encoding_clashes ();
> }
>
> } // namespace selftest
