| Summary: | Reducing copies of nestByValue() | ||
|---|---|---|---|
| Product: | Eigen | Reporter: | Gael Guennebaud <gael.guennebaud> |
| Component: | Core - expression templates | Assignee: | Nobody <eigen.nobody> |
| Status: | NEW --- | ||
| Severity: | Optimization | CC: | chtz, gael.guennebaud, jacob.benoit.1 |
| Priority: | Normal | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
With the aforementioned changes we could even get rid of explicit nestByValue() by introducing them implicitly when detecting a rvalue ref of type T for which T usually prefers to be nested by reference. The user would still have to turn named local temporaries into a xvalue using move().
To this end we could define two helpers (not tested):
template<typename T>
using nest_by_value_t = typename conditional<prefer_nesting_by_ref<T>::value,NestByValue<T>,T>::type;
template<typename T, typename = enable_if<prefer_nesting_by_ref<T>::value> >
auto nest_by_value(T&& x) {
return NestByValue<T>(std::move(x));
}
template<typename T, typename = enable_if<!prefer_nesting_by_ref<T>::value> >
T&& nest_by_value(T&& x) {
return std::move(x);
}
Then, for instance, in operator+:
auto DenseBase<Derived>::operator+(DenseStorage<Other> &&other) && {
return CwiseBinaryOp<scalar_sum_op<...>, const nest_by_value_t<Derived>, const nest_by_value_t<Other> >(
nest_by_value(std::move(derived())), nest_by_value(std::move(other.derived())) );
}
-- 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/1663. |
After resurrecting nestByValue(), I decided to see whether the numerous copies could be removed by move ctors. Mostly for curiosity as I'm not sure nestByValue() is a really useful feature as it was broken since 3.3.0 and nobody complained! OK, so let's consider this toy example: auto get_xpr_with_temps(const MatrixXd& a) { return a.rowwise().reverse().eval().nestByValue() + (a+a).eval().nestByValue(); } Currently this creates 6 temporaries, and 4 copies. Ouch! Those can be reduced to 2 temporaries only (the ideal) if we do the following changes: 1) Make all functions return non const expressions, including eval(). So far, they return const object to prevent stupid code like: expr.eval().normalized(); 2) Add ctors from rvalue refs to all expressions, e.g.: CwiseBinaryOp(Lhs&& aLhs, Rhs&& aRhs, const BinaryOp& func = BinaryOp()) : m_lhs(std::move(aLhs)), m_rhs(std::move(aRhs)), m_functor(func) {} (plus the 2 hybrid versions for binary expressions). 3) Add overloads members/operators for rvalue ref, e.g., for EIGEN_MAKE_CWISE_BINARY_OP(), we have to add: template<typename OtherDerived> \ EIGEN_DEVICE_FUNC EIGEN_STRONG_INLINE EIGEN_CWISE_BINARY_RETURN_TYPE(Derived,OtherDerived,OPNAME) \ (METHOD)(EIGEN_CURRENT_STORAGE_BASE_CLASS<OtherDerived> &&other) && \ { \ return EIGEN_CWISE_BINARY_RETURN_TYPE(Derived,OtherDerived,OPNAME)(std::move(derived()), std::move(other.derived())); \ } \ /* + the 2 hybrid versions */ The regular const operator/member must be qualified by "const &" instead of "const" only. That's all. Whereas points 1) and 2) are easily manageable, point 3) represents a lot of boilerplate code. E.g., think about all the block variants! Moreover, this only addresses heap-allocated temporaries, not fixed size ones.