Bugzilla – Bug 1144

Regression in 3.3_beta1

Last modified: 2016-01-25 15:14:17 UTC

3.3_beta1 has a regression, which is demonstrated by the following code: Eigen::Vector3f x(5,5,5); Eigen::Matrix3f m = x.asDiagonal(); x = Eigen::Vector3f::Zero()+m*x; In this case x will be zero [0,0,0], but should be [25,25,25]. Changing the order components "fixes" the problem: x = Eigen::Vector3f::Zero()+m*x;

I can confirm this. The same happens if Vector3f::Zero() is replaced by Vector3f x(5,5,5), y(0,0,0); Matrix3f m = x.asDiagonal(); x = y + m*x; It seems that evaluator_traits::AssumeAliasing is false and the expression gets evaluated as: x = y; x += m*x;

The "fixed" version in my case should be: x = m*x + Eigen::Vector3f::Zero();

thanks for the finding, I'm on it. I'll probably move this AssumeAliasing attribute to evaluator::Flags.

Partly fixed in: https://bitbucket.org/eigen/eigen/commits/0db197c797c3/ however, there is still a regression when the destination is resized and that the Product expression is hidden, as: x = (M*x).cwiseAbs(); In 3.2, M*x was evaluated at the construction of the cwiseAbs expression, and therefore x was used before we reached operator=. In 3.3, x is resized before M*x is evaluated within a temporary. If "assume-aliasing" is transferred to cwiseAbs, then we will end-up with two temps instead of one: one for M*x and one for the complete right-hand side because of the "assume-aliasing" flag. The only solution I can think of, would be to create the right-hand side evaluator before we resize the destination. That's easy to say, but that's inconvenient to implement because this would require to duplicate the resize logic (which currently occur once) in all specializations of Assignment... and this is thus error prone too. And we would do this for only one particular case... Overall, I would vote for not fixing that one. Finally, it is not that much different than: x = x.segment(...); for which we do not anything special...

We could whenever dest needs to be resized, evaluate the source into a temporary and then move/swap the result into the actual destination. Unfortunately, this needs to be detected at run-time and likely requires an additional branch. Also, this won't work for matrices where Max(Rows|Cols)AtCompiletime is set. If that's too inefficient or to complicated to implement, I'm ok with leaving aliased expressions which require resizing as undefined.

and that double the number of temporaries, for instance in: x = (M*x).cwiseAbs(); with M non-square, we would do: temp1 = M*x; temp2 = temp1.cwiseAbs(); x = temp2; Actually, that's what I first did when trying to fix the original regression.

If you wrote MatrixXd A,B,temp; temp.noAlias() = (A*B).cwiseAbs(); this should be equivalent to temp.noAlias() = A*B; temp = temp.cwiseAbs(); right? So (in case swap is O(1)) we could translate this B = (A*B).cwiseAbs(); into temp.noAlias() = (A*B).cwiseAbs(); B.swap(temp); I'm not saying it's trivial to implement, though ...

(In reply to Christoph Hertzberg from comment #7) > temp.noAlias() = (A*B).cwiseAbs(); > this should be equivalent to > temp.noAlias() = A*B; temp = temp.cwiseAbs(); I did some debugging and now see that my assumption was wrong. I admit that this is difficult, for example since it won't work for things like the following: C.noalias() += (A*B).cwiseAbs(); I don't see a good solution for avoiding a temporary here (using lazyProduct would avoid the temporary, of course). I guess if it only resizing assignments cause trouble now, it is not really worth blocking 3.3 for that -- putting a warning about that in the release notes would be mandatory then, of course.

Clarified doc: https://bitbucket.org/eigen/eigen/commits/4bb9a076d0da/ Summary: Bug 1144: clarify the doc about aliasing in case of resizing and matrix product. and 3.3 summary page: http://eigen.tuxfamily.org/index.php?title=3.3#Changes_that_might_impact_existing_code