-
Notifications
You must be signed in to change notification settings - Fork 34
Handle OpenMP libraries in portable way (issue #959) #960
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
Conversation
Looks like gcc has trouble with these changes. |
I can reproduce it. The problem is that with gcc it pulls in gcc's |
I see no way to specify for |
The way we were doing it was not portable "for a reason": we don't want to use Gomp because the performance isn't good. I'm not saying it was done in the right way, clearly not. But the reason is still important. So, my tip for this PR is to find a way to make it portable, but still require LLVM's OpenMP library and not the compiler's own. CMake find_package can be fiddled with HINTs and other flags, maybe this helps? |
Yeah, totally understand. Showing up of
|
I file CMake issue for that: https://gitlab.kitware.com/cmake/cmake/-/issues/26263 |
I'm closing this in favour of #984 which should work around the fact that we want LLVM's version. The answer in the CMake issue you reported means to me they won't try to make this better in any way. |
Just updated to Ubuntu 24.04 and got the same problem reported here, and the PR here does fix the issue. I'll reopen this and will test on other environments. Since we don't run benchmarks with GCC, I think it will be fine. |
The problem with GCC tests is:
|
#1019 superseeds it. |
This is now more robust than my previous trick, using @dbabokin's code and only hacking `openmp.cmake`, not the other ones. This works on my machine for GCC and Clang, let's see what the CI thinks about it. Fixes #959 --------- Co-authored-by: Dmitry Babokin <[email protected]>
Closes #959
See the issue for detailed problem description.