Skip to content
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.

Sign up for GitHub

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

Use MPI_BYTE for unknown types #81

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Use MPI_BYTE for unknown types #81

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/impl/KokkosComm_types.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@ template <typename Scalar>
MPI_Datatype mpi_type() {
using T = std::decay_t<Scalar>;

if constexpr (std::is_same_v<T, std::byte>)
return MPI_BYTE;

else if constexpr (std::is_same_v<T, char>)
if constexpr (std::is_same_v<T, char>)
return MPI_CHAR;
else if constexpr (std::is_same_v<T, unsigned char>)
return MPI_UNSIGNED_CHAR;
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


else {
static_assert(std::is_void_v<T>, "mpi_type not implemented");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to assert that T is trivially copyable at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return MPI_CHAR; // unreachable
Expand Down
Loading