Skip to content

ompi/attributes: revamp attribute handling. #3891

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

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

ggouaillardet
Copy link
Contributor

we now have 12 cases to deal (4 writers and 3 readers) :

  1. C void* is written into the attribute value, and the value is read into a C void* (unity)
  2. C void* is written, Fortran INTEGER is read
  3. C void* is written, Fortran INTEGER(KIND=MPI_ADDRESS_KIND) is read
  4. Fortran INTEGER is written, C void* is read
  5. Fortran INTEGER is written, Fortran INTEGER is read (unity)
  6. Fortran INTEGER is written, Fortran INTEGER(KIND=MPI_ADDRESS_KIND) is read
  7. Fortran INTEGER(KIND=MPI_ADDRESS_KIND) is written, C void* is read
  8. Fortran INTEGER(KIND=MPI_ADDRESS_KIND) is written, Fortran INTEGER is read
  9. Fortran INTEGER(KIND=MPI_ADDRESS_KIND) is written, Fortran INTEGER(KIND=MPI_ADDRESS_KIND) is read (unity)
  10. Intrinsic is written, C void* is read
  11. Intrinsic is written, Fortran INTEGER is read
  12. Intrinsic is written, Fortran INTEGER(KIND=MPI_ADDRESS_KIND) is read

MPI-2 Fortran "integer representation" has type INTEGER(KIND=MPI_ADDRESS_KIND) as clarified
at mpiwg-rma/rma-issues#1

Signed-off-by: Gilles Gouaillardet [email protected]

(cherry picked from commit 72cfbb6)

@ggouaillardet
Copy link
Contributor Author

@bwbarrett

from #2940

the changes involves the prototype of a function pointer.
so there are several ways to see this.

  1. this breaks ABI
  2. this does not break ABI (the pointer size did not change)
  3. current implementation is broken anyway, so this really a bug fix, and since the pointer size did not change, this is an acceptable commit

i'd rather go with the latter option.

i set the milestone to v3.0.0, but if it is too big of a change at this time and you are willing to accept this for v3.0.1, then feel free to bump the milestone accordingly

@bwbarrett
Copy link
Member

@ggouaillardet, your commit message leaves a lot to be desired in the "why?" category. What was broken, what's fixed, what's still broken? What's your opinion on the ABI changes and why?

We can't take an ABI change for 3.0.1 and would prefer not take it for the fall release, but it's unclear where we even are on ABI changes.

@hppritcha hppritcha requested review from jjhursey and removed request for jsquyres July 17, 2017 19:11
@hppritcha hppritcha assigned jjhursey and unassigned jsquyres Jul 17, 2017
@hppritcha
Copy link
Member

assigning to @jjhursey since @jsquyres is on vacation this week and Josh is our new fortran expert.

we now have 12 cases to deal (4 writers and 3 readers) :

1. C `void*` is written into the attribute value, and the value is read into a C `void*` (unity)
2. C `void*` is written, Fortran `INTEGER` is read
3. C `void*` is written, Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)` is read
4. Fortran `INTEGER` is written, C `void*` is read
5. Fortran `INTEGER` is written, Fortran `INTEGER` is read (unity)
6. Fortran `INTEGER` is written, Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)` is read
7. Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)` is written, C `void*` is read
8. Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)` is written, Fortran `INTEGER` is read
9. Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)` is written, Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)` is read (unity)
10. Intrinsic is written, C `void*` is read
11. Intrinsic is written, Fortran `INTEGER` is read
12. Intrinsic is written, Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)` is read

We previously handled 9 cases ( 3 x 3 types) :

a) C `void *`
b) Fortran MPI-1
c) Fortran MPI-2

though a) is unchanged and c) is equivalent to Fortran `INTEGER(KIND=MPI_ADDRESS_KIND)`,
b) was not correctly handled since a Fortran `INTEGER` is not always equivalent to a C `int`.
though it might have happened to work on little endian systems, the change is critical for big endian systems.

MPI-2 Fortran "integer representation" has type `INTEGER(KIND=MPI_ADDRESS_KIND)` as clarified
at mpiwg-rma/rma-issues#1

Signed-off-by: Gilles Gouaillardet <[email protected]>

(cherry picked from commit open-mpi/ompi@72cfbb6)
@ggouaillardet ggouaillardet force-pushed the topic/v3.0.x/attributes branch from 197a3f6 to 3b2df6d Compare July 18, 2017 00:19
@ggouaillardet
Copy link
Contributor Author

@bwbarrett i rewrote the commit message to explain what this commit is fixing

@bosilca i checked again about the ABI changes, and unless i got it wrong (again), there are no ABI changes :-)

the type of the callbacks for MPI_Comm_create_keyval(), MPI_Create_keyval(), MPI_Type_create_keyval() and MPI_Win_create_keyval() was modified in ompi/mpi/fortran/mpif-h/prototypes_mpi.h, but this is just a name change and the callback signature was unmodified.
so i do not think anymore there is any ABI change in this PR

Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

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

I don't see any ABI issues, from my review. I think this is good to go. Thanks!

@hppritcha
Copy link
Member

@bwbarrett good to go

@hppritcha hppritcha merged commit fd53174 into open-mpi:v3.0.x Jul 18, 2017
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