From fd5e307528fafab5e936b974d800c36562fba5fb Mon Sep 17 00:00:00 2001 From: Berk Hess Date: Mon, 7 May 2018 16:09:38 +0200 Subject: [PATCH] Clean up PME domain count passing The PME domain count was passed in two ints with node and major/minor in the name, whereas it was actually the number of domains along x/y. Now a struct is passed around and three useless arguments to init_domain_decomposition have been removed. Change-Id: I6ac92f2d0515e8dec78581b65714cd5b8c48bc51 --- src/gromacs/domdec/domdec.cpp | 28 +++++++---------------- src/gromacs/domdec/domdec.h | 17 ++++++++------ src/gromacs/ewald/pme-load-balancing.cpp | 5 ++--- src/gromacs/ewald/pme.cpp | 37 ++++++++++++++++--------------- src/gromacs/ewald/pme.h | 5 +++-- src/gromacs/ewald/tests/pmetestcommon.cpp | 4 +++- src/gromacs/mdrun/runner.cpp | 12 +++++----- 7 files changed, 50 insertions(+), 58 deletions(-) diff --git a/src/gromacs/domdec/domdec.cpp b/src/gromacs/domdec/domdec.cpp index ec8a20d588..94a4f39565 100644 --- a/src/gromacs/domdec/domdec.cpp +++ b/src/gromacs/domdec/domdec.cpp @@ -2004,18 +2004,15 @@ static int dd_simnode2pmenode(const gmx_domdec_t *dd, return pmenode; } -void get_pme_nnodes(const gmx_domdec_t *dd, - int *npmenodes_x, int *npmenodes_y) +NumPmeDomains getNumPmeDomains(const gmx_domdec_t *dd) { if (dd != nullptr) { - *npmenodes_x = dd->comm->npmenodes_x; - *npmenodes_y = dd->comm->npmenodes_y; + return { dd->comm->npmenodes_x, dd->comm->npmenodes_y }; } else { - *npmenodes_x = 1; - *npmenodes_y = 1; + return { 1, 1 }; } } @@ -6345,8 +6342,7 @@ static void set_dd_limits_and_grid(FILE *fplog, t_commrec *cr, gmx_domdec_t *dd, const gmx_mtop_t *mtop, const t_inputrec *ir, const matrix box, const rvec *xGlobal, - gmx_ddbox_t *ddbox, - int *npme_x, int *npme_y) + gmx_ddbox_t *ddbox) { real r_bonded = -1; real r_bonded_limit = -1; @@ -6663,12 +6659,6 @@ static void set_dd_limits_and_grid(FILE *fplog, t_commrec *cr, gmx_domdec_t *dd, comm->npmenodes_y = 0; } - /* Technically we don't need both of these, - * but it simplifies code not having to recalculate it. - */ - *npme_x = comm->npmenodes_x; - *npme_y = comm->npmenodes_y; - snew(comm->slb_frac, DIM); if (isDlbDisabled(comm)) { @@ -7241,9 +7231,7 @@ gmx_domdec_t *init_domain_decomposition(FILE *fplog, t_commrec *cr, const gmx_mtop_t *mtop, const t_inputrec *ir, const matrix box, - const rvec *xGlobal, - gmx_ddbox_t *ddbox, - int *npme_x, int *npme_y) + const rvec *xGlobal) { gmx_domdec_t *dd; @@ -7259,17 +7247,17 @@ gmx_domdec_t *init_domain_decomposition(FILE *fplog, t_commrec *cr, set_dd_envvar_options(fplog, dd, cr->nodeid); + gmx_ddbox_t ddbox = {0}; set_dd_limits_and_grid(fplog, cr, dd, options, mdrunOptions, mtop, ir, box, xGlobal, - ddbox, - npme_x, npme_y); + &ddbox); make_dd_communicators(fplog, cr, dd, options.rankOrder); if (thisRankHasDuty(cr, DUTY_PP)) { - set_ddgrid_parameters(fplog, dd, options.dlbScaling, mtop, ir, ddbox); + set_ddgrid_parameters(fplog, dd, options.dlbScaling, mtop, ir, &ddbox); setup_neighbor_relations(dd); } diff --git a/src/gromacs/domdec/domdec.h b/src/gromacs/domdec/domdec.h index 23f347251a..662b56f3c7 100644 --- a/src/gromacs/domdec/domdec.h +++ b/src/gromacs/domdec/domdec.h @@ -124,9 +124,15 @@ int dd_natoms_vsite(const gmx_domdec_t *dd); void dd_get_constraint_range(const gmx_domdec_t *dd, int *at_start, int *at_end); -/*! \brief Get the number of PME nodes along x and y, can be called with dd=NULL */ -void get_pme_nnodes(const struct gmx_domdec_t *dd, - int *npmenodes_x, int *npmenodes_y); +/*! \libinternal \brief Struct for passing around the number of PME domains */ +struct NumPmeDomains +{ + int x; //!< The number of PME domains along dimension x + int y; //!< The number of PME domains along dimension y +}; + +/*! \brief Returns the number of PME domains, can be called with dd=NULL */ +NumPmeDomains getNumPmeDomains(const gmx_domdec_t *dd); /*! \brief Returns the set of DD ranks that communicate with pme node cr->nodeid */ std::vector get_pme_ddranks(const t_commrec *cr, int pmenodeid); @@ -200,10 +206,7 @@ gmx_domdec_t *init_domain_decomposition(FILE *fplog, const gmx_mtop_t *mtop, const t_inputrec *ir, const matrix box, - const rvec *xGlobal, - gmx_ddbox_t *ddbox, - int *npme_x, - int *npme_y); + const rvec *xGlobal); /*! \brief Initialize data structures for bonded interactions */ void dd_init_bondeds(FILE *fplog, diff --git a/src/gromacs/ewald/pme-load-balancing.cpp b/src/gromacs/ewald/pme-load-balancing.cpp index d88a0dd2ce..e5f60a34d2 100644 --- a/src/gromacs/ewald/pme-load-balancing.cpp +++ b/src/gromacs/ewald/pme-load-balancing.cpp @@ -316,7 +316,6 @@ static gmx_bool pme_loadbal_increase_cutoff(pme_load_balancing_t *pme_lb, const gmx_domdec_t *dd) { pme_setup_t *set; - int npmeranks_x, npmeranks_y; real fac, sp; real tmpr_coulomb, tmpr_vdw; int d; @@ -328,7 +327,7 @@ static gmx_bool pme_loadbal_increase_cutoff(pme_load_balancing_t *pme_lb, set = &pme_lb->setup[pme_lb->n-1]; set->pmedata = nullptr; - get_pme_nnodes(dd, &npmeranks_x, &npmeranks_y); + NumPmeDomains numPmeDomains = getNumPmeDomains(dd); fac = 1; do @@ -361,7 +360,7 @@ static gmx_bool pme_loadbal_increase_cutoff(pme_load_balancing_t *pme_lb, */ grid_ok = gmx_pme_check_restrictions(pme_order, set->grid[XX], set->grid[YY], set->grid[ZZ], - npmeranks_x, + numPmeDomains.x, true, false); } diff --git a/src/gromacs/ewald/pme.cpp b/src/gromacs/ewald/pme.cpp index 844754bfb0..e386eba265 100644 --- a/src/gromacs/ewald/pme.cpp +++ b/src/gromacs/ewald/pme.cpp @@ -83,6 +83,7 @@ #include #include +#include "gromacs/domdec/domdec.h" #include "gromacs/ewald/ewald-utils.h" #include "gromacs/fft/parallel_3dfft.h" #include "gromacs/fileio/pdbio.h" @@ -534,7 +535,7 @@ int minimalPmeGridSize(int pmeOrder) bool gmx_pme_check_restrictions(int pme_order, int nkx, int nky, int nkz, - int nnodes_major, + int numPmeDomainsAlongX, bool useThreads, bool errorsAreFatal) { @@ -569,15 +570,15 @@ bool gmx_pme_check_restrictions(int pme_order, /* Check for a limitation of the (current) sum_fftgrid_dd code. * We only allow multiple communication pulses in dim 1, not in dim 0. */ - if (useThreads && (nkx < nnodes_major*pme_order && - nkx != nnodes_major*(pme_order - 1))) + if (useThreads && (nkx < numPmeDomainsAlongX*pme_order && + nkx != numPmeDomainsAlongX*(pme_order - 1))) { if (!errorsAreFatal) { return false; } gmx_fatal(FARGS, "The number of PME grid lines per rank along x is %g. But when using OpenMP threads, the number of grid lines per rank along x should be >= pme_order (%d) or = pmeorder-1. To resolve this issue, use fewer ranks along x (and possibly more along y and/or z) by specifying -dd manually.", - nkx/(double)nnodes_major, pme_order); + nkx/static_cast(numPmeDomainsAlongX), pme_order); } return true; @@ -590,8 +591,7 @@ static int div_round_up(int enumerator, int denominator) } gmx_pme_t *gmx_pme_init(const t_commrec *cr, - int nnodes_major, - int nnodes_minor, + const NumPmeDomains &numPmeDomains, const t_inputrec *ir, int homenr, gmx_bool bFreeEnergy_q, @@ -623,17 +623,17 @@ gmx_pme_t *gmx_pme_init(const t_commrec *cr, pme->nnodes = 1; pme->bPPnode = TRUE; - pme->nnodes_major = nnodes_major; - pme->nnodes_minor = nnodes_minor; + pme->nnodes_major = numPmeDomains.x; + pme->nnodes_minor = numPmeDomains.y; #if GMX_MPI - if (nnodes_major*nnodes_minor > 1) + if (numPmeDomains.x*numPmeDomains.y > 1) { pme->mpi_comm = cr->mpi_comm_mygroup; MPI_Comm_rank(pme->mpi_comm, &pme->nodeid); MPI_Comm_size(pme->mpi_comm, &pme->nnodes); - if (pme->nnodes != nnodes_major*nnodes_minor) + if (pme->nnodes != numPmeDomains.x*numPmeDomains.y) { gmx_incons("PME rank count mismatch"); } @@ -659,7 +659,7 @@ gmx_pme_t *gmx_pme_init(const t_commrec *cr, } else { - if (nnodes_minor == 1) + if (numPmeDomains.y == 1) { #if GMX_MPI pme->mpi_comm_d[0] = pme->mpi_comm; @@ -670,7 +670,7 @@ gmx_pme_t *gmx_pme_init(const t_commrec *cr, pme->nodeid_minor = 0; } - else if (nnodes_major == 1) + else if (numPmeDomains.x == 1) { #if GMX_MPI pme->mpi_comm_d[0] = MPI_COMM_NULL; @@ -682,16 +682,16 @@ gmx_pme_t *gmx_pme_init(const t_commrec *cr, } else { - if (pme->nnodes % nnodes_major != 0) + if (pme->nnodes % numPmeDomains.x != 0) { - gmx_incons("For 2D PME decomposition, #PME ranks must be divisible by the number of ranks in the major dimension"); + gmx_incons("For 2D PME decomposition, #PME ranks must be divisible by the number of domains along x"); } pme->ndecompdim = 2; #if GMX_MPI - MPI_Comm_split(pme->mpi_comm, pme->nodeid % nnodes_minor, + MPI_Comm_split(pme->mpi_comm, pme->nodeid % numPmeDomains.y, pme->nodeid, &pme->mpi_comm_d[0]); /* My communicator along major dimension */ - MPI_Comm_split(pme->mpi_comm, pme->nodeid/nnodes_minor, + MPI_Comm_split(pme->mpi_comm, pme->nodeid/numPmeDomains.y, pme->nodeid, &pme->mpi_comm_d[1]); /* My communicator along minor dimension */ MPI_Comm_rank(pme->mpi_comm_d[0], &pme->nodeid_major); @@ -920,7 +920,7 @@ gmx_pme_t *gmx_pme_init(const t_commrec *cr, } /* Use atc[0] for spreading */ - init_atomcomm(pme.get(), &pme->atc[0], nnodes_major > 1 ? 0 : 1, TRUE); + init_atomcomm(pme.get(), &pme->atc[0], numPmeDomains.x > 1 ? 0 : 1, TRUE); if (pme->ndecompdim >= 2) { init_atomcomm(pme.get(), &pme->atc[1], 1, FALSE); @@ -999,7 +999,8 @@ void gmx_pme_reinit(struct gmx_pme_t **pmedata, // so we don't expect the actual logging. // TODO: when PME is an object, it should take reference to mdlog on construction and save it. GMX_ASSERT(pmedata, "Invalid PME pointer"); - *pmedata = gmx_pme_init(cr, pme_src->nnodes_major, pme_src->nnodes_minor, + NumPmeDomains numPmeDomains = { pme_src->nnodes_major, pme_src->nnodes_minor }; + *pmedata = gmx_pme_init(cr, numPmeDomains, &irc, homenr, pme_src->bFEP_q, pme_src->bFEP_lj, FALSE, ewaldcoeff_q, ewaldcoeff_lj, pme_src->nthread, pme_src->runMode, pme_src->gpu, nullptr, dummyLogger); //TODO this is mostly passing around current values diff --git a/src/gromacs/ewald/pme.h b/src/gromacs/ewald/pme.h index 168baaf036..84f121959f 100644 --- a/src/gromacs/ewald/pme.h +++ b/src/gromacs/ewald/pme.h @@ -66,6 +66,7 @@ struct gmx_wallclock_gpu_pme_t; struct gmx_device_info_t; struct gmx_pme_t; struct gmx_wallcycle; +struct NumPmeDomains; enum class GpuTaskCompletion; @@ -114,7 +115,7 @@ int minimalPmeGridSize(int pmeOrder); */ bool gmx_pme_check_restrictions(int pme_order, int nkx, int nky, int nkz, - int nnodes_major, + int numPmeDomainsAlongX, bool useThreads, bool errorsAreFatal); @@ -124,7 +125,7 @@ bool gmx_pme_check_restrictions(int pme_order, * \returns Pointer to newly allocated and initialized PME data. */ gmx_pme_t *gmx_pme_init(const t_commrec *cr, - int nnodes_major, int nnodes_minor, + const NumPmeDomains &numPmeDomains, const t_inputrec *ir, int homenr, gmx_bool bFreeEnergy_q, gmx_bool bFreeEnergy_lj, gmx_bool bReproducible, diff --git a/src/gromacs/ewald/tests/pmetestcommon.cpp b/src/gromacs/ewald/tests/pmetestcommon.cpp index ca65e70984..d47c1e9d35 100644 --- a/src/gromacs/ewald/tests/pmetestcommon.cpp +++ b/src/gromacs/ewald/tests/pmetestcommon.cpp @@ -45,6 +45,7 @@ #include +#include "gromacs/domdec/domdec.h" #include "gromacs/ewald/pme-gather.h" #include "gromacs/ewald/pme-gpu-internal.h" #include "gromacs/ewald/pme-grid.h" @@ -113,7 +114,8 @@ static PmeSafePointer pmeInitInternal(const t_inputrec *inputRec, } const auto runMode = (mode == CodePath::CPU) ? PmeRunMode::CPU : PmeRunMode::GPU; t_commrec dummyCommrec = {0}; - gmx_pme_t *pmeDataRaw = gmx_pme_init(&dummyCommrec, 1, 1, inputRec, atomCount, false, false, true, + NumPmeDomains numPmeDomains = { 1, 1 }; + gmx_pme_t *pmeDataRaw = gmx_pme_init(&dummyCommrec, numPmeDomains, inputRec, atomCount, false, false, true, ewaldCoeff_q, ewaldCoeff_lj, 1, runMode, nullptr, gpuInfo, dummyLogger); PmeSafePointer pme(pmeDataRaw); // taking ownership diff --git a/src/gromacs/mdrun/runner.cpp b/src/gromacs/mdrun/runner.cpp index a6a62e2039..729a293626 100644 --- a/src/gromacs/mdrun/runner.cpp +++ b/src/gromacs/mdrun/runner.cpp @@ -383,8 +383,6 @@ static TaskTarget findTaskTarget(const char *optionString) int Mdrunner::mdrunner() { matrix box; - gmx_ddbox_t ddbox = {0}; - int npme_major, npme_minor; t_nrnb *nrnb; t_forcerec *fr = nullptr; t_fcdata *fcd = nullptr; @@ -844,6 +842,7 @@ int Mdrunner::mdrunner() useGpuForNonbonded || (emulateGpuNonbonded == EmulateGpuNonbonded::Yes), *hwinfo->cpuInfo); } + /* Initalize the domain decomposition */ if (PAR(cr) && !(EI_TPI(inputrec->eI) || inputrec->eI == eiNM)) { @@ -851,8 +850,7 @@ int Mdrunner::mdrunner() cr->dd = init_domain_decomposition(fplog, cr, domdecOptions, mdrunOptions, &mtop, inputrec, - box, xOnMaster, - &ddbox, &npme_major, &npme_minor); + box, xOnMaster); // Note that local state still does not exist yet. } else @@ -860,8 +858,6 @@ int Mdrunner::mdrunner() /* PME, if used, is done on all nodes with 1D decomposition */ cr->npmenodes = 0; cr->duty = (DUTY_PP | DUTY_PME); - npme_major = 1; - npme_minor = 1; if (inputrec->ePBC == epbcSCREW) { @@ -1233,7 +1229,9 @@ int Mdrunner::mdrunner() { try { - pmedata = gmx_pme_init(cr, npme_major, npme_minor, inputrec, + pmedata = gmx_pme_init(cr, + getNumPmeDomains(cr->dd), + inputrec, mtop.natoms, nChargePerturbed, nTypePerturbed, mdrunOptions.reproducible, ewaldcoeff_q, ewaldcoeff_lj, -- 2.11.4.GIT