On Fri, May 17, 2024 at 04:45:05PM +0100, Richard Sandiford wrote: > Andrew Carlotti <andrew.carlo...@arm.com> writes: > > The end goal of the series is to change the definition of > > aarch64_feature_flags > > from a uint64_t typedef to a class with 128 bits of storage. This class > > uses > > operator overloading to mimic the existing integer interface as much as > > possible, but with added restrictions to facilate type checking and > > extensibility. > > > > Patches 01-10 are preliminary enablement work, and have passed regression > > testing. Are these ok for master? > > > > Patch 11 is an RFC, and the only patch that touches the middle end. I am > > seeking clarity on which part(s) of the compiler should be expected to > > handle > > or prevent non-bool types in instruction pattern conditions. The actual > > patch > > does not compile by itself (though it does in combination with 12/12), but > > that > > is not important to the questions I'm asking. > > > > Patch 12 is then a small patch that actually replaces the uint64_t typedef > > with > > a class. I think this patch is fine in it's current form, but it depends > > on a > > resolution to the issues in patch 11/12 first. > > Thanks for doing this. > > Rather than disallowing flags == 0, etc., I think we should allow > aarch64_feature_flags to be constructed from a single uint64_t. > It's a lossless conversion. The important thing is that we don't > allow conversions the other way (and the patch doesn't allow them).
I agree that allowing conversion from a single int should be safe (albeit it was probably helpful to disallow it during the development of this series). It does feel a little bit strange to have a separate mechanism for setting the first 64 bits (and zeroing the rest). Do you consider the existing code in some places to be clearer than the new versions in this patch series? If so, it would be helpful to know which patches (or parts of patches) I should drop. > Also, I think we should make the new class in 12/12 be a templated > <size_t N> type that provides an N-bit bitmask. It should arguably > also be target-independent code. aarch64_feature_flags would then be > an alias with the appropriate number of bits. I think the difficult part is to do this for generic N while still satisfying C++11 constexpr function requirements (we can't use a loop, for example). However, while writing this response, I've realised that I can do this using recursion, with an N-bit bitmask being implemented as a class containing an M-bit integer and (recursively) and (N-M)-bit bitmask. > For the RFC in 11/12, how about, as another prepatch before 12/12, > removing all the mechanical: > > #define AARCH64_ISA_LS64 (aarch64_isa_flags & AARCH64_FL_LS64) > > style macros and replacing uses with something like: > > AARCH64_HAVE_ISA (LS64) This sounds like a good approach, and is roughly what I was already planning to do (although I hadn't worked out the details yet). I think that can entirely replace 11/12 in the context of this series, but the questions about instruction pattern condition type checking still ought to be addressed separately. > Uses outside aarch64.h should arguably be changed to TARGET_* instead, > since the convention seems to be that TARGET_* checks the underlying > ISA flag and also any other relevant conditions (where applicable). > > Thanks, > Richard
