search

KIT-MRT / mrt_cmake_modules

Automated catkin project handling framework
BSD 3-Clause "New" or "Revised" License
41 stars 24 forks source link

UseMrtStdCompilerFlags.cmake defaults the target to sandybridge (x86) #7

Closed filiperinaldi closed 4 years ago

filiperinaldi commented 4 years ago

Arguably not an bug report, but a feature request...

UseMrtStdCompilerFlags.cmake restricts the use of these modules to amd64 machines by defaulting to "sandybridge":

# Select arch flag
if(MRT_ARCH)
  if(NOT MRT_ARCH STREQUAL "None" AND NOT MRT_ARCH STREQUAL "none")
    set(_arch "-march=${MRT_ARCH}")
  endif()
else()
  # sandybridge is the lowest common cpu arch for us
  set(_arch "-march=sandybridge")
endif()
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${_arch}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${_arch}")

Trying to build a project using mrt_cmake_modules in an AArch64 (Arm) machine fails. The failure can be obvious (you will see compilation errors and with the mach=sandybridge flag being used, or some find_package() may fail with no reason (this is due the fact they use the CMake's try/run mechanism to build small C apps).

Note that this was the real problem why find_package(OpenMP) was failing for me on this issue: #6

Is there a good reason why sandybridge is being forced as the defaul mach? The compiler will already have default to a sane value for the target it is running from. If the "application" using mrt_cmake_modules has a minimum arch target requirement, then I suppose this is what MRT_ARCH is there for.

poggenhans commented 4 years ago

The reason sandybridge is chosen by default is actually that the default choice of the compiler for x86 CPUs (namely x86-64) is pretty shitty, because it completely disables any vectorized instructions. Because almost all of the x86 CPUs used today support AVX, we chose to use sandybridge as a fallback, because it was the first CPU to fully support it. And I would say that this default has payed off. It introduced very few problems and made a lot of code faster by default.

I see that this default is problematic when compiling for different architectures. If you have a proposal for any logic to reliably detect this case, I would be happy to introduce it. CMAKE_CROSSCOMPILING seems to be available, but I a not sure if that works for ARM as well. And it will for sure not work for ARM-native builds...

filiperinaldi commented 4 years ago

I will definitely have a look. Cross compilation does not seem to be a major issue as you can add MRT_ARCH to the toolchain.cmake file as the user knows what the target is.