search

Shark-ML / Shark

The Shark Machine Leaning Library. See more:
http://shark-ml.github.io/Shark/
GNU Lesser General Public License v3.0
504 stars 131 forks source link

Issue with VS 2017 15.8: _ENABLE_EXTENDED_ALIGNED_STORAGE vs _DISABLE_EXTENDED_ALIGNED_STORAGE #249

Open shlublu opened 6 years ago

shlublu commented 6 years ago

The latest update of Microsoft Visual Studio 2017 (VS 2017 15.8, 14th of Aug 2018) makes the following error message to appear when I compile any project using Shark 4.0.0:

You've instantiated std::aligned_storage<Len, Align> with an extended alignment (in other words, Align > alignof(max_align_t)). Before VS 2017 15.8, the member type would non-conformingly have an alignment of only alignof(max_align_t). VS 2017 15.8 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility (only for uses of aligned_storage with extended alignments). Please define either (1) _ENABLE_EXTENDED_ALIGNED_STORAGE to acknowledge that you understand this message and that you actually want a type with an extended alignment, or (2) _DISABLE_EXTENDED_ALIGNED_STORAGE to silence this message and get the old non-conformant behavior.

This is a static_assert(_Always_false<_Aligned>). Looks like this comes from <shark_includes>/linalg/blas/kernels/default/mgemm.hpp but I'm not 100% sure whether this is the only file that is concerned. Should this be needed I kept the full log of what happened, I can send it to you.

My local Shark 4.0.0 was installed according to the official guidelines available at http://www.shark-ml.org/sphinx_pages/build/html/rest_sources/installation.html#windows-and-visual-studio but with one difference, not relevant here I think: I'm using Boost 1.67 instead of Boost 1.59 mentioned in the document above.

As I need to move forward on my project while Shark 4.0.0 works perfectly in my environment so far, I'm currently using _DISABLE_EXTENDED_ALIGNED_STORAGE for now in my project. This seems to work properly. I'll try to recompile both Shark 4.0.0 and my project with _ENABLE_EXTENDED_ALIGNED_STORAGE later when I'll have more time.

However it would be interesting to:

Thanks a lot for everything anyway. You guys do a great job and this lib is awesome!

Ulfgard commented 6 years ago

Hi,

Thanks for reporting this!

I think the correct behaviour should be given by _ENABLE_EXTENDED_ALIGNED_STORAGE. We wanted to have 16 byte alignment in GEMM so that the compiler could use fast SSE/AVX intrinsics. The old behaviour/disable flag will prevent the compiler from doing this.

I have no windows machine to test until Sunday. We will issue a bugfix release 4.0.1 where this is handled properly.

shlublu commented 6 years ago

Sounds great, thank you! At that stage, I can just confirm that _DISABLE_EXTENDED_ALIGNED_STORAGE works, which is not surprising as this is the way it used to be (but silently) before. However this looks a bit patchy-dirty :)

I won't have a chance to recompile my whole stuff (local Shark + client projects) using _ENABLE_EXTENDED_ALIGNED_STORAGE before a few weeks actually. But I'll give a try and let you know should I come back before the 4.0.1 !