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

Standard ABI generation code and library refactor. #12033

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jtronge
Copy link
Contributor

@jtronge jtronge commented Oct 30, 2023

This adds a script for generating C ABI functions, as well as bigcount interfaces, based on template files for each function. Standard ABI functions are linked into a new libmpi_abi.la and backend code that was originally in libmpi.la is now stored in libopen-mpi.la. There are still many symbols missing from the standard ABI, so it's likely that more complicated code will not compile.

@github-actions
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

f12c656: Add initial ABI generation code and new libraries

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@jtronge jtronge force-pushed the abi-generate branch 6 times, most recently from 20c3e2d to 6ad2350 Compare November 2, 2023 16:13
Two external MPI libraries are now created: libmpi.la and libmpi_abi.la.
Backend code that was originally in libmpi.la has been extracted into
libopen-mpi.la to be linked into both libraries.

Parts of the Open MPI C interface are now being generated by a python
script (abi.py) from modified source files (named with *.in). This
script generates files for both the ompi ABI and the standard ABI from
the same source file, also including new bigcount interfaces.

To compile standard ABI code, there's a new mpicc_abi compiler wrapper.
The standard ABI does not yet include all functions or symbols, so more
complicated source files will not compile. ROMIO must be disabled for
the code to link, since it's relying on the external MPI interface.

Signed-off-by: Jake Tronge <[email protected]>
@ggouaillardet
Copy link
Contributor

There is an issue in VPATH mode, and here is a possible fix (probably worth rewriting for style...)

diff --git a/ompi/mpi/c/Makefile.am b/ompi/mpi/c/Makefile.am
index b50fa7d1f9..f665b8bbbd 100644
--- a/ompi/mpi/c/Makefile.am
+++ b/ompi/mpi/c/Makefile.am
@@ -598,9 +598,9 @@ ompi_%.c: %.c.in abi.py abi.h
 standard_%.c: %.c.in abi.py abi.h
 	$(srcdir)/abi.py source standard $< > $@
 abi.h: abi.py $(prototype_sources)
-	$(srcdir)/abi.py header $(prototype_sources) > $@
+	$(srcdir)/abi.py header --srcdir=$(srcdir)/ $(prototype_sources) > $@
 
 standard_abi/mpi.h: abi.py $(prototype_sources)
-	mkdir -p $(srcdir)/standard_abi
-	$(srcdir)/abi.py header --external $(prototype_sources) > $@
+	mkdir -p standard_abi
+	$(srcdir)/abi.py header --srcdir=$(srcdir)/ --external $(prototype_sources) > $@
 endif
diff --git a/ompi/mpi/c/abi.py b/ompi/mpi/c/abi.py
index d06a2770e1..693cd7adcd 100755
--- a/ompi/mpi/c/abi.py
+++ b/ompi/mpi/c/abi.py
@@ -1,5 +1,7 @@
 #!/usr/bin/env python3
-# Copyright (c) 2023    Triad National Security, LLC. All rights reserved.
+# Copyright (c) 2023      Triad National Security, LLC. All rights reserved.
+# Copyright (c) 2023      Research Organization for Information Science
+#                         and Technology (RIST).  All rights reserved.
 # $COPYRIGHT$
 #
 # Additional copyrights may follow
@@ -1051,9 +1053,9 @@ class SourceTemplate:
         self.body = body
 
     @staticmethod
-    def load(fname):
+    def load(fname, prefix=""):
         """Load a template file and return the SourceTemplate."""
-        with open(fname) as fp:
+        with open(prefix + fname) as fp:
             header = []
             prototype = []
             body = []
@@ -1190,7 +1192,7 @@ def standard_abi(base_name, template):
 
 def gen_header(args):
     """Generate an ABI header and conversion code."""
-    prototypes = [SourceTemplate.load(file_).prototype for file_ in args.file]
+    prototypes = [SourceTemplate.load(file_, args.srcdir).prototype for file_ in args.file]
 
     builder = ABIHeaderBuilder(prototypes, external=args.external)
     builder.dump_header()
@@ -1219,6 +1221,7 @@ def main():
     parser_header = subparsers.add_parser('header')
     parser_header.add_argument('file', nargs='+', help='list of template source files')
     parser_header.add_argument('--external', action='store_true', help='generate external mpi.h header file')
+    parser_header.add_argument('--srcdir', help='source directory')
     parser_header.set_defaults(func=gen_header)
 
     parser_gen = subparsers.add_parser('source')

@ggouaillardet
Copy link
Contributor

@bwbarrett (one) CI failure was caused because python 3.8 is now a requirement and is not installed on an EC2 instance. Could you please take care of that?

@jsquyres
Copy link
Member

jsquyres commented Nov 6, 2023

@jtronge Now that I understand that abi.py is written by you (and not the Forum), is it possible to write it in a way to be compatible with Python 2 and Python 3?

@hppritcha
Copy link
Member

I don't like that idea. python2 has been deprecated for years.

@hppritcha
Copy link
Member

maintaining a script written in python2 would be a lot of overhead.

@jsquyres
Copy link
Member

jsquyres commented Nov 6, 2023

maintaining a script written in python2 would be a lot of overhead.

It may not be all that much overhead; we have other .py scripts in the tree that are both Py2- and Py3-compatible. That being said, I won't argue for this -- I was just asking about the possibility. As long as the tarball ships all the generated bindings and users won't run abi.py by default, it's fine. My only real concern is RHEL 7/CentOS 7, where Python 3 is not the default. But they're dying next year, so this is probably all much ado about nothing.

@bwbarrett
Copy link
Member

maintaining a script written in python2 would be a lot of overhead.

It may not be all that much overhead; we have other .py scripts in the tree that are both Py2- and Py3-compatible. That being said, I won't argue for this -- I was just asking about the possibility. As long as the tarball ships all the generated bindings and users won't run abi.py by default, it's fine. My only real concern is RHEL 7/CentOS 7, where Python 3 is not the default. But they're dying next year, so this is probably all much ado about nothing.

I've been under water for the last week; I'll hopefully have time tomorrow to update the AMIs to include Python3 and we can move on.

@jtronge
Copy link
Contributor Author

jtronge commented Nov 6, 2023

Thanks @ggouaillardet for the suggestion. I think my most recent commit should allow for VPATH builds.

@jsquyres I've lowered the Python version to 3.6 and tested it locally. I'm guessing a conversion from 3-2 might require a restructure of some parts of the script and could introduce bugs.

Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

38209b6: Add inline attribute to generated functions

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@jtronge
Copy link
Contributor Author

jtronge commented Nov 30, 2023

Our script is generating static inline functions for each file. After adding __opal_attribute_always_inline__ and testing with clang -Rpass=inline ... it looks like these are in fact getting inlined:

standard_send.c:112:17: remark: 'ompi_abi_send' inlined into 'MPI_Send_c' with (cost=always): always inline attribute at callsite MPI_Send_c:5:17; [-Rpass=inline]
    ret_value = ompi_abi_send(buf, count, type_tmp, dest, tag, comm_tmp);

If the ABI source files have not been generated, then Python >=3.6 is
required. ABI source file generation is turned on by default as long as
the proper Python is found. The standard ABI library will only be built
with '--enable-standard-abi'.

Signed-off-by: Jake Tronge <[email protected]>
@jtronge
Copy link
Contributor Author

jtronge commented Feb 7, 2024

This PR is on hold for the moment. We're waiting on a more complete definition of the Standard ABI as well as PR #12226, which adds bigcount support for both C and Fortran. I will be pushing a README shortly describing some of the design decisions that we discussed previously.

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

Successfully merging this pull request may close these issues.

5 participants