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

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

Looks like what I'm asking for is essentially to revert this change
(not literally revert it as stuff changed since, but do the converse
of what it changed). Gael, can you explain what was the motivation for
it? Do you agree with my argument in previous email?

Benoit

# HG changeset patch
# User Gael Guennebaud <g.gael@xxxxxxx>
# Date 1282116955 -7200
# Node ID 6eb14e3807ce8341356a5301de6ab0ed7ad32789
# Parent  d49bff4b2b01309617c023dc40c87de49c4a400a
* disable unalignment detection when vectorization is not enabled
* revert MapBase unalignment detection

diff --git a/Eigen/src/Core/MapBase.h b/Eigen/src/Core/MapBase.h
--- a/Eigen/src/Core/MapBase.h
+++ b/Eigen/src/Core/MapBase.h
@@ -184,17 +184,17 @@ template<typename Derived> class MapBase

   protected:

     void checkSanity() const
     {
       EIGEN_STATIC_ASSERT(EIGEN_IMPLIES(ei_traits<Derived>::Flags&PacketAccessBit,

ei_inner_stride_at_compile_time<Derived>::ret==1),

PACKET_ACCESS_REQUIRES_TO_HAVE_INNER_STRIDE_FIXED_TO_1);
-      ei_assert(EIGEN_IMPLIES(ei_traits<Derived>::Flags&AlignedBit,
(size_t(m_data) % 16 == 0))
+      ei_assert(EIGEN_IMPLIES(ei_traits<Derived>::Flags&AlignedBit,
(size_t(m_data) % (sizeof(Scalar)*ei_packet_traits<Scalar
>::size)) == 0)
         && "data is not aligned");
     }


2011/11/27 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
>