Re: [eigen] two technical points: WithAlignedOperatorNew and std::complex casting

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

2008/12/25 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
> Hi,
>
> On Wed, Dec 24, 2008 at 7:13 AM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
>
>> 1) Currently, WithAlignedOperatorNew is empty if EIGEN_VECTORIZE is
>> not defined. Which amounts to say, "if there's no vectorization, then
>> no need to get aligned pointers". I think we should change that to
>> letting WithAlignedOperatorNew be always the same i.e. always get
>> aligned pointers. Reasons:
>>
>> a) the current solution seems dangerous to me if the user links
>> together two files a.cpp and b.cpp using Eigen, if a.cpp is compiled
>> without vectorization and b.cpp is compiled with vectorization, if a
>> structure created in a.cpp is passed to b.cpp then b.cpp will expect
>> it to be aligned...
>> Notice that for static alignment (EIGEN_ALIGN_128) we already go for
>> always-align-even-when-vectorization-is-disabled for similar reasons.
>
> ok, good point. So clearly we must be consistent here, and enable or
> disable both EIGEN_ALIGN_128 and WithAlignedOperatorNew and not only
> one or the other. The drawback to always align, even when this is
> useless, is a possible waste of memory. On the other hand, I guess
> that all architectures targeted by Eigen feature a vector unit
> (SSE/Altivec), and so the vectorized path is aimed to be enabled 99.9%
> of the time... and so I agree with your proposal to always enable
> WithAlignedOperatorNew. One more argument: in the future, if we
> realize this was not the right choice we can still go from "always
> align" to "align if vectorization is enabled" without any trouble
> while the other way round could break binary compatibility.

Commited in r903399.
The waste-of-memory issue is not a sufficient reason not to do that:
1) it is necessary anyway when vectorization is turned on
2) the binary compatibility issue prompting this change is a higher
priority, when vectorization is not turned on
Rather, we should educate our users about why doing
struct foo : Eigen:WithAlignedOperatorNew{
  Vector4f v;
  float x;
};
foo *array = new foo[many];
will result in a significant waste of memory (3/8 = 37.5% of occupied
memory is alignment whitespace).

>> 2) Should we allow doing "some_complex_matrix = some_real_matrix" ? In
>> other words should we allow operator= to implicitly cast from real to
>> complex?
>> In the last days I've been very conservative in these respects so
>> beta3 forbids that. But looking back to Assign.h, we are already
>> allowing some convenience here like some_row_vector =
>> some_column_vector. Maybe some_complex_matrix = some_real_matrix is
>> also in the domain of what we can allow to make the user's life
>> easier, without much risk.
>>
>> One of the reasons why we may want to forbid auto-casting, is when
>> casting has a direct or indirect (like preventing vectorization) cost.
>> However here, this casting seems to have neither cost. So... maybe
>> allow it?
>
> hm... IMO, I would prefer to be strict on casting simply because
> casting is *always* expensive and enforcing the user to do explicit
> cast will enforce him to think a bit whether he can avoid it or not.
> And again, we can still easily go from "no explicit casting" to "a few
> implicit casting" in the future, but not the other way round.
>
> On the other hand, I agree to allow mixing complex and real matrices
> when it makes sense and yield to optimizations (e.g., cwise-product).

OK to keep the current conservative behavior for now.
Just notice why I considered relaxing it a little in this particular case:
1) it didn't involve any non-explicit conversion ctor, it was just
behavior of the operator=.
2) it didn't involve any costly conversion like between float and
double, it was just a convenient way to say that a real number is a
complex number with imaginary part 0.
But ok to keep the current behavior for now. anyway a big overhaul of
complex support is in order for 2.1.

Cheers,
Benoit

---