Skip to content

COMP: removed hardcoded -march=corei7 compilation flag #2391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
seanm wants to merge 1 commit into from
Closed

COMP: removed hardcoded -march=corei7 compilation flag #2391

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions CMake/ITKSetStandardCompilerFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,10 +240,7 @@ function(check_compiler_optimization_flags c_optimization_flags_var cxx_optimiza
# Check this list on both C and C++ compilers
set(InstructionSetOptimizationFlags
# https://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/i386-and-x86_002d64-Options.html
# NOTE the corei7 release date was 2008
#-mtune=native # Tune the code for the computer used compile ITK, but allow running on generic cpu archetectures
-mtune=generic # for reproducible results https://github.com/InsightSoftwareConsortium/ITK/issues/1939
-march=corei7 # Use ABI settings to support corei7 (circa 2008 ABI feature sets, core-avx circa 2013)
Copy link
Copy Markdown
Member

@dzenanz dzenanz Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be exposed as advanced CMake configuration variables? Also, if sensible defaults could be chosen based on the host architecture that would be great.

Copy link
Copy Markdown
Contributor Author

@seanm seanm Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Querying the host architecture is part of the problem. It just doesn't work when cross compiling. If I'm on an arm computer and building for intel, it does not help to examine the host.

Copy link
Copy Markdown
Member

@dzenanz dzenanz Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-compiling is rare, and I think it is better to bother small number of people doing it with specifying a different option, than 99% of everyone else.

Copy link
Copy Markdown
Contributor Author

@seanm seanm Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not rare on macOS. These days, every app is built as a universal binary (i.e. containing intel & arm executable code). Whether you build your app on intel or arm, the other half of it is essentially cross-compiled.

Introspection of the host causes all kinds of problems. For example, HDF5 was doing a try-compile to check sizeof(long double). It's 16 on intel, but 8 on arm. Assuming the answer is the same for both your intel half and arm half is incorrect.

Copy link
Copy Markdown
Contributor Author

@seanm seanm Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and it's exactly to catch things like that long double example that I'm setting up a cdash submission that builds as x86_64-only but on arm64 hardware. Since the code can still be run (in emulation) it will reveal such problems.

This march=corei7 thing is the only remaining build issue.

Copy link
Copy Markdown
Member

@issakomi issakomi Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is challenging, x86_64 should work with -march=corei7, IMHO, even if it should be better used conditionally or removed. There is a problem with Eigen, even if pass most important

-DCMAKE_CXX_COMPILER_TARGET:STRING="x86_64-apple-macos11"
-DCMAKE_C_COMPILER_TARGET:STRING="x86_64-apple-macos11"

and other params itkExternal_Eigen3.cmake fails, sometimes provided target is used, but at some point
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -x c -target arm64-apple-macos11.1

Copy link
Copy Markdown
Member

@thewtex thewtex Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itkExternal_Eigen3.cmake

Where is this? We may need to propagage CMAKE_CXX_COMPILER_TARGET for Eigen.

Copy link
Copy Markdown
Member

@issakomi issakomi Mar 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is at line 58, the generation of Eigen3 failed with command above the line. I have tried to pass target, but finally i build own Eigen3. ITK configuration worked:

/Applications/CMake.app/Contents/bin/cmake -DBUILD_TESTING:BOOL=OFF -DCMAKE_BUILD_TYPE:STRING=Release -DCMAKE_CONFIGURATION_TYPES:STRING=Release -DCMAKE_SYSTEM_PROCESSOR:STRING=x86_64 -DCMAKE_OSX_ARCHITECTURES:STRING=x86_64 -DCMAKE_C_COMPILER_TARGET:STRING=x86_64-apple-macos10.14 -DCMAKE_CXX_COMPILER_TARGET:STRING=x86_64-apple-macos10.14 -DITK_USE_SYSTEM_EIGEN:BOOL=ON -DEigen3_DIR:STRING="/Users/mi/eigen/build" -G Xcode ../ITK

Looks like related things were correct:

-- Performing Test CXX_HAS_WARNING-mtune_generic
-- Performing Test CXX_HAS_WARNING-mtune_generic - Success
-- Performing Test CXX_HAS_WARNING-march_corei7
-- Performing Test CXX_HAS_WARNING-march_corei7 - Success

-- Performing Test VXL_HAS_SSE2_HARDWARE_SUPPORT
-- Performing Test VXL_HAS_SSE2_HARDWARE_SUPPORT - Success

-- Looking for 128-bit float. [Checking long double...]
-- Looking for 128-bit float. Found long double.

But there are still very many questions, the post will be too long if i start to write about, if someone will play with it, look into cmake logs too, specially cmake's "try compile" is difficult. Cmake_system_processor too, tried also to set in tool-chain file, but still don't understand, but seems to be not crucial if target triple is set. Nonetheless i could build one of my applications, but it is a little bit too early to declare x86_64 build on m1 as "supported", IMHO. Good thing is that arm64 native build seems to be OK.

Edit:

/Applications/CMake.app/Contents/bin/cmake -DBUILD_TESTING:BOOL=OFF -DCMAKE_BUILD_TYPE:STRING=Release -DCMAKE_CONFIGURATION_TYPES:STRING=Release -DCMAKE_APPLE_SILICON_PROCESSOR:STRING=x86_64 -DCMAKE_OSX_ARCHITECTURES:STRING=x86_64 -DCMAKE_C_COMPILER_TARGET:STRING=x86_64-apple-macos10.14 -DCMAKE_CXX_COMPILER_TARGET:STRING=x86_64-apple-macos10.14 -DITK_USE_SYSTEM_EIGEN:BOOL=ON -DEigen3_DIR:STRING="/Users/mi/eigen/build" -G Xcode ../ITK

or simplified:

/Applications/CMake.app/Contents/bin/cmake -DCMAKE_APPLE_SILICON_PROCESSOR:STRING=x86_64 -DITK_USE_SYSTEM_EIGEN:BOOL=ON -DEigen3_DIR:STRING="/Users/mi/eigen/build" -G Xcode ../ITK

Copy link
Copy Markdown
Member

@issakomi issakomi Mar 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewtex This change in itkExternal_Eigen3.cmake fixes Eigen3 failure, if CMAKE_APPLE_SILICON_PROCESSOR is used. More tests are required, but seems that other variables didn't help.

  execute_process(
    COMMAND
    ${CMAKE_COMMAND} ${_eigen3_source_dir}
+   "-DCMAKE_APPLE_SILICON_PROCESSOR=${CMAKE_APPLE_SILICON_PROCESSOR}"
    "-DCMAKE_INSTALL_PREFIX=${_eigen3_cmake_install_prefix}"
    "-DCMAKE_INSTALL_INCLUDEDIR=${_eigen3_cmake_install_includedir}"
    "-DCMAKE_INSTALL_DATADIR=${_eigen3_cmake_install_datadir}"
    "-DCMAKE_GENERATOR=${CMAKE_GENERATOR}"
    "-DCMAKE_SH=${CMAKE_SH}"
    "-DCMAKE_GENERATOR_TOOLSET=${CMAKE_GENERATOR_TOOLSET}"
    "-DCMAKE_SH=${CMAKE_SH}"
    "-DCMAKE_GENERATOR_PLATFORM=${CMAKE_GENERATOR_PLATFORM}"
    "-DCMAKE_GENERATOR_INSTANCE=${CMAKE_GENERATOR_INSTANCE}"
    "-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}"
    "-DCMAKE_C_FLAGS=${CMAKE_C_FLAGS}"
    "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
    "-DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}"
    ${_additional_external_project_args}
    WORKING_DIRECTORY ${_eigen3_build_dir}
    OUTPUT_VARIABLE ITKEigen3Config_STDOUT
    ERROR_VARIABLE ITKEigen3Config_STDERR
    )

Copy link
Copy Markdown
Member

@dzenanz dzenanz Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good analysis. Could you turn this into a patch @issakomi?

)
endif()
set(c_and_cxx_flags ${InstructionSetOptimizationFlags})
Expand Down