Re: [PATCH 00/12] aarch64: Extend aarch64_feature_flags to 128 bits

[Andrew Carlotti <andrew.carlotti@arm.com>]

On Fri, May 17, 2024 at 04:45:05PM +0100, Richard Sandiford wrote:
> Andrew Carlotti <andrew.carlotti@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