cburstedde / libsc

The "sc" auxiliary library
www.p4est.org/
GNU Lesser General Public License v2.1
35 stars 34 forks source link

MPI 2.0 Datatypes #173

Closed hannesbrandt closed 6 months ago

hannesbrandt commented 6 months ago

In sc_mpi.h we define several MPI Datatypes that are not part of the MPI 1.3 standard, but were added in more recent versions: MPI_SIGNED_CHAR, MPI_INT8_T and MPI_UNSIGNED_LONG_LONG. Both MPI_SIGNED_CHAR and MPI_UNSIGNED_LONG_LONG are not yet used in libsc, while MPI_INT8_T is used several times in the test_mpi_pack example. It may be an option to remove them or to check for their existence during configuration.

cburstedde commented 6 months ago

In sc_mpi.h we define several MPI Datatypes that are not part of the MPI 1.3 standard, but were added in more recent versions: MPI_SIGNED_CHAR, MPI_INT8_T and MPI_UNSIGNED_LONG_LONG. Both MPI_SIGNED_CHAR and MPI_UNSIGNED_LONG_LONG are not yet used in libsc, while MPI_INT8_T is used several times in the test_mpi_pack example. It may be an option to remove them or to check for their existence during configuration.

Thanks for checking. As a third option, we might redefine:

Would this work cleanly without anyone even noticing? Otherwise we should really remove them and convert test_mpi_pack to using sc_MPI_BYTE.

hannesbrandt commented 6 months ago

As a third option, we might redefine:

  • sc_UNSIGNED_LONG_LONG to MPI_LONG_LONG
  • sc_MPI_SIGNED_CHAR to MPI_CHAR
  • sc_MPI_INT8_T to MPI_BYTE

Would this work cleanly without anyone even noticing?

I think this approach might cause troubles. According to the C99 standard long long can represent values up to $2^{63}-1$ while unsigned long long can represent values up to $2^{64}-1$. So, the name sc_UNSIGNED_LONG_LONG might tempt the user to assign values that can not be represented by MPI_LONG_LONG. Similarly, the number of bits in a byte is not defined to be 8 by the C standard and signed char might cover a different range than char.

cburstedde commented 6 months ago

As a third option, we might redefine:

  • sc_UNSIGNED_LONG_LONG to MPI_LONG_LONG
  • sc_MPI_SIGNED_CHAR to MPI_CHAR
  • sc_MPI_INT8_T to MPI_BYTE

Would this work cleanly without anyone even noticing?

I think this approach might cause troubles. According to the C99 standard long long can represent values up to 263−1 while unsigned long long can represent values up to 264−1. So, the name sc_UNSIGNED_LONG_LONG might tempt the user to assign values that can not be represented by MPI_LONG_LONG. Similarly, the number of bits in a byte is not defined to be 8 by the C standard and signed char might cover a different range than char.

Thanks for the analysis! Please feel free to push to #175, entirely removing questionable types if need be.

cburstedde commented 6 months ago

Pinging @lukasdreyer to warn that MPI_INT8_T may be removed. Please check if a workaround will be possible.

cburstedde commented 6 months ago

Merged. This is now officially a release candidate.

lukasdreyer commented 6 months ago

The workaround would be the same as you took in the test, to replace SC_MPI_INT8_T by SC_MPI_BYTE, although I am not happy with it, as it is not as declarative as it should be. We will probably create our own wrappers, T8_MPI_INT8_T, which I was hoping not being necessary to do.

cburstedde commented 6 months ago

The workaround would be the same as you took in the test, to replace SC_MPI_INT8_T by SC_MPI_BYTE, although I am not happy with it, as it is not as declarative as it should be. We will probably create our own wrappers, T8_MPI_INT8_T, which I was hoping not being necessary to do.

Thanks for your feedback. I understand this may incur some extra effort.

We'd be able to put it back in using a compile-and-link test as with other MPI features. Currently we test for basic MPI, for the MPI I/O API, and for some MPI 3 features. We might just realign these tests to check for MPI 1, MPI 2, and MPI 3 respectively, and include the data type checks with those.

We've already deprecated configuring MPI 1 without MPI 2 (files), since the replacement code for parallel file handling is not reasonable to maintain.

So for the next version we may come up with something along these lines.