Skip to content

osc/rdma: fix datatype extent usage in ompi_osc_rdma_rget_accumulate_… #3572

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

Jump to bottom
Merged
merged 1 commit into from
May 26, 2017
Merged

osc/rdma: fix datatype extent usage in ompi_osc_rdma_rget_accumulate_… #3572

merged 1 commit into from
May 26, 2017

Conversation

ggouaillardet
Copy link
Contributor

…internal()

Refs #3569

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

@ggouaillardet ggouaillardet requested a review from hjelmn May 24, 2017 06:02
@ggouaillardet
Copy link
Contributor Author

@hjelmn could you please double check this before i commit ?

i am confident this fixes the issue reported in #3569 but not so sure the extent is correctly handled if module->acc_single_intrinsic

bwbarrett
bwbarrett requested changes May 24, 2017
View reviewed changes
Copy link
Member

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

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

I'd really like to see the "why" for this change, either in a comment in the code or in the description of the commit. "Fix datatype extent usage" isn't really helpful to the next developer. What's the fix and what was broken?

I think it would be much more clear if you renamed extent to origin_extent and target_extent through the function; it's a subtle change you should highlight more.

@bwbarrett bwbarrett mentioned this pull request May 24, 2017
Closed
@ggouaillardet ggouaillardet force-pushed the topic/ompi_osc_rdma_rget_accumulate_internal branch from 9be9094 to 693fa18 Compare May 25, 2017 00:49
@ggouaillardet
Copy link
Contributor Author

@bwbarrett fair enough, i updated both the patch and commit message

@ibm-ompi
Copy link

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/ca23fd28bf3b17c00d42663723c42860

@ibm-ompi
Copy link

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/e0a7590976439674086250d53cd264de

@ibm-ompi
Copy link

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/223798cfbdf6845fb92c8d56848e9d0d

@ggouaillardet ggouaillardet force-pushed the topic/ompi_osc_rdma_rget_accumulate_internal branch from 693fa18 to 23b9b24 Compare May 25, 2017 01:40
@ggouaillardet
Copy link
Contributor Author

:bot:mellanox:retest

bwbarrett
bwbarrett approved these changes May 25, 2017
View reviewed changes
Copy link
Member

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

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

typo in your commit message. Should be "use either origin_extent or target_extent" ("or" instead of "of")

@markalle markalle mentioned this pull request May 25, 2017
Closed
@ggouaillardet
osc/rdma: use extent of the appropriate datatype in ompi_osc_rdma_rge…
0f79259 
…t_accumulate_internal()

origin_datatype and target_datatype might be different and hence have different extent,
so use either origin_extent or target_extent when appropriate.

Refs open-mpi#3569

Signed-off-by: Gilles Gouaillardet <[email protected]>
@ggouaillardet ggouaillardet force-pushed the topic/ompi_osc_rdma_rget_accumulate_internal branch from 23b9b24 to 0f79259 Compare May 26, 2017 05:00
hjelmn
hjelmn approved these changes May 26, 2017
View reviewed changes
Copy link
Member

@hjelmn hjelmn left a comment

Choose a reason for hiding this comment

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

Thanks for catching this typo.

@hjelmn
Copy link
Member

hjelmn commented May 26, 2017

Please PR to v3.0, v2.x, and v2.0.x.

@hjelmn hjelmn merged commit 7d5cc8e into open-mpi:master May 26, 2017
@ggouaillardet ggouaillardet mentioned this pull request May 29, 2017
Merged
@ggouaillardet
Copy link
Contributor Author

@hjelmn i made PRs for v3.x and v2.x (v2.0.x is a bit different)

while doing that, i found the fix was not quite right, so i made #3600
can you please review it ?
then i will cherry-pick the commit into both PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjelmn hjelmn hjelmn approved these changes

@bwbarrett bwbarrett bwbarrett approved these changes

Assignees

@hjelmn hjelmn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ggouaillardet @ibm-ompi @hjelmn @bwbarrett