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

[Richard Sandiford <richard.sandiford@arm.com>]

Andrew Carlotti <andrew.carlotti@arm.com> writes:
> 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).

With a templated class, I think it makes sense.  The constructor would
take a variable number of arguments and any unspecified elements would
implicitly be zero.  In that sense, a single uint64_t isn't a special
case.  It's just an instance of a generic rule.

> 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.

Probably patches 3, 4, and (for unrelated reasons) 9.  (9 feels like
a microoptimisation, given that the underlying issue has been fixed.)

>> 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.

I think it'd be better to keep a flat object, not least for debugging.

Things like operator| could be handled using code like:

----------------------------------------------------------------
template<int N>
struct operators
{
  template<typename Result, typename Operator, typename Arg, typename ...Rest>
  static constexpr Result binary(Operator op, const Arg &x, const Arg &y,
				 Rest ...rest)
  {
    return operators<N - 1>::template binary<Result, Operator, Arg>
      (op, x, y, op (x[N - 1], y[N - 1]), rest...);
  }
};

template<>
struct operators<0>
{
  template<typename Result, typename Operator, typename Arg, typename ...Rest>
  static constexpr Result binary(Operator op, const Arg &x, const Arg &y,
				 Rest ...rest)
  {
    return Result { rest... };
  }
};

using T = std::array<int, 2>;

template<typename T>
constexpr T f(T x, T y) { return x | y; }
constexpr T x = { 1, 2 };
constexpr T y = { 0x100, 0x400 };
constexpr T z = operators<2>::binary<T> (f<int>, x, y);
----------------------------------------------------------------

(Unfortunately, constexpr lambdas are also not supported in C++11.)

>> 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.

Yeah, stronger typing would be good.  I think in practice the generators
should add the "bool (...)" wrapper.

Thanks,
Richard