-
Notifications
You must be signed in to change notification settings - Fork 68
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
Fix benchmarks failing and adjust compilation flags #118
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.
Looks great, thank you @miclegr! Just one note (really a TODO) about applying these same optimizations on Windows, although only ~4% of our daily installs are on Windows so we may be able to do that separately.
python/CMakeLists.txt
Outdated
# Allow for some math optimization on unix but not -ffast-math | ||
# See: https://simonbyrne.github.io/notes/fastmath/#flushing_subnormals_to_zero | ||
if (IS_UNIX) | ||
add_compile_options(-fassociative-math -fno-signaling-nans -fno-trapping-math -fno-signed-zeros -freciprocal-math -fno-math-errno) | ||
endif() | ||
|
||
# Relase build on unix with all optimizations | ||
if (CMAKE_BUILD_TYPE STREQUAL "Release" AND IS_UNIX) | ||
add_compile_options(-O3) | ||
endif() |
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.
A TODO for later - we should probably specify /Ox
here when building for MSVC/Windows.
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.
Since it's trivial to add, I added it to this PR :)
Description
Three contributions in this PR:
-O3
and some math optimization