Skip to content

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
Contributor

@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.

@varunagrawal
Copy link
Contributor

@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
Contributor

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
Contributor

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
Contributor

@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
Contributor

@stefangachter thanks! I was doing something silly.

Copy link
Contributor

@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