This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 156 - Several CDash Tests fail under MSVC 2008 + 2010
Summary: Several CDash Tests fail under MSVC 2008 + 2010
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: unspecified
Hardware: All All
: --- Unknown
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.0-beta3
  Show dependency treegraph
 
Reported: 2011-01-18 12:46 UTC by Frank Meier-Dörnberg
Modified: 2019-12-04 10:11 UTC (History)
3 users (show)



Attachments
Should fix the alignment issue. (1.41 KB, patch)
2011-02-03 19:27 UTC, Hauke Heibel
no flags Details | Diff
This should finally fix the alignment issue while preventing the issues raised in bug #54. (7.13 KB, patch)
2011-02-05 17:39 UTC, Hauke Heibel
no flags Details | Diff

Description Frank Meier-Dörnberg 2011-01-18 12:46:06 UTC
Since mid of December, I thing, several Test fail because of errors like listed below:

(See e.g. http://eigen.tuxfamily.org/CDash/viewBuildError.php?buildid=4555)

MSVC 2008 + MSVC 2010 Errors:

#1
c:\develop\cdash\src\eigen\src/Core/Transpose.h(78) : error C2719: 'matrix': formal parameter with __declspec(align('16')) won't be aligned

#2
c:\develop\cdash\src\eigen\src/Core/Block.h(122) : error C2719: 'xpr': formal parameter with __declspec(align('16')) won't be aligned


probably unrelated:
#3
C:\Develop\cdash\src\test\sparse_product.cpp(59) : error C2666: 'Eigen::DynamicSparseMatrix<_Scalar>::operator =' : 5 overloads have similar conversions

MSVC 2010 Error:
#4
C:\Program Files\Microsoft Visual Studio 9.0\VC\INCLUDE\deque(715) : error C2719: '_Val': formal parameter with __declspec(align('16')) won't be aligned
        c:\develop\cdash\src\eigen\src/StlSupport/StdDeque.h(98) : see reference to class template instantiation 'std::deque<_Ty,_Ax>' being compiled
        with
        [
            _Ty=Eigen::internal::workaround_msvc_stl_support<Eigen::Matrix<float,4,4>>,
            _Ax=Eigen::aligned_allocator_indirection<Eigen::internal::workaround_msvc_stl_support<Eigen::Matrix<float,4,4>>>
        ]
Comment 1 Frank Meier-Dörnberg 2011-02-03 18:24:02 UTC
#3 is most likely unrelated an covered by Bug 164.
Comment 2 Hauke Heibel 2011-02-03 19:27:44 UTC
Created attachment 80 [details]
Should fix the alignment issue.
Comment 3 Hauke Heibel 2011-02-03 19:28:43 UTC
I attached a potential fix for the alignment issue. It is not ready to be pushed since I did not run all tests.

I think also that the specializations of as_argument for Matrix and Array are not needed anymore.
Comment 4 Hauke Heibel 2011-02-03 19:42:17 UTC
I am unable to reproduce any errors on MSVC 2010 (32bit, debug|release) for

o stddeque_1
o stddeque_2
o stddeque_3
o stddeque_4
o stddeque_5
Comment 5 Benoit Jacob 2011-02-03 19:51:16 UTC
(In reply to comment #3)
> I attached a potential fix for the alignment issue. It is not ready to be
> pushed since I did not run all tests.

Yeah, for this kind of change, it's really important to run all tests.

I see that you are still using nested<>... here. I seem to remember that's bad. I don't remember the precise thought process, but it was along the lines of: this as_argument business is just about only allowing the constructors, in Block and other expressions, that are legitimate given the constness of the expression type. This has nothing to do with nested<>.

Also it's a pity that we don't have tests for const-correctness. As was discussed, it's hard to write tests that succeed when they fail to compile. But the stuff that you're touching here was introduced in r3632 for const correctness, bug 54.

So could you please at least check that the test case in bug 54 comment 28 still fails to compile? Please test all 3 parts of it individually. (Block, Transpose, Diagonal).

> 
> I think also that the specializations of as_argument for Matrix and Array are
> not needed anymore.

Indeed. It's funny, I originally tried something along the lines of what you are doing here and don't remember why I didn't settle for this.
Comment 6 Benoit Jacob 2011-02-03 19:59:07 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > I attached a potential fix for the alignment issue. It is not ready to be
> > pushed since I did not run all tests.
> 
> Yeah, for this kind of change, it's really important to run all tests.
> 
> I see that you are still using nested<>... here. I seem to remember that's bad.

Sorry... I just realized that that's exactly what my current code is doing.

Ignore this part! You've got my green light to push this, and also to remove the specializations (I confirm that with your patch they are not needed anymore) if it passes tests and doesn't introduce a const correctness regression. (See my previous comment)
Comment 7 Frank Meier-Dörnberg 2011-02-03 20:02:26 UTC
I will be happy to run tests on my lame crutch.
Then it will take just a little ...
Comment 8 Hauke Heibel 2011-02-03 22:35:45 UTC
I have to do some more tests tomorrow. It is not as easy as I thought and right now I am a bit tired. Benoit, your link to bug #54 was pretty essential. ;)
Comment 9 Hauke Heibel 2011-02-05 12:53:57 UTC
Benoit, I have fixed as_argument and everything works as expected. I am still not pushing it since I am now pretty much convinced that as_argument is not needed at all!

I tested that too and it seems to be working. My current approach is to simply replace 

typename internal::as_argument<XprType>::type xpr

by

XprType& xpr

Do you recall why you did not do this in the beginning? Is there an issue I am overseeing!?
Comment 10 Hauke Heibel 2011-02-05 15:56:47 UTC
What I proposed does not compile right away but I am prepping the repo such that this change will work correctly.
Comment 11 Hauke Heibel 2011-02-05 17:39:16 UTC
Created attachment 83 [details]
This should finally fix the alignment issue while preventing the issues raised in bug #54.
Comment 12 Hauke Heibel 2011-02-06 20:01:25 UTC
Everything is fixed besides the std::deque issue though this one is already covered by bug #83 and thus I am closing this issue.
Comment 13 Nobody 2019-12-04 10:11:02 UTC
-- 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/156.

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