Alignment issues with Eigen datatypes and vectorization about grid_map HOT 8 OPEN

Dear @mjschuster, thanks a lot! This might actually be an issue and I can look into this at a later point. If you have a suggestion on how to implement this fix, feel free to create a PR.

from grid_map.

Dear @pfankhauser, thanks!
It'd be great if you could have a look at this. I myself probably won't have the time for a pull request on this issue anytime soon. My suggestion would be to go through the code and see if its possible to

• use Eigen aligned allocators for all structs / classes having fixed-size vectorizable Eigen datatypes as members (see https://eigen.tuxfamily.org/dox-devel/group__TopicStructHavingEigenMembers.html) (that should be easy)
• make sure not to pass Eigen objects by value (see https://eigen.tuxfamily.org/dox-devel/group__TopicPassingByValue.html)
• use Eigen aligned allocators for all stl containers with fixed-size alignable Eigen datatypes or classes containing them (see https://eigen.tuxfamily.org/dox-devel/group__TopicStlContainers.html) (this might be the most difficult / annoying part)

As a workaround so far, I just deactivated vectorization by defining EIGEN_DONT_VECTORIZE and EIGEN_DISABLE_UNALIGNED_ARRAY_ASSERT as suggested in https://eigen.tuxfamily.org/dox-devel/group__TopicUnalignedArrayAssert.html to keep ABI compatibility.

I however suspect that typical operations performed on grid maps could greatly benefit from vectorization - it would be interesting to have a speed comparison.
(As far as I understand it, this is only about explicit vectorization in the Eigen library. So a good compiler with appropriate optimization settings might still be able to add some vectorization through SSE etc. instructions on its own -- though probably less optimal?)

from grid_map.

Hi all,

as we had some alignment related crashes with grid_map, so I added a small fix concerning the alignment issues with Eigen. Although I did not have time to thoroughly look through all the code, I added a missing EIGEN_MAKE_ALIGNED_OPERATOR_NEW in the GridMap class, which has fixed-size Eigen members.

I created a pull request with this fix. But I have to add, that as I did not check the entire code, there could be lurking other alignment issues.

Best regards

from grid_map.

Thanks, @thomasemter! I merged your PR and extended it with other places where this code had fixed-size Eigen members. Let me know if we need to work more on this issue or you have any other problems.

from grid_map.

from grid_map.

Thanks @thomasemter @pfankhauser !
While these fixes should address some of the alignment issues, I think there still are some more (and more complicated) ones left, in particular with respect to my second and third bullet point in the comment above.

For example in https://github.com/ethz-asl/grid_map/blob/master/grid_map_core/include/grid_map_core/Polygon.hpp#L206 you create a std::vector of Position, which is a typedef to Eigen::Vector2d (see https://github.com/ethz-asl/grid_map/blob/master/grid_map_core/include/grid_map_core/TypeDefs.hpp#L18).
According to https://eigen.tuxfamily.org/dox-devel/group__TopicStlContainers.html you should use the Eigen allocators then: "In practice you must use the Eigen::aligned_allocator (not another aligned allocator), and #include <Eigen/StdVector>.", e.g. something like:
std::vector<Position, Eigen::aligned_allocator<Position> > vertices_;

In addition, according to https://eigen.tuxfamily.org/dox-devel/group__TopicPassingByValue.html these Eigen types should not be passed by value, as is done for example in https://github.com/ethz-asl/grid_map/blob/master/grid_map_core/include/grid_map_core/Polygon.hpp#L157 and many other places.

Thus, while it might work for now without crashes, there are several places left that can cause trouble in the future...

from grid_map.

Hi all,

Thanks @mjschuster for pointing out other Eigen aligment issues.

I fixed all std::vector Eigen aligment issues I could find. I hope I have found all and I also did not find any other std container type in the code that would need fixing.

from grid_map.

Hi all,

I did test the alignment fixes with the current master and rebased the fixes accordingly.

from grid_map.