Fix alignment of test structs

[hahnjo]

Clang complains for subscript.cpp:

error: requested alignment is less than minimum alignment of 8 for type 'S<float>'

The reason is that the struct also contains a double that requires
alignment of at least 8 bytes on x86_64 and the program is thus
ill-formed. Remedy by taking alignof(double) into account.

Other approaches explored:

1. Adding multiple alignas, the compiler should use the "strictest
   (largest) non-zero expression". However GCC (including the latest
   version 10.2.0) always applies the last attribute and older versions
   of Clang (tested: 5.0.2 as in Xcode 9.4.1) also error out.
2. Using std::max to get the maximum. GCC complains that the result
   is not an integer constant.

[mattkretz]

If you ever put an __int128 or long double in there it'll break again. Also the test might now hide actual bugs for cases where the alignment was less than 8 before. How about:

template <class T>
struct alignas(std::is_arithmetic<T>::value ? sizeof(T) : alignof(T)) AlignedBase {};

template <class T>
struct S : AlignedBase<T>
{
  // ...
};

[hahnjo]

True, but this is test code and it already makes assumptions about the required alignment. AFAICT plugging in a struct holding a double (with alignment of 4 bytes) wouldn't work either, right? I mean, the code has to handle the cases it is instantiated for...

Inheriting from AlignedBase should work, or aligning any member (those of type T lend themselves naturally). As does my proposal to have a struct DefaultAlignment that only overrides the case for double. I don't have a strong opinion which route to go.

[agheata]

I guess the test won't work anyway if arbitrary types are plugged in, since it assigns index values to the T members. So setting the alignment to be correct in a generic way is maybe not the primary goal for this test. Since the problem is understood, in the interest of moving forward I would agree with Jonas's fix even as temporary solution, unless you really prefer Matthias the AlignedBase fallback for non-arithmetic types.

[mattkretz]

I'll elaborate:

• For the T = double case, Jonas' and my solution have the same effect.
• For e.g. T = short in Jonas` solution all three structs now have an alignment of 8. With my solution they have an alignment of 2 in gather and scatter, and 8 (-m64) or 4 (-m32) in subscript.

Forcing a higher alignment effectively reduces test coverage, which is why I don't like the proposed PR as is.

Jonas' proposal to use a DefaultAlignment struct does not reduce test coverage, but it reduces clarity about the requirement on the struct. The requirement for structured gathers & scatters is that the alignment of the struct is a multiple of the sizeof of the gathered datatype. That's a simple alignas(sizeof(T)) to say that. We could put it on the first data member or on the struct. Putting it on the first data member works around the limitation of C++ that struct alignas(2) S { int x; }; is ill-formed. But at the cost of being a workaround and potentially unclear for a reader of the code. Placing it on the struct thus would be better but requires either a baseclass workaround or something like e.g. alignas(max(sizeof(T), alignof(double))) for subscript.cpp.

since it assigns index values to the T members.

I don't understand what you're trying to say here. FWIW, the structured gather & scatter take a base pointer, data member pointer, and index vector. The implementation uses the member pointer to offset the base pointer and scales the index vector so that the indexes are not relative to the struct array anymore but to individual elements of type T. That's why the scale factor must be integral and the alignment of the structs must be be a multiple of sizeof(T).

[hahnjo]

or something like e.g. alignas(max(sizeof(T), alignof(double))) for subscript.cpp.

Instead of sizeof(T), I think it should again say std::is_arithmetic<T>::value ? sizeof(T) : alignof(T). It occurred to me that there's a more concise way of writing this by specifying multiple alignas. According to the standard, the compiler should take the "strictest (largest) non-zero expression" and this works fine for Clang. GCC always honors the last specification, there's an open bug report since 2014: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64236

Edit: Nope, doesn't work with Apple's clang...

[mattkretz]

Your last solution looks good. It's still unfortunate that a type from the data members must be repeated. But this is test code...