ompi icon indicating copy to clipboard operation
ompi copied to clipboard

Standard ABI generation code and library refactor.

Open jtronge opened this issue 2 years ago • 12 comments

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.

jtronge avatar Oct 30 '23 17:10 jtronge

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

f12c6565: 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!

github-actions[bot] avatar Oct 30 '23 17:10 github-actions[bot]

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 avatar Nov 04 '23 03:11 ggouaillardet

@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?

ggouaillardet avatar Nov 04 '23 07:11 ggouaillardet

@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?

jsquyres avatar Nov 06 '23 21:11 jsquyres

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

hppritcha avatar Nov 06 '23 22:11 hppritcha

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

hppritcha avatar Nov 06 '23 22:11 hppritcha

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.

jsquyres avatar Nov 06 '23 22:11 jsquyres

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.

bwbarrett avatar Nov 06 '23 22:11 bwbarrett

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.

jtronge avatar Nov 06 '23 23:11 jtronge

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

38209b68: 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!

github-actions[bot] avatar Nov 28 '23 21:11 github-actions[bot]

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);

jtronge avatar Nov 30 '23 18:11 jtronge

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.

jtronge avatar Feb 07 '24 23:02 jtronge