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

Intended way to generate MPI.jl compatible C bindings #667

Open
termi-official opened this issue Oct 4, 2022 · 13 comments
Open

Intended way to generate MPI.jl compatible C bindings #667

termi-official opened this issue Oct 4, 2022 · 13 comments

Comments

@termi-official
Copy link
Contributor

I am currently trying to generate MPI.jl compatible bindings of the C library HYPRE with the help Clang.jl . The current generator (found here https://github.com/termi-official/HYPRE.jl/blob/master/gen/generator.jl) creates files that have incompatible data types (see e.g. https://github.com/termi-official/HYPRE.jl/blob/master/lib/LibHYPRE.jl#L6), so I was wondering what the intended way to grab the correct MPI headers and data type definitions is?

cc @fredrikekre @Gnimuc

@giordano
Copy link
Member

giordano commented Oct 4, 2022

Can gen/src/MPIgenerator.jl be of inspiration? That script is used to generate the new low-level API.

@fredrikekre
Copy link
Contributor

(Slightly tangential, I am curious why there is a mismatch between

const MPI_Comm = UInt32
and https://github.com/fredrikekre/HYPRE.jl/blob/50c06161e9b654c5095dffd6a6b6e0978f1ae851/lib/LibHYPRE.jl#L6 since they are both (apparently) generated based on MPICH headers)

@termi-official
Copy link
Contributor Author

termi-official commented Oct 4, 2022

Thanks for the quick answer and your suggestion. I already tried to use this one as a template.

diff --git a/gen/generator.jl b/gen/generator.jl
index d6880cb..3df8f19 100644
--- a/gen/generator.jl
+++ b/gen/generator.jl
@@ -1,10 +1,18 @@
 using Clang.Generators
-using HYPRE_jll, MPICH_jll
+using HYPRE_jll, MPIPreferences
 
 cd(@__DIR__)
 
+if MPIPreferences.binary == "MPICH_jll"
+    import MPICH_jll: artifact_dir
+elseif MPIPreferences.binary == "OpenMPI_jll"
+    import OpenMPI_jll: artifact_dir
+else
+    error("Unknown MPI binary: $(MPIPreferences.binary)")
+end
+
 hypre_include_dir = normpath(HYPRE_jll.artifact_dir, "include")
-mpi_include_dir = normpath(MPICH_jll.artifact_dir, "include")
+mpi_include_dir = normpath(artifact_dir, "include")
 
 options = load_options(joinpath(@__DIR__, "generator.toml"))

but the types are still not matching - and we generate quite a bit of redundant code which is readily available in MPI.jl.

@giordano
Copy link
Member

giordano commented Oct 4, 2022

(Slightly tangential, I am curious why there is a mismatch between

const MPI_Comm = UInt32
and https://github.com/fredrikekre/HYPRE.jl/blob/50c06161e9b654c5095dffd6a6b6e0978f1ae851/lib/LibHYPRE.jl#L6 since they are both (apparently) generated based on MPICH headers)

I'm not entirely sure that file was automatically generated from header files,

@vchuravy
Copy link
Member

vchuravy commented Oct 4, 2022

This could maybe be of inspiration https://github.com/cesmix-mit/LAMMPS.jl/tree/main/res

@simonbyrne
Copy link
Member

simonbyrne commented Oct 4, 2022

I'm not entirely sure that file was automatically generated from header files,

It wasn't, that was manually translated.

I don't think it matters which one we use. I'm not sure why I changed them to unsigned? Perhaps changing them back to Cint would make things easier.

@termi-official
Copy link
Contributor Author

This could maybe be of inspiration https://github.com/cesmix-mit/LAMMPS.jl/tree/main/res

Sorry to ask again, but how exactly is the variable mpi_header_dir (https://github.com/cesmix-mit/LAMMPS.jl/blob/main/res/wrap.jl#L13) resolved? I could not find it in MPI.jl, LAMMPS_jll or Clang.jl .

@fredrikekre
Copy link
Contributor

fredrikekre commented Oct 4, 2022

I don't think it matters which one we use. I'm not sure why I changed them to unsigned? Perhaps changing them back to Cint would make things easier.

Okay, I ran into MethodError: Cannot convert an object of type MPI.Comm to an object of type Int32 when ccalling a function generated with Clang (based on the MPICH header) which then had Cint for the argument, and cconvert(::Type, ::MPI.Comm) fails. Perhaps

Base.cconvert(::Type{MPI_Comm}, comm::Comm) = comm
could be relaxed a bit? Probably that could be

Base.cconvert(::Type{T}, comm::Comm) where {T <: Integer} = convert(T, comm.val)

?

@simonbyrne
Copy link
Member

Lets just change them to signed: that's what they are in the headers.

@Gnimuc
Copy link
Contributor

Gnimuc commented Oct 5, 2022

Basically, we can reuse symbols from MPI by treating MPI headers as system headers.

Change this line:

push!(args, "-I$(mpi_include_dir)")

to

push!(args, "-isystem$(mpi_include_dir)")

@Gnimuc
Copy link
Contributor

Gnimuc commented Oct 5, 2022

In this way, all useless symbols under mpi_include_dir will be ignored, so symbols from MPI.jl can be added on demand and you get no conflicts.

@termi-official
Copy link
Contributor Author

Basically, we can reuse symbols from MPI by treating MPI headers as system headers.

Change this line:

push!(args, "-I$(mpi_include_dir)")

to

push!(args, "-isystem$(mpi_include_dir)")

Exactly what I was looking for - thanks!

Now, if I am understanding correctly, then Binary Builder ensures that an artifact matching the MPI backend selected in MPIPreferences is selected, right?

@simonbyrne
Copy link
Member

I've changed the MPICH and Microsoft MPI handles to now be signed ints (#668), I'll tag a new patch release.

It would be great to turn this discussion into some docs.

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

No branches or pull requests

6 participants