|Re: [eigen] two technical points: WithAlignedOperatorNew and std::complex casting|
[ Thread Index |
| More lists.tuxfamily.org/eigen Archives
- To: eigen@xxxxxxxxxxxxxxxxxxx
- Subject: Re: [eigen] two technical points: WithAlignedOperatorNew and std::complex casting
- From: "Gael Guennebaud" <gael.guennebaud@xxxxxxxxx>
- Date: Thu, 25 Dec 2008 12:19:52 +0100
- Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to :subject:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references; bh=gfPoVWrwfX10+Z2sakbLiKWC4Bu0UljdAewlcRPueRs=; b=dTG53OqOXedwMkjoGXCNUWbklARBXntxiv6f5qaSFt5vol7LW/x7XbUAQVu4+yQFe6 qqml6/NRdNSUH9dk30WawxzKvUTrweLQQ5KZjo9p7FahoqLR1JYXDLpqckWgCxYH5U5N OfG1b5kgZW7rH66nFya5cfBjYkCO6B59rxPss=
- Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=NzyUWff05uXX3p5fjIVpH2BuYWwK7HBSjDyOoZM9ES866E4ek22X6gSqj7i8oWkgqF iVerOLVrCXyp+ekEmFMKx/VGe9vw+k9iItnpIEjK7OWafO8MvOYVpX8J6SnUDaoJ2Mjk smrABIbKo0uMjnLfBt11qCjYh5PRtBKMZFFzU=
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.
> 2) Should we allow doing "some_complex_matrix = some_real_matrix" ? In
> other words should we allow operator= to implicitly cast from real to
> 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).
> And yes, a user (Ricard) already reported to have been relying on this
> auto casting in the past so it's really a concrete issue.