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

mpi4py #1070

Merged
merged 43 commits into from
Aug 2, 2023
Merged

mpi4py #1070

merged 43 commits into from
Aug 2, 2023

Conversation

alexnick83
Copy link
Contributor

@alexnick83 alexnick83 commented Jul 26, 2022

Adds basic mpi4py-syntax compatibility to MPI DaCe Python programs:

  • Method calls on mpi4py.MPI.COMM_WORLD are caught by function call replacements (the same as for dace.comm)
  • Method calls on mpi4py.MPI.COM_WORLD through an intermediate variable, e.g., commworld = mpi4py.MPI.COMM_WORLD; commworld.Bcast(A), are caught by method call replacements, which will also trigger for any mpi4py.MPI.Intracomm object. However, DaCe will return an error saying that only COMM_WORLD is currently supported.
  • (Experimental) Bcast calls on Cartcomm and Intracomm objects other than COMM_WORLD will trigger adding input Scalar int32 data to the SDFG with the same name as the object. This Scalar must be set equal to the communicator object's Fortran (int) handle. The LibraryNode will generate an MPI_Comm_f2c call to utilize the communicator directly in C.
  • MPI_Request is now a first-party opaque datatype to enable using mpi4py syntax for calls such as Isend and Irecv.

alexnick83 and others added 30 commits July 26, 2022 17:17
…mpi4py compatible cartesian comm methods, bcast, and allreduce.
…PI.Request. Added preprocessor class for converting modulo expressions for C/C++ compatibility.
Added mpi4py send/recv support and alltoall library node
…mpi4py.MPI.COMM_WORLD any longer. All (mpi4py) communicators are now allowed to pass as-is through preprocessing.
…n the ProgramVisitor's defined variables. When calling a method on an object, if the object is not in the ProgramVisitor's current/outer scope variables, pass to the method a tuple with the object's name and the object itself.
…le that holds the Fortran int handle of a communicator.
…ass method replacements should now trigger. Added (experimental) support for calling Bcast from a Cart/Intracomm object defined in CPython.
@alexnick83 alexnick83 requested a review from tbennun July 12, 2023 14:02
result.update({k: self.sdfg.process_grids[v] for k, v in self.variables.items() if v in self.sdfg.process_grids})
try:
from mpi4py import MPI
result.update({k: v for k, v in self.globals.items() if isinstance(v, MPI.Comm)})
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 happy about this. Can we generalize to add more "supported global types" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that we could add another API method to register supported global types, where the user would specify how exactly they should be handled. Shouldn't this be a new PR, though?

dace/frontend/python/newast.py Outdated Show resolved Hide resolved
@@ -4662,7 +4673,9 @@ def _gettype(self, opnode: ast.AST) -> List[Tuple[str, str]]:

result = []
for operand in operands:
if isinstance(operand, str) and operand in self.sdfg.arrays:
if isinstance(operand, str) and operand in self.sdfg.process_grids:
result.append((operand, type(self.sdfg.process_grids[operand]).__name__))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do process grids take precedence over everything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A communicator is registered in the process grids but has a corresponding Scalar with the same name (currently, an integer) to allow creating AccessNodes. So far, we only had persistent communicators, COMM_WORLD or communicators created in the program's init. However, going forward, we want to support communicator creation/deletion during the program's execution, raising the need for expressing dependencies among communicator creation routines and communication calls on the SDFG. I suppose the best solution is to make an opaque type and only check SDFG.arrays.

dace/frontend/python/preprocessing.py Outdated Show resolved Hide resolved


class ModuloConverter(ast.NodeTransformer):
""" Converts a % b expressions to (a + b) % b for C/C++ compatibility. """
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will make some memlets that use % more complex. Is there a way to limit this behavior? Maybe do it in the code generator (i.e., cppunparse or Mod in the runtime .h file)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is frontend specific (see, e.g., -2 % 5 Python vs C), so I would definitely not handle it in code generation. I don't believe that the added complexity is an issue (sympy is probably not going to simplify the modulo operator anyway, so I doubt it breaks any optimization). We could instead map it to a pymod call, but that is probably an even worse solution if the concern is the simplification of symbolic expressions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this issue comes up now and then, and we forget about it after a while. The problem is that we like to use Python syntax on the SDFG to take advantage of the AST module, but we want this syntax to follow C semantics. Therefore, the frontend has to rewrite certain Python expressions so that they return the same result both in Python and C/C++.

Copy link
Contributor Author

@alexnick83 alexnick83 Jul 23, 2023

Choose a reason for hiding this comment

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

Maybe you are thinking that SDFG elements that include Python code should be unparsed to C/C++ equivalent expressions (semantically) during code generation. However, then the other frontends must change to convert C/Fortran/whatever semantics to Python. Is this a good idea?

tests/library/mpi/mpi4py_test.py Outdated Show resolved Hide resolved
tests/library/mpi/mpi_alltoall_test.py Outdated Show resolved Hide resolved
dace/frontend/common/distr.py Outdated Show resolved Hide resolved
dace/frontend/common/distr.py Outdated Show resolved Hide resolved
dace/frontend/common/distr.py Show resolved Hide resolved
@alexnick83 alexnick83 requested a review from tbennun July 26, 2023 18:37
@alexnick83 alexnick83 merged commit 8adcea6 into master Aug 2, 2023
9 checks passed
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