-
Notifications
You must be signed in to change notification settings - Fork 779
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
Check for native architecture and set GTSAM_COMPILE_OPTIONS_PUBLIC accordingly #1296
Conversation
There was a problem hiding this 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.
@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! |
cmake/GtsamBuildTypes.cmake
Outdated
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")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
@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: |
@stefangachter thanks! I was doing something silly. |
There was a problem hiding this 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!
Thank you, Varun! Wouldn't consider this a major contribution, but a contribution nevertheless :-) |
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 toGTSAM_COMPILE_OPTIONS_PUBLIC
ifGTSAM_BUILD_WITH_MARCH_NATIVE
is on, see #1138 (review)Note, could not test with Apple architecture, because hardware missing.