Use MPI_BYTE for unknown types #81
Conversation
| @@ -99,10 +96,8 @@ MPI_Datatype mpi_type() { | |||
| else if constexpr (std::is_same_v<T, Kokkos::complex<double>>) | |||
| return MPI_DOUBLE_COMPLEX; | |||
|
|
|||
| else { | |||
| static_assert(std::is_void_v<T>, "mpi_type not implemented"); | |||
There was a problem hiding this comment.
It might make sense to assert that T is trivially copyable at least.
There was a problem hiding this comment.
Looking ahead, I think trivially-copyable is a problem for Trilinos FAD types - they are actually trivially-copyable, but because of how they are implemented they have non-default ctors, etc that makes C++ think they're not trivially-copyable.
There was a problem hiding this comment.
Maybe if we run into this problem (a problem of success), we can make a special configure-time toggle to remove this trivially-copyable check.
There was a problem hiding this comment.
One thing I'm not a fan of is that if you have type T with large sizeof(T), n * sizeof(T) may exceed INT_MAX, while storing them in a view is totally fine.
There was a problem hiding this comment.
For now, maybe we can just send as MPI_BYTE as long as the size (MPI_Type_size) matches the extent (MPI_Type_get_extent) and the lower bound (MPI_Type_get_extent) is 0; is this the best way to check if a type is contiguous?
There was a problem hiding this comment.
Kokkos could offer a trait that overrides std::is_trivially_copyable so that users can tell Kokkos that they know what they are doing (e.g., not trying to copy pointers between processes).
There was a problem hiding this comment.
We discussed that also in the context of deep_copy where we are assuming that all types used in kernels are logically trivially copyable. There, we decided that this would be too big of a change to error out unless the user has specialized such a trait.
|
To actually fix #79 you would also need to make sure that you scale the number of bytes to send and receive accordingly, though. |
I feel sheepish for forgetting this. |
| @@ -99,6 +96,9 @@ MPI_Datatype mpi_type() { | |||
| else if constexpr (std::is_same_v<T, Kokkos::complex<double>>) | |||
| return MPI_DOUBLE_COMPLEX; | |||
|
|
|||
| else if constexpr (std::is_trivially_copyable_v<T>) | |||
| return MPI_BYTE; | |||
There was a problem hiding this comment.
I'm not sure that this fix alone solves your problem. I'm missing the part where we get the size of T. In https://github.com/kokkos/kokkos-comm/blob/develop/src/impl/KokkosComm_send.hpp#L72 we only get the type (MPI_BYTE here) and take the span, not the span*sizeof(T). Maybe mpi_type() should provide a count multiplier as well?
There was a problem hiding this comment.
Yes, @masterleinad identified this problem. I'm not sure what's the best way to proceed here. Your suggestion is one of the possible variants.
|
What if the fallback case was just to construct, register, and cache a contiguous MPI derived datatype that is sizeof T in a static variable. |
That may work. I'm not familiar with it. Could you post a snippet of code to demonstrate what you are thinking? |
|
Untested: else /*constexpr*/ {
static MPI_Datatype cached = MPI_DATATYPE_NULL;
if (MPI_DATATYPE_NULL == cached) {
MPI_Type_contiguous(sizeof(T), MPI_BYTE, &cached);
MPI_Type_commit(&cached);
}
return cached;
}each call to mpi_type with a new T that reaches the fallback with a different T will have a different |
Oh, that's a cool idea! This sounds like it could work well. Maybe create a templated function to create mpi types, so that users could specialize it on their own. With the default being the two MPI calls you posted. |
|
Yeah it would be great if this could be exposed to the user to override it. I think we'd have to refactor this to be a SFINAE-style implementation rather than |
Yeah, probably just a templated function with the default type creation and specializations. |
|
Who frees the temporary type? It should probably be put into a list of types that get destroyed at the end... |
|
|
|
Are we actually required to call MPI_Type_free? |
Some (like this presentation, slide 4) state that "You are not required to use it or deallocate it, but it is recommended (there may be a limit)". I didn't see anything in the standard saying that it is required. |
|
It's not required. Datatypes are a funny thing in MPI because they can persist between Sessions (mostly for historically reasons, because they are not bound to any Session). Having said that, KokkosComm should strive to exit cleanly with all resources freed. This makes it easier to use leak detection tools and is just good practice. |
|
This ties in to the init / finalize discussions I think. We have to have the opportunity to do something before / when MPI_Finalize gets called. |
|
We could have a function that returns a `std:pair<MPI_Datatype,
std::size_t>`. For basic types `.second` is 1, for derived types it's
`sizeof(T)` and `.first` is `MPI_BYTE`. It's all `constexpr` and with big
count in MPI we don't need to worry about integer overflows.
…On Mon, Jun 24, 2024, 11:49 Carl Pearson ***@***.***> wrote:
This ties in to the init / finalize discussions I think. We have to have
the opportunity to do something before / when MPI_Finalize gets called.
—
Reply to this email directly, view it on GitHub
<#81 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACTXKJSUURADZYJURVUZMWTZJA5YRAVCNFSM6AAAAABJE6KTX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBWHA4DQNBUGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Fix #79.