robotology / osqp-eigen

Simple Eigen-C++ wrapper for OSQP library
https://robotology.github.io/osqp-eigen/
BSD 3-Clause "New" or "Revised" License
385 stars 114 forks source link

Add SIMD option to CMake #96

Closed costashatz closed 1 year ago

costashatz commented 3 years ago

In many of my projects, I am using SIMD flags with Eigen for improved performance (e.g., march=native). This PR adds an option to enable this in gcc and clang compilers. Feel free to add MSVC or ICC if needed. By default this option is OFF as native flags can cause many issues. Tested on an ArchLinux machine and works like charm with and without the flag (both the lib and the tests).

traversaro commented 3 years ago

Thanks! Just a curiosity, as I saw several of this logic in many projects, and while initially I thought they made sense, I am not sure anymore about this. There is any reason why you prefer to have this logic in each project, rather then just setting the compilation flags you want to use by seeting the CXXFLAGS environement variable or similar mechanisms, that permit to obtain the same result without the need to have specific logic in each project?

costashatz commented 3 years ago

@traversaro the main reason (for my side) is that I am not using CMake for my code. My tool of preference is waf, and there CXXFLAGS environmental variable is not read/used. To be honest though (since I am not using CMake a lot), and in my projects (using waf) I always use march=native by default, I never actually thought about that. If you think using the environmental variable is better, I can do this for my CIs/projects and you can ignore this PR. One counterexample is some corner cases per compiler: for example with latest gcc and eigen (in my projects, but I guess other people do similar stuff) we need both march=native and -faligned-new, but for older gcc or icc the extra flag is not needed (an error thrown for older gcc versions). Having a logic for a default environmental variable like this, it's not going to be easy. But to be honest, I am not considering myself an active CMake user, so I am totally fine with what you believe is best for your project.

traversaro commented 3 years ago

@traversaro the main reason (for my side) is that I am not using CMake for my code. My tool of preference is waf, and there CXXFLAGS environmental variable is not read/used.

Sorry for the late reply. If you are not using CMake for build osqp-eigen but you have your own waf-base build system for osqp-eigen, then it is not clear to me how adding the ADD_SIMD option is going to help you.

To be honest though (since I am not using CMake a lot), and in my projects (using waf) I always use march=native by default, I never actually thought about that. If you think using the environmental variable is better, I can do this for my CIs/projects and you can ignore this PR.

To clarify, CXXFLAGS is just one of the ways in which an arbitrary compilation flag can be inserted in a CMake project in a composable way, another is setting the CMAKE_CXX_FLAGS CMake option (that indeed is initialized by CXXFLAGS env variable). The one that you may prefer to use I guess depends on your setup and the tools you are using to compile.

One counterexample is some corner cases per compiler: for example with latest gcc and eigen (in my projects, but I guess other people do similar stuff) we need both march=native and -faligned-new, but for older gcc or icc the extra flag is not needed (an error thrown for older gcc versions). Having a logic for a default environmental variable like this, it's not going to be easy. But to be honest, I am not considering myself an active CMake user, so I am totally fine with what you believe is best for your project.

Actually this kind of scenario are the reason why I prefer to avoid having simd-activation logic in downstream projects. Let's say that at some point for your project you don't need anymore -march=native and -faligned-new, but some other options (perhaps depending on a new compiler version and Eigen version, released after the osqp-eigen version you are using was released). Similarly, other people in their projects may have other needs. If you are building ~20 projects, and this option logic is duplicated in all of them, I guess you need to manually modify this options logic in each one of them. This is error-prone, and depending on the system you use for patching the projects (for example, if you fork the projects) it may complicate updating some projects.

If instead you are setting this flags and their logic in just one place and you just propagate them to downstream projects build system using the existing "composability" features of CMake and other build systems, you just need to keep that logic in one place, and whenever you need to modify it, you can modify it just in one place. Regarding the difficulty of having this logic at the build tool level, I guess it depends a lot on the build tool/style that you are using. If you are using a superbuild using CMake's FetchContent, for example it is trivial to have exactly the same logic you have here. If instead you are using colcon from a bash script, you can just have the logic in bash and then set the CXXFLAGS env variable, or the CMAKE_CXX_FLAGS for each project via the --cmake-args colcon option. If you point me to the build tool/environment you are using, it may be easier to us to understand your actual problem, to avoid the XY problem.

traversaro commented 1 year ago

@costashatz are you still interested in replying/following up this PR, or can we close it? Thanks!

costashatz commented 1 year ago

@traversaro I would say that I agree with you and we should close this PR :) Thanks for all the comments!