New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 1006 - By-Value parameters of DenseBase::redux
Summary: By-Value parameters of DenseBase::redux
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - expression templates (show other bugs)
Version: 3.2
Hardware: x86 - 32-bit Windows
: Normal Internal Design
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-28 18:45 UTC by Basil Fierz
Modified: 2016-01-28 11:20 UTC (History)
4 users (show)



Attachments
Patch changing the BinaryOp parameters to references (5.21 KB, text/plain)
2015-04-28 18:45 UTC, Basil Fierz
no flags Details

Description Basil Fierz 2015-04-28 18:45:46 UTC
Created attachment 567 [details]
Patch changing the BinaryOp parameters to references

Dear Eigen Developer,

The goal of the project I’m was working on for quite some time, is to use Eigens template expression engine as a code-generator for vectorised computations.
The main concept is to wrap  __m128 and __m256 in scalar-like types in order to execute operation sequences on several data items (the source can be found here: https://github.com/bfierz/vcl).
This works surprisingly well in 64bit using MSVC (2010 to 2013) and shows the power behind the Eigens technology and the ability of the compiler to optimize code.

However, I was recently forced to work on 32bit windows machines. Since these do not allow aligned parameters, all ‘Scalar’ parameters need to be passed by reference.
And this is why I am writing here. Eigen does this in all the (for me) relevant cases, except one.

In DenseBase the redux method checks for the result of a BinaryOp parameter, which unfortunately takes by-value parameters:

template<typename BinaryOp>
EIGEN_DEVICE_FUNC
typename internal::result_of<BinaryOp(typename internal::traits<Derived>::Scalar,typename internal::traits<Derived>::Scalar)>::type
redux(const BinaryOp& func) const;

I could changes this with only few lines of code, however, I was not sure if I would raise any sleeping dragons with this change. Thus, do you think such a change can be done without significant side-effects? I have prepared a small patch (based on 3.2.4), which I attached to this email.

Thank you,

Basil Fierz
Comment 1 Christoph Hertzberg 2015-04-28 20:36:23 UTC
Does this bug cause unaligned assertions for you, or does your code not compile or does it generate warnings?
Do you have the same problem with Scalars returned by value?


Question to other developers:
What's the use of the result_of for redux() anyways? 
All redux_impl::run methods simply return Derived::Scalar, and the documentation says that BinaryOp must be associative, so 
  result_of<BinaryOp(T,T)>==T2 != T
would only make sense, if
  BinaryOp(T2,T), BinaryOp(T2,T2) and BinaryOp(T,T2)
also return T2 (and redux_impl would need to be rewritten accordingly).
Comment 2 Basil Fierz 2015-04-28 21:26:40 UTC
Sorry, somehow I haved missed to specify the exact type of error.
It fails to compile with the following error pointing to the redux method:

'unnamed-parameter': formal parameter with __declspec(align('16')) won't be aligned

Return values on the other hand do not seem to cause any problems.
Comment 3 Christoph Hertzberg 2015-04-29 09:48:07 UTC
That's a bit strange, since Scalar itself should not require alignment (unless we pass a packet somewhere). Could you attach a complete error log, please?

As for fixing this, I still think simply requiring that (for redux) BinaryOp must return a Scalar is the cleanest solution -- I doubt that we would break anything with that.
Comment 4 Basil Fierz 2015-04-30 07:29:23 UTC
This is the exact issue I have. I am using __m128 as scalar types, extended with operators to make the interface Eigen-compatible:

class float4 { __m128 d; };

These wide scalar types in turn cause the noted compilation error above pointing to the redux method in DenseBase. The patch I have appended does not change the return-value, however it changes the Interface of the BinaryOp template parameter to take const-reference parameters. 

Following you find the exact error message:

3>f:\libs\eigen-3.2.4\eigen\src/Core/DenseBase.h(415): error C2719: 'unnamed-parameter': formal parameter with __declspec(align('16')) won't be aligned
3>          f:\libs\eigen-3.2.4\eigen\src/Core/MatrixBase.h(50) : see reference to class template instantiation 'Eigen::DenseBase<Derived>' being compiled
3>          with
3>          [
3>              Derived=Eigen::Matrix<Vcl::float4,3,3,0,3,3>
3>          ]
3>          f:\libs\eigen-3.2.4\eigen\src/Core/PlainObjectBase.h(87) : see reference to class template instantiation 'Eigen::MatrixBase<Derived>' being compiled
3>          with
3>          [
3>              Derived=Eigen::Matrix<Vcl::float4,3,3,0,3,3>
3>          ]
3>          f:\libs\eigen-3.2.4\eigen\src/Core/Matrix.h(129) : see reference to class template instantiation 'Eigen::PlainObjectBase<Eigen::Matrix<Vcl::float4,3,3,0,3,3>>' being compiled
3>          F:\projects\vcl\src\libs\vcl.math\vcl\math\jacobieigen33_selfadjoint.cpp(38) : see reference to class template instantiation 'Eigen::Matrix<Vcl::float4,3,3,0,3,3>' being compiled
Comment 5 Gael Guennebaud 2015-05-04 11:50:09 UTC
I agree that Scalar must be passed by const reference, however I will update the result_of helper in a different way to keep it as more general as possible.

@Christoph, a user could take advantage of the current implementation of redux to fuse casting/reduction in a more efficient way, for instance to accumulate standard numbers (e.g., int/double) into an arbitrary precision variable (e.g., bignum,rational,mpreal).
Comment 6 Gael Guennebaud 2015-05-04 12:43:16 UTC
(In reply to Gael Guennebaud from comment #5)
> @Christoph, a user could take advantage of the current implementation of
> redux to fuse casting/reduction in a more efficient way, for instance to
> accumulate standard numbers (e.g., int/double) into an arbitrary precision
> variable (e.g., bignum,rational,mpreal).

hm, alright, to take advantage of this, redux_impl has to be updated (as you said, sorry for being too fast).

So finally, I agree that result_of can be remove here, and properly reintroduced later if real use cases appears...
Comment 7 Gael Guennebaud 2016-01-21 14:12:46 UTC
For reference:
https://bitbucket.org/eigen/eigen/commits/104e5c911fe
Remove the usage of result_of for DenseBase::redux
Comment 8 Gael Guennebaud 2016-01-28 11:20:56 UTC
Should be good enough for that bug entry:
https://bitbucket.org/eigen/eigen/commits/f2e6b911501d/

Note You need to log in before you can comment on or make changes to this bug.