New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 1144 - Regression in 3.3_beta1
Regression in 3.3_beta1
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Core - expression templates
3.3 (current stable)
All All
: Highest Wrong Result
Assigned To: Nobody
:
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2016-01-07 10:37 UTC by gladk
Modified: 2016-01-25 15:14 UTC (History)
3 users (show)



Attachments

Description gladk 2016-01-07 10:37:58 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;
Comment 1 Christoph Hertzberg 2016-01-07 11:15:33 UTC
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;
Comment 2 gladk 2016-01-07 12:17:32 UTC
The "fixed" version in my case should be:

x =  m*x + Eigen::Vector3f::Zero();
Comment 3 Gael Guennebaud 2016-01-07 16:59:50 UTC
thanks for the finding, I'm on it. I'll probably move this AssumeAliasing attribute to evaluator::Flags.
Comment 4 Gael Guennebaud 2016-01-09 08:00:09 UTC
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...
Comment 5 Christoph Hertzberg 2016-01-11 13:48:25 UTC
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.
Comment 6 Gael Guennebaud 2016-01-12 08:44:55 UTC
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.
Comment 7 Christoph Hertzberg 2016-01-12 09:16:44 UTC
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 ...
Comment 8 Christoph Hertzberg 2016-01-12 21:21:22 UTC
(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.
Comment 9 Gael Guennebaud 2016-01-25 15:14:17 UTC
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

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