-
Notifications
You must be signed in to change notification settings - Fork 407
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
Try using FindOpenMP instead of figuring out flags manually #4105
Conversation
Retest this please. |
1 similar comment
Retest this please. |
This pull currently doesn't add any linker flags but passes our regular CI. It would be good to check this on a somewhat wider ranger of configurations. |
I configured and built this PR with intel/17.0.4 and intel/19.0.3 each with cmake/3.16.0 and cmake/3.19.3, there were no issues in any of these cases |
I think the interesting cases to test with are |
I'm not able to test clang_msvc, here is from my attempt to test with clang_cray that failed to configure (I don't have much experience with that env, so I could be doing something wrong): Load cray computing env and cmake/3.16.2
Configure
Configure output:
Is there anything obvious that I may be doing wrong on my end? |
I almost forgot that I can check myself for clang_cray. On
so that seems to work. |
@masterleinad I'm able to get the develop branch to configure,
However I am running into build errors in this case:
|
DId you test CUDA + OpenMP? |
I did some testing with ICPX for simple OpenMP run, OpenMPTarget, and CUDA+OpenMP even with nvc++ and it all works. So this appears to be good on that front. Still we need to resolve the cray stuff. |
@masterleinad is that still WIP? Or does this work now? |
I didn't change anything but I would think that the error in #4105 (comment) is more an issue with the machine being misconfigured. I would be interesting to hear if the problem persists. |
4a29d0a
to
1f233f4
Compare
1f233f4
to
040419a
Compare
I updated this pull request to just use
which is the same approach we use for
which doesn't do anything for |
I guess that would also help in #5184 |
0ff97bf
to
db66946
Compare
One disadvantage of this approach would be that using different compilers for Kokkos and downstream applications causes issues when using |
Was that ever something we really supported? Is it something we want to explicitly disclaim from 4.0 onward? |
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 good to me
Hi, correct me if I'm wrong, but this fix is not in 3.7.1 right ? |
Yes, this feature is only merged to the 4.0.00 release branch. |
Thanks. Is there a roadmap page somewhere ? |
We normally only do release notes (#5439, https://github.com/kokkos/kokkos/blob/master/CHANGELOG.md). |
Trying to address #3256 by relying on
CMake
'sFindOpenMP
.