|Summary:||Add support for C++17 operator new alignment|
|Component:||Core - vectorization||Assignee:||Nobody <eigen.nobody>|
|Severity:||Feature Request||CC:||chtz, gael.guennebaud, jacob.benoit.1, laurent.rineau, markos, patrikhuber, taylor|
|Version:||3.3 (current stable)|
|Bug Depends on:|
Description Alex 2017-03-25 17:03:18 UTC
Comment 1 patrikhuber 2018-01-15 02:08:40 UTC
gcc supports this too from 7.0, and MSVC supports this now too from VS 2017 15.5 on. I've just read through this whole allocator stuff, I discovered it more or less by chance. I have been using Eigen for a few years now and have also been using fixed-size Eigen types, like Vector2f or Vector4f, as part of std::vector, because that's a very natural thing to do. I found and read https://eigen.tuxfamily.org/dox/group__TopicStlContainers.html today, and I was very surprised by it. In fact, I didn't believe it. "What, I *have to* do this? Surely not? This must be some special case that's not concerning me. Surely I can just put a Vector2f into an std::vector. And it has always worked for me. Hmm, and the workarounds are all quite complicated and intrusive. I think I'll just ignore this." After googling some more, I found that I probably indeed should learn more about this... (https://stackoverflow.com/questions/31325769/using-eigenaligned-allocator-on-stdvector-on-classes-containing-eigen-t). It seems like on x86-x64 it just works because the compiler does the right thing and allocates 16 byte aligned. But there was some conflicting info whether this applies to stack and/or heap allocations. Hmm. I don't care about x86-32 anymore, but what about Android, iOS? It seems like many people are confused and have a very hard time dealing with this: e.g. https://github.com/googlecartographer/cartographer/issues/34, https://github.com/RobotLocomotion/drake/issues/1854. Apparently in the first link, they went to rewrite lots of their API to use PIMPL, just to accommodate for this peculiarity in Eigen. Hmm. Really, none of the solutions are great. It seems to me the best option is just to ignore all of it and hope for the best, and as long as one is not using an exotic or old platform, it's going to work. Maybe the documentation could be clarified a bit? And maybe updated if needed? What about C++11 alignas, does any of these alignment specifiers help? P0035R4 seems a very bright light at the end of this complicated tunnel. As all major compilers support it now (some of it already 1+ years ago), would this not be quite a high priority? Looking forward to any input and hope you appreciate my comment.
Comment 2 patrikhuber 2018-02-08 11:02:51 UTC
Here is a link to an old discussion about C++11 std::alignas/alignof, which I think is relevant: https://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2014/02/msg00063.html It's actually a great thread from the very beginning (https://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2014/02/msg00048.html), and I think it would really be time to go C++11/14. It's 2018 now, not 2014 anymore as in that thread, and good C++11/14 compilers are now mainstream on all major platforms and in fact have much less bugs than old compilers like <=gcc-4.9, <=MSVC2015 etc.
Comment 3 Gael Guennebaud 2019-02-19 10:29:59 UTC
I did some quick tests, and alignment_of seems to be properly handled in C++17 mode starting with gcc 7 and clang 5 (I don't have clang 4), which is consistent with this table: https://en.cppreference.com/w/cpp/compiler_support However, to make clang 5 and 6 happy I have to remove our custom operator new from PlainObjectBase. No problem with gcc 7 and clang 7. So it seems that we have nothing to do except removing EIGEN_MAKE_ALIGNED_OPERATOR_NEW_IF if proper C++17 support and updating the documentation, and add C++17 nightly builds...
Comment 4 Christoph Hertzberg 2019-02-19 12:29:02 UTC
Removing EIGEN_MAKE_ALIGNED_OPERATOR_NEW_IF in C++17 mode makes sense. And I guess we should then also use `std::aligned_alloc` instead of any hand-crafted aligned-allocs. I also totally agree on running the testsuite in C++17 (and C++14) mode (cf Bug 1655). I can test clang 3.5-3.9, 4.0, 5.0 and 6.0 relatively easy (but I have limited time, since this is also my work-pc).
Comment 5 Gael Guennebaud 2019-02-19 13:08:22 UTC
Created attachment 932 [details] Remove new operators in C++17 Here is the patch, fully tested with clang/gcc, but not with MSVC nor ICC.
Comment 6 Gael Guennebaud 2019-02-19 14:28:56 UTC
std::aligned_alloc has two limitations: - The size parameter must be an integral multiple of alignment. - The alignment must be a valid alignment supported by the implementation. "valid" is not defined. On POSIX system this is fine for us, but that's all. I'm afraid this will just over-complicate the, already messy, logic.
Comment 7 Gael Guennebaud 2019-02-20 13:07:12 UTC
Applied: https://bitbucket.org/eigen/eigen/commits/9c927294bfc0/ Summary: Bug 1409: make EIGEN_MAKE_ALIGNED_OPERATOR_NEW* macros empty in c++17 mode: - this helps clang 5 and 6 to support alignas in STL's containers. - this makes the public API of our (and users) classes cleaner And doc updates: https://bitbucket.org/eigen/eigen/commits/caa49b9bade8/
Comment 8 Nobody 2019-12-04 16:54:25 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/1409.