Skip to content

Remove C-Wrapper for MPI_Init #16

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 21, 2025
Merged

Remove C-Wrapper for MPI_Init #16

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a782b7
Remove C-Wrapper for MPI_Init
adit4443ya Mar 18, 2025
63c8915
Test : Add MPI_Init Test
adit4443ya Mar 18, 2025
9ffdcd5
remove 'value' attribute from mpi_c_bindings.f90 function's arguments
gxyd Mar 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/mpi.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,18 +113,21 @@ module mpi

subroutine MPI_Init_proc(ierr)
use mpi_c_bindings, only: c_mpi_init
use iso_c_binding, only : c_int
use iso_c_binding, only : c_int, c_ptr, c_null_ptr
integer, optional, intent(out) :: ierr
integer :: local_ierr
integer(c_int) :: local_ierr
integer(c_int) :: argc
type(c_ptr) :: argv = c_null_ptr
argc = 0
! Call C MPI_Init directly with argc=0, argv=NULL
local_ierr = c_mpi_init(argc, argv)

if (present(ierr)) then
call c_mpi_init(ierr)
else
call c_mpi_init(local_ierr)
if (local_ierr /= 0) then
print *, "MPI_Init failed with error code: ", local_ierr
end if
ierr = int(local_ierr)
else if (local_ierr /= 0) then
print *, "MPI_Init failed with error code: ", local_ierr
end if
end subroutine
end subroutine MPI_Init_proc

subroutine MPI_Init_thread_proc(required, provided, ierr)
use mpi_c_bindings, only : c_mpi_init_thread
Expand Down
10 changes: 6 additions & 4 deletions src/mpi_c_bindings.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ module mpi_c_bindings
implicit none

interface
subroutine c_mpi_init(ierr) bind(C, name="mpi_init_wrapper")
use iso_c_binding, only: c_int
integer(c_int), intent(out) :: ierr
end subroutine c_mpi_init
function c_mpi_init(argc, argv) bind(C, name="MPI_Init")
use iso_c_binding, only : c_int, c_ptr
integer(c_int), value :: argc
type(c_ptr), value :: argv
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 both the arguments have to be pass-by-value here? What if I keep it intent(inout)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to see an explanation for why this needs to be passed with value attribute with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that passing pointers/reference would make it complex , Hence to keep it safe i used value
Although i didn't checked with intent(inout)
So this is the current situation

Wihout value attribute all tests passes locally
with intent(inout) lfortran fails
with this error

code generation error: asr_to_llvm: module failed verification. Error:
Call parameter type does not match function signature!
  %argv = alloca void*, align 8
 void*  %0 = call i32 @MPI_Init(i32* %argc, void** %argv)

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, I tried locally, and when no intent is specified, then ./run_tests.sh passes with both LFortran and GFortran.

While, when intent(inout) is specified, then LFortran fails, but GFortran succeeds. By default, when no intent is specified, then the intent is actually intent(inout) I think.

So, there is anyways a bug in LFortran, which should be extracted and reported against LFortran.

I thought that passing pointers/reference would make it complex

I don't really understand this. @certik any thoughts on this?

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 reading: https://docs.open-mpi.org/en/v5.0.x/man-openmpi/man3/MPI_Init.3.html#description, which says:

All MPI programs must contain a call to MPI_Init or MPI_Init_thread. Open MPI accepts the C argc and argv arguments to main, but neither modifies, interprets, nor distributes them:

Copy link
Collaborator Author

@adit4443ya adit4443ya Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's Do one thing
We know that LFortran gives error in the intent(inout) declaration

So for now let me create an MRE and open an issue on LFortran and till then we would use value parameter,
Once that issue is resolved we will remove this workaround and things won't get blocked.

Here is the MRE:-lfortran/lfortran#6696

What's your view @gxyd

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me create issue in this repo too so that we do not skip it
Here:- #27

Copy link
Collaborator

@gxyd gxyd Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While, when intent(inout) is specified, then LFortran fails, but GFortran succeeds. By default, when no intent is specified, then the intent is actually intent(inout) I think.

Like I mentioned here, LFortran doesn't fail when no intent is specified, so we still need to answer, whether value is the correct way or not specifying any intent at all.

Copy link
Collaborator Author

@adit4443ya adit4443ya Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I guess maybe we need to look at any documentation for what to use
My intuition is not to use value or intent at all, (also i used GROK to do some research and the first thing it told me is not to use it)
maybe i read it somewhere, but i did not found the exact line stating that which parameter is ideal enough.

As with the value approach we might add an extra instruction of storing and then loading
Also i referred this https://gcc.gnu.org/onlinedocs/gfortran/Interoperable-Subroutines-and-Functions.html
It does not use intent(),
Also intent should not affect the passing of the objects at the runtime right?
Hence that issue iss the one which we have to fix at lfortran, but what i feel what ideal to do is not to use anything at all, until unless something comes up again with similar argument

Copy link
Collaborator

Choose a reason for hiding this comment

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

As with the value approach we might add an extra instruction of storing and then loading
Also i referred this https://gcc.gnu.org/onlinedocs/gfortran/Interoperable-Subroutines-and-Functions.html
It does not use intent()

This is good, it makes it clear that we don't need value attribute specified here.

integer(c_int) :: c_mpi_init
end function c_mpi_init

subroutine c_mpi_init_thread(required, provided, ierr) bind(C, name="mpi_init_thread_wrapper")
use iso_c_binding, only: c_int
Expand Down
10 changes: 5 additions & 5 deletions src/mpi_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@

#define MPI_STATUS_SIZE 5

void mpi_init_wrapper(int *ierr) {
int argc = 0;
char **argv = NULL;
*ierr = MPI_Init(&argc, &argv);
}
// void mpi_init_wrapper(int *ierr) {
// int argc = 0;
// char **argv = NULL;
// *ierr = MPI_Init(&argc, &argv);
// }

void mpi_init_thread_wrapper(int *required, int *provided, int *ierr) {
int argc = 0;
Expand Down
36 changes: 36 additions & 0 deletions tests/init_1.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
program test_mpi_init_rank_size
use mpi
implicit none
integer :: ierr, rank, size

! Initialize MPI
call MPI_Init(ierr)
if (ierr /= MPI_SUCCESS) then
print *, "MPI_Init failed with error code: ", ierr
stop 1
end if

! Get rank and size
call MPI_Comm_rank(MPI_COMM_WORLD, rank, ierr)
if (ierr /= MPI_SUCCESS) then
print *, "MPI_Comm_rank failed with error code: ", ierr
call MPI_Finalize(ierr)
stop 1
end if

call MPI_Comm_size(MPI_COMM_WORLD, size, ierr)
if (ierr /= MPI_SUCCESS) then
print *, "MPI_Comm_size failed with error code: ", ierr
call MPI_Finalize(ierr)
stop 1
end if

print *, "Hello from rank ", rank, " of ", size

! Finalize MPI
call MPI_Finalize(ierr)
if (ierr /= MPI_SUCCESS) then
print *, "MPI_Finalize failed with error code: ", ierr
stop 1
end if
end program test_mpi_init_rank_size