-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Enable OpenMP with Apple Clang (Mac default compiler) #5146
Conversation
@hcho3 Just want to make sure that you are aware of the following issues that have been fixed only in the latest CMake version ( Fix: https://gitlab.kitware.com/cmake/cmake/merge_requests/3916 In case you do not want to bump CMake version and keep it
|
@StrikerRUS Indeed, the build fails for CMake 3.12. I had tested this PR on my Macbook Pro and it was using CMake 3.16. Switching to CMake 3.12 resulted in error Since Homebrew makes it easy to install latest versions of packages, I'm inclined to require CMake 3.16+ for Mac OSX users. |
4654020
to
1b31fb0
Compare
3309906
to
c2b582a
Compare
c2b582a
to
bec99c3
Compare
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.
Do you know whether the OMP flags are correctly set for CUDA files? One pain point in CMake is, flags for C++ and CUDA are separately managed.
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.
LGTM. Thanks for cleaning up the code along with the fix.
Thanks for reviewing. Don't merge this before dmlc/dmlc-core#586, as without it this PR will break. |
Ready for merging? |
I'll merge dmlc/dmlc-core#587 soon (today). Then I'll merge this PR. |
Done. Please merge this when all tests are done running. |
@hcho3 @StrikerRUS Thanks! |
Simplify logic for importing OpenMP into the CMake build by treating OpenMP as a target. We no longer have to set compiler flags; all it takes is to import target
OpenMP::OpenMP_CXX
viatarget_link_libraries()
. See https://cliutils.gitlab.io/modern-cmake/chapters/packages/OpenMP.html for more information.This will be especially useful for Mac OSX users. The old way requires them to use Homebrew GCC, which is a heavy dependency (*). The new way allows them to use Apple Clang (default system compiler) instead, provided that they install OpenMP library via Homebrew (
brew install libomp
). Furthermore, we can release XGBoost 1.0.0 on Homebrew with OpenMP enabled.CMake version is raised to
3.93.12 to accessOpenMP::OpenMP_CXX
target. 3.12 is needed to usetarget_link_libraries()
with object targetobjxgboost
. Note that we've been already requiring 3.12 for the GPU algorithms.Closes #4477.
Depends on dmlc/dmlc-core#586
(*) See discussion at Homebrew/homebrew-core#43246, where Homebrew maintainers asked to remove GCC dependency from XGBoost.