Re: [eigen] Mapping array of scalars into quaternions

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

2009/11/13 Mathieu Gautier <mathieu.gautier@xxxxxx>:
>> Everytime you add a EIGEN_STRONG_INLINE on a function that has a
>> nontrivial body of its own (rather than just forwarding the call to
>> another function), I need to see some justification (as far as I can
>> see, that would have to be justified by performance measurements). I'm
>> by no means an expert, i'm really honestly asking. My fear is about
>> uselessly increasing code size (and killing instruction caches).
>
> OK, I perform some benchmark on simple operations with gcc4.4, icc and vc++
>
> 1/ q = q1*q2;
> 2/ q = q1*q2*q3.inverse()*q4;
> 3/ v = q._transformVector(v1);
>
> (v : vector, q : quaternion)
>
> using EIGEN_STRONG_INLINE didn't improve the performance with vc++. The asm
> generated is never completly inlined both with inline and
> EIGEN_STRONG_INLINE. It's also have no influence with gcc, since the asm
> generated is always fully inlined.
>
> with icc (disabling any SSE extension) I have the following results for 10
> trials of 100,000,000 operations on a Intel xeon X5550:
>
> mean for 100,000,000 operations
>
>    inline | EIGEN_STRONG_INLINE
> 1/   2.22s |    1.70s
> 2/   8.21s |    6.24s
> 3/   2.63s |    2.02s
>
> the ratio is roughly the same (0.76).

Thanks for doing these experiments. This really pleads in favor of
EIGEN_STRONG_INLINE, and the fact that gcc and msvc are inlining
anyways means that it's quite safe.

Just one thing: the current definition is:

// EIGEN_FORCE_INLINE means "inline as much as possible"
#if (defined _MSC_VER)
#define EIGEN_STRONG_INLINE __forceinline
#else
#define EIGEN_STRONG_INLINE inline
#endif

so it only is different from "inline" when _MSC_VER is defined (which
normally means that the compiler is MSVC). Does that mean that ICC
defines _MSC_VER? Or that you are using a modified EIGEN_STRONG_INLINE
for ICC?

Anyway, I'm now OK for EIGEN_STRONG_INLINE.

>  > I also have a few superficial comments:
>>
>> First, "inline" keywords need only be put on function definitions, no
>> need to put them on function definitions [1]. I realize that
>> Quaternion.h already had this problem before your patch. I just am
>> noticing it now, because EIGEN_STRONG_INLINE takes more space on my
>> screen.
>
> :) When using icc, EIGEN_STRONG_INLINE is only effective if put in class
> body. It must be specific to the "__force_inline" keywords. "inline" keyword
> may be remove from the class body and only put in front of method
> definition.

Hm, this is very interesting to know. Thanks for this finding.

>> Second, in the unit test (thanks for writing it), you could just
>> reserve static arrays with EIGEN_ALIGN16, see what we do in
>> MatrixStorage.h.
>
> ok
>
>> Third, when you significantly extend/rewrite a file, add yourself a
>> copyright line. I already added you one on Quaternion.h, also do it on
>> quaternion.cpp.
>
> I'll change these to points and I'm waiting a reply about inlining to send a
> new patch.

Ok, just send and i'll push.

Benoit