Generate bigcount interfaces for Fortran and C #12226

jtronge · 2024-01-10T21:16:33Z

This adds scripts for generating the C and Fortran mpi_f08 API bindings from template files, while also generating bigcount interfaces for those that require them. On the Fortran side it also adds support for TS 29113 when possible, allowing for better Fortran array handling that matches the standard (some files were imported from PR #10302).

Python >=3.6 is required for running these scripts, which is only necessary when the binding files have not already been generated. Users of the distribution tarball should not need to generate these files and thus should not require Python.

We used https://github.com/cea-hpc/pcvs-benchmarks and the MPI4PY test suite to help ensure all big count interfaces (C and Fortran) are being generated.

PR #12033 is a previous version of this focused specifically on ABI support.

Note that there are additional changes needed to Open MPI for bigcount support. One is that the datatype system needs to be embiggened. This PR includes workarounds for this lack of support that will need to be changed once the datatype system is embiggened. These areas of code are marked with TODO:BIGCOUNT.

jsquyres

I haven't finished reading / understanding / reviewing both generate_bindings.py scripts yet, but since I'm so late for this and the clock is ticking on @jtronge's availability, I'm going to submit what I have so far.

ompi/mpi/fortran/use-mpi-f08/Makefile.am

jsquyres · 2024-01-30T20:13:41Z

ompi/mpi/fortran/use-mpi-f08/Makefile.am


 # JMS Somehow this variable substitution isn't quite working, and I
 # don't have time to figure it out.  So just wholesale copy the file
 # list.  :-(
-#pmpi_api_files = $(mpi_api_files:%=profile/p%)
+#pmpi_api_files = $(mpi_api_files:%=p%)


If you can figure out why this substitution isn't working, go ahead and uncomment it. Otherwise, it would probably be ok to delete this whole commented-out block.

ompi/include/mpi.h.in

ompi/mpi/c/Makefile.am

ompi/mpi/c/generate_bindings.py

jsquyres · 2024-02-07T15:54:24Z

ompi/mpi/c/generate_bindings.py

+    args = parser.parse_args()
+
+    # Always add the header
+    print('/* THIS FILE WAS AUTOGENERATED BY ompi/mpi/c/abi.py. DO NOT EDIT BY HAND. */')


Additionally, it might be better to have a traditional --outfile=FILENAME kind of CLI arg that indicates the name of the file that you want to write to. This leaves stdout available for info/verbose/debug kinds of output.

jsquyres · 2024-02-07T16:11:53Z

ompi/mpi/c/generate_bindings.py

+* Functions requiring a bigcount implementation should have type COUNT in
+  place of MPI_Count or int for each count parameter. Bigcount functions will
+  be generated automatically for any function that includes a COUNT type.
+"""


Please add a larger comment here at the top explaining the purpose of this script, and at least a high-level overview of what it does / how it works. This is a very large script; having an intro to it at the top would be most helpful to the reader (e.g., me).

jsquyres · 2024-02-07T16:15:30Z

Should #12033 be closed (if this PR wholly replaces it)?

jtronge · 2024-02-07T17:14:40Z

I think I'll try to remove the extra standard ABI code from the script, as well as the additional items you caught @jsquyres.

I don't think this code completely replaces PR #12033, especially since that one includes the refactor of libmpi into multiple libraries that we discussed.

config/ompi_setup_mpi_fortran.m4

docs/developers/bindings.rst

jtronge · 2024-02-14T22:44:16Z

/azp run

jsquyres · 2024-11-28T12:32:36Z

we don't intend to address the lower level datatype issues in this PR.

Oh... how is this PR correct, then? If we have datatype size mismatches, the code isn't going to be correct.

jsquyres

I made a bunch of nit-picky comments, some of which will be trivial to fix.

I think probably the only real substantial thing is the question about the compiler warnings / not attempting to address the changes needed by the underlying OMPI datatype functions. We'll need to figure that out -- we can't just ignore that.

ompi/mpi/fortran/use-mpi-f08/base/bigcount.h

docs/developers/bindings.rst

jsquyres · 2024-11-28T14:03:52Z

ompi/mpi/c/Makefile.am

@@ -57,436 +484,439 @@ ompi_HEADERS = $(headers)
 endif

 #
-# List of all C files that have profile versions
+# List of all C files that have profile versions (generated_*.c files were
+# generated from prototype_sources above).
 #
 interface_profile_sources = \


Not critical, but is it possible to generate interface_profile_sources from prototype_sources?

Is there a way to do that with automake?

@jsquyres i think this is the last open conversation. do we need to do something in this PR or can this wait till a followup?

docs/developers/bindings.rst

ompi/mpi/fortran/use-mpi-f08/mod/mpi-f08-interfaces.h.in

ompi/mpi/bindings/ompi_bindings/c_type.py

ompi/mpi/bindings/ompi_bindings/parser.py

ompi/mpi/bindings/ompi_bindings/c_type.py

jsquyres · 2024-11-28T15:15:00Z

ompi/mpi/bindings/ompi_bindings/c_type.py

+
+@Type.add_type('TYPE_COPY_ATTR_FUNCTION', abi_type=['standard'])
+class TypeTypeCopyAttrFunctionStandard(Type):
+    # TODO: This may require a special function to wrap the callback


All these attribute functions are still marked TODO

This is also for the standard ABI

bosilca

Deleting the old API files and creating new ones that are 95% identical instead of renaming and altering the renamed file drops access to past history of the MPI API support. What is the reason for taking this approach and is this reason compensating the losing history drawback ?

bosilca · 2024-12-06T20:55:56Z

ompi/mpi/fortran/base/fint_2_int.h

@@ -33,21 +33,22 @@
 */

 #if OMPI_SIZEOF_FORTRAN_INTEGER == SIZEOF_INT
-  #define OMPI_ARRAY_NAME_DECL(a) int *c_##a = NULL


The new macros are not doing anything.
How is a macro such as OMPI_ARRAY_FINT_2_INT_CLEANUP supposed to work now ? If I read this code correctly it will test if one of the arguments of the API, which is not allowed to be NULL by the standard, is NULL.

This is code that was pulled in from PR #10302, and indirectly from a branch by @ggouaillardet.

bosilca · 2024-12-06T21:01:49Z

ompi/mpi/fortran/use-mpi-f08/Makefile.am


 MOSTLYCLEANFILES = *.mod

 CLEANFILES += *.i90

 lib_LTLIBRARIES = lib@OMPI_LIBMPI_NAME@_usempif08.la
+noinst_LTLIBRARIES = lib@OMPI_LIBMPI_NAME@_usempif08_profile.la


I would like to see an answer to your previous question as well. It will be educative for all OMPI developers to understand how will the profiling layer generated and build.

ompi/mpi/fortran/use-mpi-f08/Makefile.prototype_files

hppritcha · 2024-12-16T17:20:35Z

Deleting the old API files and creating new ones that are 95% identical instead of renaming and altering the renamed file drops access to past history of the MPI API support. What is the reason for taking this approach and is this reason compensating the losing history drawback ?

actually one just needs to do

git log --follow name_of.c.in file

to get the history of a renamed file.

hppritcha · 2024-12-18T16:34:59Z

uh oh. nvidia needs to be more disks!

to take a count argument of type size_t. related to open-mpi#12226 related to open-mpi#9194 Signed-off-by: Howard Pritchard <howardp@lanl.gov>

related to open-mpi#12226 and open-mpi#9194 remove incorrect and misleading comment about ompi_3buff_op_reduce. See open-mpi#967 Signed-off-by: Howard Pritchard <howardp@lanl.gov>

This adds scripts for generating the C API bindings from template files, while also generating bigcount interfaces for those that require them. The binding script also include initial support for the mpi_f08 Fortran bindings, but doesn't yet make any changes to fortran/use-mpi-f08 Python >=3.6 is required for running these scripts, which is only necessary when the binding files have not already been generated. Users of the distribution tarball should not need to generate these files and thus should not require Python. Note that there are additional changes needed to Open MPI for bigcount support. One is that the datatype system needs to be embiggened. This PR includes workarounds for this lack of support that will need to be changed once the datatype system is embiggened. These areas of code are marked with TODO:BIGCOUNT. Co-authored-by: mphinney1100 <mphinney@lanl.gov> Co-authored-by: Howard Pritchard <hppritcha@gmail.com> Signed-off-by: Jake Tronge <jtronge@lanl.gov>

This updates fortran/use-mpi-f08 to generate most of the Fortran bindings from a script and template files. It also adds support for Fortran TS 29113 when possible, allowing for better Fortran array handling that matches the standard. The C files were imported from PR open-mpi#10302 and converted to templates to be fed into the binding script. Co-authored-by: Gilles Gouaillardet <gilles@rist.or.jp> Co-authored-by: Howard Pritchard <howardp@lanl.gov> Signed-off-by: Jake Tronge <jtronge@lanl.gov>

Signed-off-by: Jake Tronge <jtronge@lanl.gov>

github-actions bot added the Target: main label Jan 10, 2024

hppritcha requested a review from jsquyres January 10, 2024 21:18

jtronge force-pushed the bigcount branch from 1a9a108 to 37ada30 Compare January 11, 2024 15:29

jsquyres mentioned this pull request Jan 30, 2024

mpi_f08 configure tests failing for NAG #9795

Open

jsquyres reviewed Feb 7, 2024

View reviewed changes

jtronge mentioned this pull request Feb 7, 2024

Standard ABI generation code and library refactor. #12033

Draft

hppritcha reviewed Feb 8, 2024

View reviewed changes

config/ompi_setup_mpi_fortran.m4 Outdated Show resolved Hide resolved

hppritcha reviewed Feb 12, 2024

View reviewed changes

docs/developers/bindings.rst Outdated Show resolved Hide resolved

hppritcha mentioned this pull request Feb 14, 2024

COLL framework needs work to support MPI Bigcount #12336

Closed

jtronge force-pushed the bigcount branch from 2ae81f2 to cad1e4e Compare April 1, 2024 20:14

jtronge force-pushed the bigcount branch from 66909b4 to e00da70 Compare April 22, 2024 16:44

jsquyres mentioned this pull request Jul 13, 2024

Fortran: first try if type(*), dimension(..) works as-is #11591

Draft

jtronge force-pushed the bigcount branch 6 times, most recently from 8484636 to 90d6570 Compare August 1, 2024 15:07

jtronge force-pushed the bigcount branch 3 times, most recently from 6476640 to 569efa1 Compare August 12, 2024 18:46

jtronge force-pushed the bigcount branch from fa6f257 to 2811c1f Compare August 13, 2024 15:06

wenduwan added the Target: v6.0.x label Aug 13, 2024

github-actions bot removed the Target: v6.0.x label Aug 13, 2024

jtronge force-pushed the bigcount branch 2 times, most recently from 60ea322 to ab496c4 Compare August 15, 2024 21:31

open-mpi deleted a comment from github-actions bot Nov 28, 2024

jsquyres reviewed Nov 28, 2024

View reviewed changes

bosilca reviewed Dec 6, 2024

View reviewed changes

bosilca reviewed Dec 9, 2024

View reviewed changes

hppritcha force-pushed the bigcount branch 2 times, most recently from bfd0681 to cff5b03 Compare December 18, 2024 16:32

hppritcha added a commit to hppritcha/ompi that referenced this pull request Jan 2, 2025

ops: embiggen the ops framework

e3b9e44

to take a count argument of type size_t. related to open-mpi#12226 related to open-mpi#9194 Signed-off-by: Howard Pritchard <howardp@lanl.gov>

hppritcha added a commit to hppritcha/ompi that referenced this pull request Jan 2, 2025

ops: embiggen the ops framework

97c30f5

to take a count argument of type size_t. related to open-mpi#12226 related to open-mpi#9194 Signed-off-by: Howard Pritchard <howardp@lanl.gov>

hppritcha added a commit to hppritcha/ompi that referenced this pull request Jan 3, 2025

ops: embiggen the ops framework

b047013

to take a count argument of type size_t. related to open-mpi#12226 related to open-mpi#9194 Signed-off-by: Howard Pritchard <howardp@lanl.gov>

hppritcha mentioned this pull request Jan 3, 2025

ops: embiggen the ops framework #13015

Closed

hppritcha mentioned this pull request Jan 9, 2025

ops: add support for user-defined big count ops #13030

Merged

hppritcha force-pushed the bigcount branch from cff5b03 to 770f615 Compare January 22, 2025 19:54

jtronge and others added 3 commits February 3, 2025 08:42

Bigcount: Update docs and address other comments

08c7ab6

Signed-off-by: Jake Tronge <jtronge@lanl.gov>

hppritcha force-pushed the bigcount branch from 770f615 to 08c7ab6 Compare February 3, 2025 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate bigcount interfaces for Fortran and C #12226

Generate bigcount interfaces for Fortran and C #12226

jtronge commented Jan 10, 2024 •

edited by hppritcha

Loading

jsquyres left a comment

jsquyres Jan 30, 2024

jsquyres Feb 7, 2024

jsquyres Feb 7, 2024

jsquyres commented Feb 7, 2024

jtronge commented Feb 7, 2024

jtronge commented Feb 14, 2024

jsquyres commented Nov 28, 2024

jsquyres left a comment

jsquyres Nov 28, 2024

jtronge Dec 9, 2024

hppritcha Dec 18, 2024

jsquyres Nov 28, 2024

jtronge Dec 9, 2024

bosilca left a comment

bosilca Dec 6, 2024

jtronge Dec 9, 2024

bosilca Dec 6, 2024

hppritcha commented Dec 16, 2024

hppritcha commented Dec 18, 2024

Generate bigcount interfaces for Fortran and C #12226

Are you sure you want to change the base?

Generate bigcount interfaces for Fortran and C #12226

Conversation

jtronge commented Jan 10, 2024 • edited by hppritcha Loading

jsquyres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsquyres commented Feb 7, 2024

jtronge commented Feb 7, 2024

jtronge commented Feb 14, 2024

jsquyres commented Nov 28, 2024

jsquyres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bosilca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hppritcha commented Dec 16, 2024

hppritcha commented Dec 18, 2024

jtronge commented Jan 10, 2024 •

edited by hppritcha

Loading