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

Define MPI_ABI and remove MPI_ABI_VERSION #29

Closed
wants to merge 2 commits into from
Closed

Conversation

dalcinl
Copy link
Collaborator

@dalcinl dalcinl commented Mar 18, 2024

I'm opening this PR to discuss two somewhat separate but related concepts.

Define MPI_ABI macro

  • We need a way to perform conditional compilation under the ABI. This can be as trivial as #define MPI_ABI 1 if the implementation supports the ABI. This is what MPICH has implemented so far. This is also mpi4py is already taking advantage of. This PR the definition of MPI_ABI to the stubs mpi.h.
  • As this is primarily and so far about conditional compilation in C, this definition does not apply to Fortran.

Remove MPI_ABI_VERSION and MPI_ABI_VERSION

  • @hzhou has expressed his opposition to adding MPI_ABI_(SUB)VERSION to mpi.h as an unnecessary leakage of ABI details to the user.
  • @dalcinl has also expressed concerns about whether MPI_ABI_(SUB)VERSION is ultimately useful, as the each MPI standard implicitly defines the backward compatibility guarantees from the set of added and removed symbols from each MPI standard release.

@dalcinl dalcinl self-assigned this Mar 18, 2024
@dalcinl
Copy link
Collaborator Author

dalcinl commented Mar 18, 2024

Comment from @hzhou in favor of MPI_ABI and rejecting MPI_ABI_VERSION #28 (comment).

Copy link
Collaborator

@hzhou hzhou left a comment

Choose a reason for hiding this comment

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

I like it :)

@jeffhammond
Copy link
Member

okay i guess i'll update the latex tomorrow then

@dalcinl
Copy link
Collaborator Author

dalcinl commented Mar 18, 2024

@jeffhammond I did not say it explicitly, but then MPI_ABI_get_version() should also be gone from your proposal.

However, I still think we need the MPI_Abi_is_supported(int *flag) query routine (or a slightly better name). If @hzhou objects, I can elaborate.

@hzhou
Copy link
Collaborator

hzhou commented Mar 18, 2024

@jeffhammond I did not say it explicitly, but then MPI_ABI_get_version() should also be gone from your proposal.

However, I still think we need the MPI_Abi_is_supported(int *flag) query routine (or a slightly better name). If @hzhou objects, I can elaborate.

You need a runtime method to query the ABI (implementation)? I think that's fine. But maybe we can make it more general by returning a (registered) ABI enum (MPICH_ABI, OMPI_ABI, MPI_ABI)?

@dalcinl
Copy link
Collaborator Author

dalcinl commented Mar 18, 2024

You need a runtime method to query the ABI (implementation)? I think that's fine.

Actually, just a boolean whether the ABI is supported or not. From there, with MPI_Get_version() and MPI_Get_library_version(), users should be able to perform any dispatch they need.

But maybe we can make it more general by returning a (registered) ABI enum (MPICH_ABI, OMPI_ABI, MPI_ABI)?

I would love to have something that. I feel a bit uncomfortable hardwiring in the MPI standard the name of concrete implementations, but practicality beats purity. Not to mention that this may not age well if a brand-new implementation ever appears. Well, a brand new implementation would probably follow just MPI_ABI rather than supporting the standard MPI_ABI and a new brew of their own. Then the standard could advice that MPICH_ABI and OMPI_ABI are the legacy ABIs in widespread use at the time the ABI was proposed. We would just need need to iron-out the exact names and the wording to propose this. BTW, we cannot reuse the name MPI_ABI because that was the macro advertising ABI support.

@hzhou
Copy link
Collaborator

hzhou commented Mar 19, 2024

You need a runtime method to query the ABI (implementation)? I think that's fine.

Actually, just a boolean whether the ABI is supported or not. From there, with MPI_Get_version() and MPI_Get_library_version(), users should be able to perform any dispatch they need.

Okay, so why MPI_Get_library_version() couldn't tell you whether it is the MPI-ABI? BTW, what does supported in MPI_Abi_is_supported mean?

@dalcinl
Copy link
Collaborator Author

dalcinl commented Mar 19, 2024

Okay, so why MPI_Get_library_version() couldn't tell you whether it is the MPI-ABI?

Because a) that's not the primary purpose of that routines, and b) the way to encode ABI support would depend on how each implementation whats to dump that information in the library version string using a non-standard format.

BTW, what does supported in MPI_Abi_is_supported mean?

Indeed, the name is confusing. It means that that particular library uses the MPI standard ABI. To put a concrete example centered around MPICH: if you dlopen() and query that routine from libmpi.so.12, you get false; but if you dlopen() and query from libmpi_abi.so.0, you get true.

Maybe a nicer approach is to consider MPI_Get_library_abi(...):

  • MPI_Get_library_abi(int *abi) returning abi=0 for the standard MPI ABI we are proposing, (abi&1) == 1 for MPICH, and (abi&2) == 2 for Open MPI. Derivatives of each major implementation can agree each other (and document) on other bits of abi to distinguish among themselves.
  • A even more flexible alternative could be to make the argument a string: MPI_Query_library_abi(char abi[64]). If the library uses the standard ABI, it returns the all-lowercase string abi="mpi", otherwise it returns an implementation-specific string, e.g. something like "mpich" or "ompi".

I think I'm really liking this one, it is a concrete refinement of Hui's previous proposal.

@jeffhammond
Copy link
Member

jeffhammond commented Mar 26, 2024

#include <mpi.h>
#if defined(MPI_ABI_VERSION) && (MPI_ABI_VERSION >= 1)
#define MPI_ABI 1
#endif

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 this pull request may close these issues.

3 participants