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.
Many thanks,
V.
+ {
+ 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
