Skip to content
This repository was archived by the owner on Sep 30, 2022. It is now read-only.

mpi/c: Add each check for count==0 in nonblocking reduce interface #1254

Merged
merged 1 commit into from
Jul 19, 2016
Merged

mpi/c: Add each check for count==0 in nonblocking reduce interface #1254

merged 1 commit into from
Jul 19, 2016

Conversation

jjhursey
Copy link
Member

@jjhursey jjhursey commented Jul 1, 2016

  • Matches the blocking versions of these interfaces
    • iallreduce.c to match allreduce.c
    • ireduce.c to match reduce.c
    • ireduce_scatter.c to match reduce_scatter.c
  • Workaround for IMB-NBC benchmark, similar to the workaround
    in place for the IMB-MPI1 benchmark for the blocking collectives.

(cherry picked from commit open-mpi/ompi@96779f6)
open-mpi/ompi#1836

 * Matches the blocking versions of these interfaces
   - `iallreduce.c` to match `allreduce.c`
   - `ireduce.c` to match `reduce.c`
   - `ireduce_scatter.c` to match `reduce_scatter.c`
 * Workaround for IMB-NBC benchmark, similar to the workaround
   in place for the IMB-MPI1 benchmark for the blocking collectives.

(cherry picked from commit open-mpi/ompi@96779f6)
@jjhursey
Copy link
Member Author

jjhursey commented Jul 1, 2016

bot:label:Code-cleanup-low-priority
bot:label:bug
bot:milestone:v2.0.1

@ibm-ompi
Copy link

ibm-ompi commented Jul 1, 2016

Build Failed with XL compiler! Please review the log, and get in touch if you have questions.

Gist: https://gist.github.com/70410b08e5c2b9aaeb8c98c5af30d562

@ibm-ompi
Copy link

ibm-ompi commented Jul 1, 2016

Build Failed with GNU compiler! Please review the log, and get in touch if you have questions.

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

@mellanox-github
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1828/ for details.

@jjhursey
Copy link
Member Author

jjhursey commented Jul 5, 2016

bot:ibm:retest

@jjhursey
Copy link
Member Author

bot:retest

@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1845/ for details.

@jjhursey
Copy link
Member Author

@jsquyres can you review? It looks like you put in the original protections for the blocking versions:

bot:assign: @jsquyres

@@ -93,6 +94,15 @@ int MPI_Iallreduce(const void *sendbuf, void *recvbuf, int count,
OMPI_ERRHANDLER_CHECK(err, comm, err, FUNC_NAME);
}

/* MPI standard says that reductions have to have a count of at least 1,
* but some benchmarks (e.g., IMB) calls this function with a count of 0.
Copy link
Member

Choose a reason for hiding this comment

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

Nit pick: IMB doesn't call any non-blocking collectives.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it does in the latest version - IMB-NBC

Choose a reason for hiding this comment

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

Oh! Do we have that in ompi tests?

Sent from my phone. No type good.

On Jul 19, 2016, at 12:05 PM, Josh Hursey <[email protected]mailto:[email protected]> wrote:

In ompi/mpi/c/iallreduce.chttps://github.com//pull/1254#discussion_r71369569:

@@ -93,6 +94,15 @@ int MPI_Iallreduce(const void *sendbuf, void *recvbuf, int count,
OMPI_ERRHANDLER_CHECK(err, comm, err, FUNC_NAME);
}

  • /* MPI standard says that reductions have to have a count of at least 1,
  • \* but some benchmarks (e.g., IMB) calls this function with a count of 0.
    

Actually it does in the latest version - IMB-NBC

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1254/files/1d8870db477abf3dfd89581ef581ef82a3b1df99#r71369569, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AKRgd7PFQFsQiGUEUB_cVgOnyg_8jtM6ks5qXPWegaJpZM4JDeja.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I ompi-tests has the 3.2.4. The latest is 4.1, which contains RMA and NBC benchmarks - might be good to add to that repo, and make an MTT section for it.

@jsquyres
Copy link
Member

Other than the comment nit-pick, 👍

bot:retest

@mellanox-github
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1949/ for details.

@jsquyres
Copy link
Member

Mellanox failure is unrelated. Per discussion on the call today, @artpol84 is investigating.

@jsquyres jsquyres merged commit 7728484 into open-mpi:v2.x Jul 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants