Re: [eigen] about changeset 6eb14e380

2010/8/18 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>

history:

- I initially changed this assert while fixing the issue leveraged by
my "InnerPanel" changeset. Let me recall that the issue was that when
vectorization is not enabled the AlignedBit flags was *always* set
because in this case the object size (rows*cols) is always a multiple
of ei_pack_traits<scalar>::size (which is equal to 1). So now the
AlignedBit flag is never set for non vectorizable type (maybe this is
still not fully satisfactory). I also changed this assert (in MapBase)
because it looked weird to me to have such an alignment check
hardcoded to 16. bytes.

- Then I forgot to run all tests without vectorization, whence
yesterday issues...

- Hauke solved these issues by reverting this assert to a hardcoded
comparison to 16

- And I reverted it back again while disabling faulting unit tests.

Ok so now, to be future proof, the main question is how to properly
allow for different alignment requirements. In my opinion, alignment
requirement should be specified by ei_packet_traits because it might
likely depend on both the vectorization engine *and* on the scalar
type. E.g.:

template<> struct ei_packet_traits<Scalar> {
enum {
// ...
RequireSpecialAlignment = 0/1,
AlignmentInBytes = ..., // (e.g., 16)
// ..
};

Note that AlignmentInBytes can be smaller than sizeof(Scalar) (e.g.,
for complex), that is why it has to be specified in bytes.

So now it is pretty clear that we cannot check for alignment when
vectorization in disabled since we cannot predict what will be the
required alignment. This has two consequences:

- if the someone uses the advanced Aligned option of Map, then we
cannot guarantee that a code working without vectorization will
continue to work when vectorization is enabled. (this is not an issue
to me since this option is really for advanced user who understands
all the issues related to alignment).

- we cannot easily be binary compatible between non vectorized and
vectorized code. This one is a major issue which has to be solved.
Solving it means we are also able to "fix" the previous one.
Basically, what we would like to do is to tell Eigen the following:
compile without vectorization but aligned the data as for this given
vectorization engine. More generally, if someone want to support more
than one vectorization engine at runtime we would have to pick the
strictest alignment requirement of both... this looks scary, so I'll
stop here for now, maybe I'm just saying stupid things and the
solution is somewhere else?

Thanks a lot for the explanation. I agree that we need to solve this big problem, what I don't agree with is your particular changeset taken isolated, today, without fixing the rest of the problem that you describe here.

Now on to discussing this problem:

Concretely, what are the future SIMD instruction sets that you are thinking about, that would have different alignment requirements? I've heard that AVX/LRB have wider packets, but do they also have bigger alignment requirements?

Notice that ABI only has to be considered within a given architecture. For example, if a totally new CPU architecture appears tomorrow with 1024 bit alignment, that's not a big deal to support since it is independent on other platforms.

So the only worrisome thing is a single CPU architecture that could have multiple SIMD instruction sets, with different alignment requirements.

Is that the case on x86 or x86-64 with AVX/LRB?
Besides x86 and x86-64 are there any other known architectures where this can happen?

Let's only worry about that if concrete architectures have this issue.

Benoit

cheers,

gael

On Wed, Aug 18, 2010 at 3:06 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
> Hi,
>
> changeset 6eb14e380 does:
>
> [SNIP]
> -      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)
> [SNIP]
> + #ifdef
> EIGEN_VECTORIZE
>
> VERIFY_RAISES_ASSERT((MQuaternionA(array3unaligned)));
> +
> #endif
> [SNIP]
> + #ifdef EIGEN_VECTORIZE
>    VERIFY_RAISES_ASSERT((Map<VectorType,Aligned>(array3unaligned, size)))
> + #endif
>
> I don't get it. First, the ei_assert above really wants to check 16-byte
> alignment. This value 16 is being replaced by a product that will only
> evaluate to 16 if vectorization is enabled. I guess that's why below you add
> the #ifdef EIGEN_VECTORIZE checks. But the unaligned-array-assert is
> unaffected by EIGEN_VECTORIZE. We wanted to keep it enabled even with
> vectorization disabled, so that people who develop on a platform without
> vectorization don't suddenly get crashes when they move to a platform with
> vectorization. So it is not good to have this VERIFY_RAISES_ASSERT only
> enabled when EIGEN_VECTORIZE is defined. All in all, this looks like a hack
> to compensate another hack.
>
> So, I really don't understand what this changeset fixes. For me, the tests
> are already passing before this changeset.
>
> Cheers,
> Benoit
>