Re: [eigen] Re: 3.0.4 coming soon, testing appreciated

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2011/11/26 Jitse Niesen <jitse@xxxxxxxxxxxxxxxxx>:
> On Sat, 26 Nov 2011, Benoit Jacob wrote:
>> Clearly this assertion is wrong. It predates my changes (actually my
>> changes were fixing the same mistake elsewhere). We never align to
>> more than 16 bytes, so requiring higher alignment than that is wrong.
>> The fix is to replace this by the same check that is performed in
>> compute_matrix_flags.
>
> Hmm, isn't the purpose of the assert to check that the pointer m_data is
> indeed aligned if it is claimed to be?

Yes, but the problem is in the definition of "aligned". In Eigen,
Aligned currently means 16-byte aligned, because the only thing that
we need alignment for, is vectorization with SSE/NEON/AltiVec. Of
course this isn't future proof and we need to refactor that for AVX,
but that's a separate problem. For example, we don't even have a
32-byte aligned malloc function at the moment.

Another very important thing to keep in mind is that whether we align
or not, and whether a given pointer is consider to be aligned, does
NOT depend on vectorization. This may seem to contradict what I just
said above. Explanation: while alignment is only used for
vectorization, we don't want to let enabling/disabling vectorization
impact at all our definition of "aligned", as that would mean that
enabling/disabling vectorization changes the ABI. Which would prevent
people from linking together vectorized and non-vectorized code.

>
> I'm quite happy to fix it, but I'm not sure what kind of alignment we
> expect. Is it as follows?
>
>  If AlignedBit is set, then the address should be a multiple of
>  the packet size (in bytes), unless a packet is bigger than 16 bytes,
>  in which case the address should be a multiple of 16.
>
> Or, to translate it to C++, the assertion in MapBase should be
>
>   eigen_assert(EIGEN_IMPLIES(internal::traits<Derived>::Flags&AlignedBit,
>     (size_t(m_data)
>        % (std::min(sizeof(Scalar)*internal::packet_traits<Scalar>::size,
>                    16))) == 0)

First of all, this still would fail for a non-vectorized Scalar type,
as we have in that case packet_traits<Scalar>::size == 1. So if the
user uses a Scalar type of size 12 (say something like Array3f) you'd
get weird results. You'd have to check
packet_traits<Scalar>::Vectorizable.

But then again, there is a much more fundamental problem:
packet_traits is an implementation detail, it can change at any time,
e.g. if we start vectorizing something that we didn't use to, while
alignment is a basic part of the ABI that should never change. So
really, we want to have the strictest notion of alignment, which is
the one currently implemented in compute_matrix_flags, which, in the
fixed-size case, is:

    ((MaxCols*MaxRows*sizeof(Scalar)) % 16) == 0

that is, we require (16-byte) alignment whenever the size is a
multiple of 16 bytes, which is the most that we can do without
increasing sizeof().

> It probably still makes sense to put this in a template helper.

Yes, but again, it's the solution currently implemented in
compute_matrix_flags that should be used.

Cheers,
Benoit

>
>
> Jitse