Summary:  ByValue parameters of DenseBase::redux  

Product:  Eigen  Reporter:  Basil Fierz <basil.fierz>  
Component:  Core  expression templates  Assignee:  Nobody <eigen.nobody>  
Status:  RESOLVED FIXED  
Severity:  Internal Design  CC:  basil.fierz, chtz, gael.guennebaud, jacob.benoit.1  
Priority:  Normal  
Version:  3.2  
Hardware:  x86  32bit  
OS:  Windows  
Whiteboard:  
Attachments: 

Description
Basil Fierz
20150428 18:45:46 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). 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: 'unnamedparameter': 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 Eigencompatible: 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 returnvalue, however it changes the Interface of the BinaryOp template parameter to take constreference parameters. Following you find the exact error message: 3>f:\libs\eigen3.2.4\eigen\src/Core/DenseBase.h(415): error C2719: 'unnamedparameter': formal parameter with __declspec(align('16')) won't be aligned 3> f:\libs\eigen3.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\eigen3.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\eigen3.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/ 