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

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

2011/11/26 Jitse Niesen <jitse@xxxxxxxxxxxxxxxxx>:
> Benoit Jacob <jacob.benoit.1@...> writes:
>
>> This is now fixed in both branches: f7e4c54e8c44 in default branch,
>> dfea1f0cbf7a in 3.0 branch.
>>
>> Can you please retry?
>>
>> The fix is very nontrivial so I would like to discuss it here.
>>
>> We used to have 2 different ways of computing whether we statically
>> align types: one for the actual static alignment stuff in
>> DenseStorage.h, checking if the byte size is a multiple of 16 bytes,
>> and one for the AlignedBit flag computation in compute_matrix_flags in
>> XprHelper.h, checking if the Scalar type is vectorizable and if
>> Rows*Cols is a multiple of the vectorization packet size.
>>
>> For example, when vectorization is disabled, Matrix4d is still
>> statically aligned (we need that to preserve the ABI) but its Flags do
>> not have the AlignedBit.
>
> It looks like this introduced another problem which manifests itself in the
> matrix_exponential_8 test failing occasionally. The issue is illustrated by the
> following program:
>
> int main()
> {
>  Matrix<std::complex<long double>, 2, 2> mat1;
>  std::cout << mat1.data() << std::endl;
>  std::cout << mat1.col(0) << std::endl;
> }
>
> The last line fails 50% of the time on the assertion on MapBase.h:174 which is
>
>      eigen_assert(EIGEN_IMPLIES(internal::traits<Derived>::Flags&AlignedBit,
> (size_t(m_data) % (sizeof(Scalar)*internal::packet_traits<Scalar>::size)) == 0)
>        && "data is not aligned");
>
> AlignedBit is set and sizeof(Scalar) is 32, but m_data is only aligned on a
> 16-byte boundary, so it bombs out.

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. Actually, it would be very useful to isolate
this alignedness computation in a little template helper, maybe call
it is_fixed_size_vectorizable<Type>::value  (returning 0 or 1).
Indeed, this will be the 3rd place in the code where we need to check
that, the others being compute_matrix_flags and in DenseStorage.h.

Are you doing this fix?

Cheers,
Benoit

>
> I don't know whether it's the assertion or the computation in
> compute_matrix_flags in XprHelper.h that is wrong.
>
>
> Jitse
>
>
>
>