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

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

ok, this is fixed now.

gael

On Mon, Nov 28, 2011 at 1:32 PM, Gael Guennebaud
<gael.guennebaud@xxxxxxxxx> wrote:
> 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
>>>
>>
>