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.

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

Convert user types to MPI_BYTE #79

Open
aprokop opened this issue Jun 11, 2024 · 2 comments · May be fixed by #81
Open

Convert user types to MPI_BYTE #79

aprokop opened this issue Jun 11, 2024 · 2 comments · May be fixed by #81

Comments

@aprokop
Copy link
Contributor

aprokop commented Jun 11, 2024

This logic seems problematic to me:

else {
static_assert(std::is_void_v<T>, "mpi_type not implemented");
return MPI_CHAR; // unreachable
}

If I have a View<Box*>, with Box being some POD struct, in my opinion that view should be sent with message size view.size() * sizeof(View::value_type) and MPI type MPI_BYTE. Instead, right now it refuses to deal with it.

@aprokop aprokop linked a pull request Jun 11, 2024 that will close this issue
@cedricchevalier19
Copy link
Member

I am not an MPI expert, but I think doing something like that is not semantically sound. Custom MPI datatypes can handle this stuff, but perhaps we can also explicitly serialize the data.

@devreal @hmt23 ?

@devreal
Copy link
Collaborator

devreal commented Jun 12, 2024

The problem with opaque types in general is that they may contains parts that we must not copy to other processes (like virtual function tables). Kokkos could check for is_trivially and transfer only those types with MPI_BYTE. We'd still copy padding bytes, which may be acceptable (it isn't for MPI derived types but Kokkos could relax that constraint). Other types need a way of serializing, maybe in the spirit of Boost::serialization (which is slow without some hackery, ask me how I know...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants