-
Notifications
You must be signed in to change notification settings - Fork 88
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 missing MPI in custom-matrix-format example #1370
Conversation
This reverts commit 3905206.
Using MPI from the MPI compiler wrapper directly means that no include path or library is added for MPI. This gives errors if a file that is not compiled by the wrappers and is linked against/includes ginkgo can't find the MPI headers or implementation. This mostly effects device code, since they will most likely not be compiled with the MPI wrapper compilers.
Fixes part of #1367 |
sorry, I do not get the usage of |
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! One minor nit (we already have way too many configuration options)
set(GINKGO_MPI_ASSUME_NO_BUILTIN_MPI ON CACHE BOOL "Disables using MPI wrapper compiler directly.") | ||
mark_as_advanced(GIKNGO_MPI_ASSUME_NO_BUILTIN_MPI) |
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 we need to make this controllable at all? It seems to be relevant only in very limited circumstances, where either mpiexec
is missing from the environment, or the user has a MPI compiler wrapper loaded that is not the one specified by CXX_COMPILER
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.
TBH, I'm not 100% sure how cmake finds MPI with this option on. Perhaps there are some weird cases, where it doesn't use the same MPI as the MPI wrapper would have. So I added the option to turn it off. I also marked it as advanced to indicate that users generally need not to worry about it.
@yhmtsai If you set |
This whole setting has me a bit confused - theoretically, CMake should interrogate the compiler wrapper for the location of the |
@upsj The comment from |
@MarcelKoch yes, I'd really like to get a full trace of the execution, because this seems like a bug to me. For this to be fixed, we need |
@upsj I will open an cmake issue to see what's going on. From a brief look at the source, you're right, it always sets the include directories, but if a compiler wrapper is used, the value of the used variable is empty. So the source code comment is moot. |
Here is the issue, let's see what happens: https://gitlab.kitware.com/cmake/cmake/-/issues/25085 |
I no longer think this fix is necessary, instead we should be propagating the C++ compiler as CUDA host compiler correctly. |
This PR fixes the missing MPI in the custom-matrix-format example when using the MPI wrapper as CXX compiler. There are actually two ways of fixing this implemented here:
<ginkgo/ginkgo.hpp>
include in the cuda file. Now the MPI headers are not required when compiling the cuda code anymore.Both of these fix the issue. I think having both of them is a good idea, because 1. the include is just not necessary, and 2. this would allow users to include
<ginkgo/ginkgo.hpp>
in device code.