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
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).
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.
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.
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
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).
(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...
For reference: https://bitbucket.org/eigen/eigen/commits/104e5c911fe Remove the usage of result_of for DenseBase::redux
Should be good enough for that bug entry: https://bitbucket.org/eigen/eigen/commits/f2e6b911501d/
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1006.