Re: [eigen] arm/neon inside eigen internals

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

2010/3/4 Konstantinos Margaritis <markos@xxxxxxxx>:
> On Thursday 04 March 2010 09:36:08 Thomas Capricelli wrote:
>> (mostly aimed at Konstantinos I guess)
>>
>> In the recent patch providing Neon support (Thanks!), i have some questions
>>  regarding the use of __ARM_NEON__
>>
>> 1) Detection of architecture should be done in Core only, and in other
>>  files, we should only use things like EIGEN_VECTORIZE_NEON. This allows,
>>  for example, the user to define EIGEN_DONT_VECTORIZE and have indeed his
>>  code not making use of simd
>
> well, the use of EIGEN_VECTORIZE_NEON is equivalent to __ARM_NEON__, ie both
> are defined or not defined at the same time -at least right now, I can think
> of no case where one compiles with -mfpu=neon but requires no vectorization,
> one would use -mfpu=vfp instead. So perhaps I can replace __ARM_NEON__ with
> EIGEN_VECTORIZE_NEON, no problem in that.

If EIGEN_DONT_VECTORIZE was defined by the user then suddenly
EIGEN_VECTORIZE_NEON is not defined but __ARM_NEON__ is still defined.

But EIGEN_VECTORIZE_NEON is still not the right thing to check for:

See Thomas' suggestion below:

>
>> 2) For ABI reason, as explained in Eigen/src/Core/util/Macros.h:40, i think
>>  the line 42 should use __arm__ instead of __ARM_NEON__ or
>>  EIGEN_VECTORIZE_NEON

Yes __arm__ is the right thing to check for.

Until yesterday alignment was disabled altogether on ARM. Now it is
enabled but since GCC has bugs, we disable certain assertions on Neon.
What about non-Neon ARM users? I'm afraid they still get the asserts
even though GCC doesn't properly align the stack --- i.e. the devel
branch must not be working for them.

So: for all these checks disabling asserts (and disabling alignment of
fixed-size matrices) the proper check is __arm__

The ultimate clean solution that's needed anyway is to decouple
EIGEN_DONT_ALIGN into two things: EIGEN_DONT_ALIGN_HEAP and
EIGEN_DONT_ALIGN_STACK. We'd then just define EIGEN_DONT_ALIGN_STACK
on ARM...

>>
>> I first thought about patching/cleaning this myself, but i feel
>>  uncomfortable doing so because:
>>
>> 1) I want to know what Konstantinos thinks about it
>> 2) I'm not sure ABI compatibility is such a hard requirement on embedded
>>  platforms  such as ARM 3) in some places (like
>>  Eigen/src/Core/util/Memory.h:426 or Eigen/src/Core/util/XprHelper.h:92)
>>  i'm not sure whether __ARM_NEON__ should be replaced by __arm__ or
>>  EIGEN_VECTORIZE_NEON. That is, should this codepath be used on all arm
>>  platforms (think 'abi') or only in the neon case.
>
> Regarding use of __arm__ instead:
>
> These #ifdefs exists only because GCC on ARM cannot guarrantee that
> __attribute__((aligned(16))) will be honoured (sometimes it is, sometimes it
> is not). This is a bug, and I am actually discussing it (via email) right now
> with the GCC ARM developers. Unaligned loads/stores is irrelevant if one is
> not using vectorization so there is no need to force these ifdefs on all arm
> platforms -in fact, I don't think it makes any difference whatsoever. I could
> replace it with EIGEN_VECTORIZE_NEON, though.

The place that really matters is the assertions. non-Neon ARM users
are probably getting assertion failures now. the asserts must get
disabled for them too.

Benoit