Skip to content
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.

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

Check for native architecture and set GTSAM_COMPILE_OPTIONS_PUBLIC accordingly #1296

Merged

Conversation

stefangachter
Copy link
Contributor

Currently, the flag GTSAM_BUILD_WITH_MARCH_NATIVE is limited to Apple architecture only. The commit checks, if the compiler supports -march=native and adds the option to GTSAM_COMPILE_OPTIONS_PUBLIC if GTSAM_BUILD_WITH_MARCH_NATIVE is on, see #1138 (review)

Note, could not test with Apple architecture, because hardware missing.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

@stefangachter great stuff, I just had some comments.

cmake/GtsamBuildTypes.cmake Outdated Show resolved Hide resolved
cmake/GtsamBuildTypes.cmake Outdated Show resolved Hide resolved
cmake/GtsamBuildTypes.cmake Outdated Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator

@stefangachter I'll run this on my M1 mac tonight and if it works well, I'll approve. Thank you so much for your patience!

list_append_cache(GTSAM_COMPILE_OPTIONS_PUBLIC "-march=native")
endif()
if(GTSAM_BUILD_WITH_MARCH_NATIVE)
if(APPLE AND (${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang") AND ("${CMAKE_CXX_COMPILER_VERSION}" VERSION_LESS "15.0"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @stefangachter. Can you please explain why you're checking the version of the C++ compiler? I'm a bit lost on the reason here and it would be good to have the reason on record.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Found the reason.
AppleClang 15+ supports march=native.

@varunagrawal
Copy link
Collaborator

@stefangachter can you please check the box which says "Allow edits from maintainers"? It should be right below the unsubscribe button in the right column.

@stefangachter
Copy link
Contributor Author

@stefangachter can you please check the box which says "Allow edits from maintainers"? It should be right below the unsubscribe button in the right column.

@varunagrawal, it was already checked, if I am not mistaken:
image

@varunagrawal
Copy link
Collaborator

@stefangachter thanks! I was doing something silly.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

I made some minor updates using my CMake foo but kept the logic pretty much the same. Thanks for the major contribution @stefangachter!

@varunagrawal varunagrawal merged commit d3440f8 into borglab:develop Jan 2, 2023
@stefangachter
Copy link
Contributor Author

Thank you, Varun! Wouldn't consider this a major contribution, but a contribution nevertheless :-)

@stefangachter stefangachter deleted the bugfix/cmake_march_native branch January 2, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants