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

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

I only remember this change fixed some issues, probably related to
Block. Now, I agree with what you said and this change looks stupid
but reverting it is not that trivial because when you do so, then the
following tests fail

         77 - corners_3 (Failed)
         80 - product_small_1 (Failed)
        148 - array_2 (Failed)
        154 - array_for_matrix_2 (Failed)
        177 - triangular_2 (Failed)
        303 - upperbidiagonalization_5 (Failed)
        357 - geo_orthomethods_2 (Failed)
        360 - geo_orthomethods_5 (Failed)
        361 - geo_orthomethods_6 (Failed)
        362 - geo_homogeneous_1 (Failed)
        363 - geo_homogeneous_2 (Failed)
        364 - geo_homogeneous_3 (Failed)
        365 - geo_quaternion_1 (Failed)
        366 - geo_quaternion_2 (Failed)
        367 - geo_quaternion_3 (Failed)
        368 - geo_quaternion_4 (Failed)
        372 - geo_transformations_2 (Failed)
        434 - umeyama_3 (Failed)

on this assert:

Eigen/src/Core/MapBase.h:176: void Eigen::MapBase<Derived,
0>::checkSanity() const [with Derived = Eigen::Block<const
Eigen::Transpose<const Eigen::Matrix<float, 2, 2> >, 1, 2, true,
true>]: Assertion `(!(internal::traits<Derived>::Flags&AlignedBit) ||
((size_t(m_data) % 16) == 0)) && "data is not aligned"' failed.

so yes, the real issue seems to be in Block.


gael

On Sun, Nov 27, 2011 at 9:49 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
> 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
>>
>