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