|Re: [eigen] Re: 3.0.4 coming soon, testing appreciated|
[ Thread Index |
| More lists.tuxfamily.org/eigen Archives
- To: eigen@xxxxxxxxxxxxxxxxxxx
- Subject: Re: [eigen] Re: 3.0.4 coming soon, testing appreciated
- From: Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
- Date: Sun, 27 Nov 2011 15:00:22 -0500
- Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:content-transfer-encoding; bh=ByI7C354HJgijwtDd6OBS1dEjgUCWT63gI+4rypCwx8=; b=SOypR45dZVVXHikeOI3itIZyLZGLX/knOM2Ah7S7HbCP9dKb+IOSdPxsZdd/lW+ePk bSayQnUoIp+UrYSDMHpUOWLN8IGKJ6uDTtQODb+Ag38+k/JSQblq8RUWLCJA2D345mew 3zmHdN46+HmAZAYRfgcpIRyxGuwNiKXnwNOpk=
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
> 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
> % (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
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
> 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.