-
Notifications
You must be signed in to change notification settings - Fork 281
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
romio: implement mpi-4 large count functions #5278
Conversation
73630d9
to
2ccc291
Compare
test:mpich/ch3/most |
The freebsd failure is obviously not related, but we need to investigate the cause at some time.
test:mpich/ch3/sock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! MPICH used to have a policy against assert and I am glad to see we are rethinking that stance.
Not embracing it, but better than not checking it. We need to turn the assertion into proper error checking at some point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, this is fine. No one uses this code so it's hard to test aggressively, but it's also unlikely anyone will report any bugs (if there are any: it looks correct to me!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"this code" meant the datatype representation code. I thought github would let me comment on individual changesets
src/mpi/romio/mpi-io/get_extent.c
Outdated
/* FIXME: handle other file data representations */ | ||
|
||
MPI_Aint extent_i; | ||
error_code = PMPI_Type_extent(datatype, &extent_i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be PMPI_Type_get_extent since MPI_Type_extent is deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the last commit replaced the deprecated MPI_Type_extent
.
|
||
return error_code; | ||
} | ||
|
||
/* prevent multiple definitions of this routine */ | ||
#ifdef MPIO_BUILD_PROFILING | ||
int MPIOI_File_iread(MPI_File fh, MPI_Offset offset, int file_ptr_type, void *buf, int count, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to bump the 'count' here to a 64 bit type, too -- which I warn you will cascade down into a lot of ROMIO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'here' and all the other 'MPIOI_File_...' internal implemenations.
OK, I see in your merge request description that you plan to bump up the internal type in a subsequent pull request. Then great: no harm in landing this request -- large counts are no worse than existing code. |
test:mpich/ch4/most |
MPI_Count is now needed as a paprameter type for romio functions.
Declare the MPI-4 large count IO functions.
Poly functions will have both the regular kind and large count kind. Refator into MPIOI internal routine to prevent duplications.
MPI_Register_datarep and MPI_Register_datarep_c differ in the types of conversion function. Since we don't actually support non-NULL conversion functions, the only difference is in the parameter types.
In future PR, we'll make internal count using MPI_Aint. For now, we simply assert the values to fit in a int, and let compiler convert MPI_Count into int.
Replace the usage of deprecated MPI_Type_extent.
Fixes #4880 |
Pull Request Description
Add large count functions:
Most of the functions only has one scalar parameters that is
int count
in small function, andMPI_Count count
in the large function. This PR simply adds an assertion to ensure the large count can fit in theint
and rely on compiler to do the integer conversion.TODO in a future PR -- make internal functions to use
MPI_Aint
forcount
.Author Checklist
Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
Commits are self-contained and do not do two things at once.
Commit message is of the form:
module: short description
Commit message explains what's in the commit.
Whitespace checker. Warnings test. Additional tests via comments.
For non-Argonne authors, check contribution agreement.
If necessary, request an explicit comment from your companies PR approval manager.