Move commrec duty checking into simple getters
authorAleksei Iupinov <a.yupinov@gmail.com>
Tue, 17 Oct 2017 11:15:24 +0000 (13:15 +0200)
committerBerk Hess <hess@kth.se>
Tue, 24 Oct 2017 12:43:22 +0000 (14:43 +0200)
This isolates all reads of cr->duty, and asserts on cr->duty being valid,
allowing to refactor its assignment in later changes.

Change-Id: I9b48be06b8d2db18105619ea1acfe38aa541b622

13 files changed:
src/gromacs/domdec/domdec.cpp
src/gromacs/ewald/pme-load-balancing.cpp
src/gromacs/ewald/pme.cpp
src/gromacs/gmxlib/network.cpp
src/gromacs/hardware/detecthardware.cpp
src/gromacs/mdlib/force.cpp
src/gromacs/mdlib/forcerec.cpp
src/gromacs/mdlib/gmx_omp_nthreads.cpp
src/gromacs/mdlib/minimize.cpp
src/gromacs/mdlib/sim_util.cpp
src/gromacs/mdtypes/commrec.h
src/programs/mdrun/md.cpp
src/programs/mdrun/runner.cpp

index 908fed99d054b440c9b3ada2ae2eb811590db060..8e580a29f7421c4c1a514eb5abc9864df6b86bfa 100644 (file)
@@ -5407,7 +5407,7 @@ void dd_setup_dlb_resource_sharing(t_commrec            *cr,
     gmx_domdec_t *dd;
     MPI_Comm      mpi_comm_pp_physicalnode;
 
-    if (!(cr->duty & DUTY_PP) || gpu_id < 0)
+    if (!thisRankHasDuty(cr, DUTY_PP) || gpu_id < 0)
     {
         /* Only ranks with short-ranged tasks (currently) use GPUs.
          * If we don't have GPUs assigned, there are no resources to share.
@@ -5688,7 +5688,7 @@ static void make_pp_communicator(FILE                 *fplog,
          */
         snew(comm->ddindex2simnodeid, dd->nnodes);
         snew(buf, dd->nnodes);
-        if (cr->duty & DUTY_PP)
+        if (thisRankHasDuty(cr, DUTY_PP))
         {
             buf[dd_index(dd->nc, dd->ci)] = cr->sim_nodeid;
         }
@@ -5749,7 +5749,7 @@ static void receive_ddindex2simnodeid(gmx_domdec_t         *dd,
         int *buf;
         snew(comm->ddindex2simnodeid, dd->nnodes);
         snew(buf, dd->nnodes);
-        if (cr->duty & DUTY_PP)
+        if (thisRankHasDuty(cr, DUTY_PP))
         {
             buf[dd_index(dd->nc, dd->ci)] = cr->sim_nodeid;
         }
@@ -5894,7 +5894,7 @@ static void split_communicator(FILE *fplog, t_commrec *cr, gmx_domdec_t *dd,
 
         /* Split the sim communicator into PP and PME only nodes */
         MPI_Comm_split(cr->mpi_comm_mysim,
-                       cr->duty,
+                       getThisRankDuties(cr),
                        dd_index(comm->ntot, dd->ci),
                        &cr->mpi_comm_mygroup);
     }
@@ -5933,7 +5933,7 @@ static void split_communicator(FILE *fplog, t_commrec *cr, gmx_domdec_t *dd,
 
         /* Split the sim communicator into PP and PME only nodes */
         MPI_Comm_split(cr->mpi_comm_mysim,
-                       cr->duty,
+                       getThisRankDuties(cr),
                        cr->nodeid,
                        &cr->mpi_comm_mygroup);
         MPI_Comm_rank(cr->mpi_comm_mygroup, &cr->nodeid);
@@ -5943,7 +5943,7 @@ static void split_communicator(FILE *fplog, t_commrec *cr, gmx_domdec_t *dd,
     if (fplog)
     {
         fprintf(fplog, "This rank does only %s work.\n\n",
-                (cr->duty & DUTY_PP) ? "particle-particle" : "PME-mesh");
+                thisRankHasDuty(cr, DUTY_PP) ? "particle-particle" : "PME-mesh");
     }
 }
 
@@ -5988,7 +5988,7 @@ static void make_dd_communicators(FILE *fplog, t_commrec *cr,
 #endif
     }
 
-    if (cr->duty & DUTY_PP)
+    if (thisRankHasDuty(cr, DUTY_PP))
     {
         /* Copy or make a new PP communicator */
         make_pp_communicator(fplog, dd, cr, CartReorder);
@@ -5998,7 +5998,7 @@ static void make_dd_communicators(FILE *fplog, t_commrec *cr,
         receive_ddindex2simnodeid(dd, cr);
     }
 
-    if (!(cr->duty & DUTY_PME))
+    if (!thisRankHasDuty(cr, DUTY_PME))
     {
         /* Set up the commnuication to our PME node */
         dd->pme_nodeid           = dd_simnode2pmenode(dd, cr, cr->sim_nodeid);
@@ -7274,7 +7274,7 @@ gmx_domdec_t *init_domain_decomposition(FILE *fplog, t_commrec *cr,
 
     make_dd_communicators(fplog, cr, dd, options.rankOrder);
 
-    if (cr->duty & DUTY_PP)
+    if (thisRankHasDuty(cr, DUTY_PP))
     {
         set_ddgrid_parameters(fplog, dd, options.dlbScaling, mtop, ir, ddbox);
 
@@ -9744,7 +9744,7 @@ void dd_partition_system(FILE                *fplog,
         make_local_gb(cr, fr->born, ir->gb_algorithm);
     }
 
-    if (!(cr->duty & DUTY_PME))
+    if (!thisRankHasDuty(cr, DUTY_PME))
     {
         /* Send the charges and/or c6/sigmas to our PME only node */
         gmx_pme_send_parameters(cr,
index dc85a081cfd7b10b0047255cc5148e632c2da774..ed26cd171b297b50a7996a5dea5ea53263f3d3b2 100644 (file)
@@ -186,7 +186,7 @@ void pme_loadbal_init(pme_load_balancing_t     **pme_lb_p,
 
     snew(pme_lb, 1);
 
-    pme_lb->bSepPMERanks      = !(cr->duty & DUTY_PME);
+    pme_lb->bSepPMERanks      = !thisRankHasDuty(cr, DUTY_PME);
 
     /* Initially we turn on balancing directly on based on PP/PME imbalance */
     pme_lb->bTriggerOnDLB     = FALSE;
index 8c02dcbff5aeb58def944be5f094ee38790ee637..91a84cd6952f3a5eb5a0e61e39b8ad3b29ae30d9 100644 (file)
@@ -619,7 +619,7 @@ int gmx_pme_init(struct gmx_pme_t   **pmedata,
             MPI_Comm_size(pme->mpi_comm_d[1], &pme->nnodes_minor);
 #endif
         }
-        pme->bPPnode = (cr->duty & DUTY_PP);
+        pme->bPPnode = thisRankHasDuty(cr, DUTY_PP);
     }
 
     pme->nthread = nthread;
index a68e54d0a5e6492a72997530a684f38dae84744e..082d980d42c21078a642f831f3d9b0e0cfc5f040 100644 (file)
@@ -283,7 +283,7 @@ void gmx_init_intranode_counters(t_commrec *cr)
     snew(hash_pp_s, nrank_world);
 
     hash_s[rank_world]    = myhash;
-    hash_pp_s[rank_world] = (cr->duty & DUTY_PP) ? myhash : -1;
+    hash_pp_s[rank_world] = thisRankHasDuty(cr, DUTY_PP) ? myhash : -1;
 
     MPI_Allreduce(hash_s,    hash,    nrank_world, MPI_INT, MPI_SUM, MPI_COMM_WORLD);
     MPI_Allreduce(hash_pp_s, hash_pp, nrank_world, MPI_INT, MPI_SUM, MPI_COMM_WORLD);
@@ -305,7 +305,7 @@ void gmx_init_intranode_counters(t_commrec *cr)
         if (hash_pp[i] == myhash)
         {
             nrank_pp_intranode++;
-            if ((cr->duty & DUTY_PP) && i < rank_world)
+            if (thisRankHasDuty(cr, DUTY_PP) && i < rank_world)
             {
                 rank_pp_intranode++;
             }
@@ -326,13 +326,13 @@ void gmx_init_intranode_counters(t_commrec *cr)
     if (debug)
     {
         char sbuf[STRLEN];
-        if ((cr->duty & DUTY_PP) && (cr->duty & DUTY_PME))
+        if (thisRankHasDuty(cr, DUTY_PP) && thisRankHasDuty(cr, DUTY_PME))
         {
             sprintf(sbuf, "PP+PME");
         }
         else
         {
-            sprintf(sbuf, "%s", (cr->duty & DUTY_PP) ? "PP" : "PME");
+            sprintf(sbuf, "%s", thisRankHasDuty(cr, DUTY_PP) ? "PP" : "PME");
         }
         fprintf(debug, "On %3s rank %d: nrank_intranode=%d, rank_intranode=%d, "
                 "nrank_pp_intranode=%d, rank_pp_intranode=%d\n",
index 177ef1087757a95d4f4e976da1c49e85793dae40..c63b73dba82a635bdd2e30883770f7d5b5c68db8 100644 (file)
@@ -144,7 +144,7 @@ static void gmx_detect_gpus(const gmx::MDLogger &mdlog, const t_commrec *cr)
 
     /*  With CUDA detect only on one rank per host, with OpenCL need do
      *  the detection on all PP ranks */
-    bool isOpenclPpRank = ((GMX_GPU == GMX_GPU_OPENCL) && (cr->duty & DUTY_PP));
+    bool isOpenclPpRank = ((GMX_GPU == GMX_GPU_OPENCL) && thisRankHasDuty(cr, DUTY_PP));
 
     if (rank_local == 0 || isOpenclPpRank)
     {
index 4af67d3c7201945afc84d03376a5529be3b82d6e..caa4e301920069f31eccb6cbb6d246ac4f55100b 100644 (file)
@@ -458,7 +458,8 @@ void do_force_lowlevel(t_forcerec *fr,      t_inputrec *ir,
                                             ewaldOutput.vir_q);
             }
 
-            if ((EEL_PME(fr->ic->eeltype) || EVDW_PME(fr->ic->vdwtype)) && (cr->duty & DUTY_PME) && (pme_run_mode(fr->pmedata) == PmeRunMode::CPU))
+            if ((EEL_PME(fr->ic->eeltype) || EVDW_PME(fr->ic->vdwtype)) &&
+                thisRankHasDuty(cr, DUTY_PME) && (pme_run_mode(fr->pmedata) == PmeRunMode::CPU))
             {
                 /* Do reciprocal PME for Coulomb and/or LJ. */
                 assert(fr->n_tpi >= 0);
index b462cbde01107fc7e05c50f1e5a85a6533a8c2c9..60fafe10a3284e13d4674921f88c5daf26f415ac 100644 (file)
@@ -3119,7 +3119,7 @@ void init_forcerec(FILE                *fp,
     snew(fr->ns, 1);
     init_ns(fp, cr, fr->ns, fr, mtop);
 
-    if (cr->duty & DUTY_PP)
+    if (thisRankHasDuty(cr, DUTY_PP))
     {
         gmx_nonbonded_setup(fr, bGenericKernelOnly);
     }
@@ -3168,7 +3168,7 @@ void free_gpu_resources(const t_forcerec        *fr,
     gmx_bool bIsPPrankUsingGPU;
     char     gpu_err_str[STRLEN];
 
-    bIsPPrankUsingGPU = (cr->duty & DUTY_PP) && fr && fr->nbv && fr->nbv->bUseGPU;
+    bIsPPrankUsingGPU = thisRankHasDuty(cr, DUTY_PP) && fr && fr->nbv && fr->nbv->bUseGPU;
 
     if (bIsPPrankUsingGPU)
     {
index 211bc2b9515d52aba59e86bd2a5e93f8e3c113d0..6c3b06b0992affb830af5c6d7c06184739cadbdd 100644 (file)
@@ -524,8 +524,7 @@ void gmx_omp_nthreads_init(const gmx::MDLogger &mdlog, t_commrec *cr,
     /* number of MPI processes/threads per physical node */
     nppn = cr->nrank_intranode;
 
-    bSepPME = ( (cr->duty & DUTY_PP) && !(cr->duty & DUTY_PME)) ||
-        (!(cr->duty & DUTY_PP) &&  (cr->duty & DUTY_PME));
+    bSepPME = (thisRankHasDuty(cr, DUTY_PP) != thisRankHasDuty(cr, DUTY_PME));
 
     manage_number_of_openmp_threads(mdlog, cr, bOMP,
                                     nthreads_hw_avail,
index 6ed5fc1a2b61daad392ac789f3b64ed707fa1d4e..52e7b42a615c789bf651bc85ecb1e168f6058a83 100644 (file)
@@ -479,7 +479,7 @@ static void finish_em(t_commrec *cr, gmx_mdoutf_t outf,
                       gmx_walltime_accounting_t walltime_accounting,
                       gmx_wallcycle_t wcycle)
 {
-    if (!(cr->duty & DUTY_PME))
+    if (!thisRankHasDuty(cr, DUTY_PME))
     {
         /* Tell the PME only node to finish */
         gmx_pme_send_finish(cr);
index 0b527045a4313b6cb177c6e9bab004d9a0894cf7..7aad7bdedeebcc76905739301189e612d9194f99 100644 (file)
@@ -837,7 +837,7 @@ static void do_force_cutsVERLET(FILE *fplog, t_commrec *cr,
 
     const auto pmeRunMode = fr->pmedata ? pme_run_mode(fr->pmedata) : PmeRunMode::CPU;
     // TODO slim this conditional down - inputrec and duty checks should mean the same in proper code!
-    const bool useGpuPme  = EEL_PME(fr->ic->eeltype) && (cr->duty & DUTY_PME) &&
+    const bool useGpuPme  = EEL_PME(fr->ic->eeltype) && thisRankHasDuty(cr, DUTY_PME) &&
         ((pmeRunMode == PmeRunMode::GPU) || (pmeRunMode == PmeRunMode::Hybrid));
     // a comment for uncrustify
     const ArrayRef<RVec> pmeGpuForces = *fr->forceBufferIntermediate;
@@ -912,7 +912,7 @@ static void do_force_cutsVERLET(FILE *fplog, t_commrec *cr,
                                  fr->shift_vec, nbv->nbat);
 
 #if GMX_MPI
-    if (!(cr->duty & DUTY_PME))
+    if (!thisRankHasDuty(cr, DUTY_PME))
     {
         /* Send particle coordinates to the pme nodes.
          * Since this is only implemented for domain decomposition
@@ -1188,7 +1188,7 @@ static void do_force_cutsVERLET(FILE *fplog, t_commrec *cr,
     reset_enerdata(enerd);
     clear_rvecs(SHIFTS, fr->fshift);
 
-    if (DOMAINDECOMP(cr) && !(cr->duty & DUTY_PME))
+    if (DOMAINDECOMP(cr) && !thisRankHasDuty(cr, DUTY_PME))
     {
         wallcycle_start(wcycle, ewcPPDURINGPME);
         dd_force_flop_start(cr->dd, nrnb);
@@ -1508,7 +1508,7 @@ static void do_force_cutsVERLET(FILE *fplog, t_commrec *cr,
         }
     }
 
-    if (PAR(cr) && !(cr->duty & DUTY_PME))
+    if (PAR(cr) && !thisRankHasDuty(cr, DUTY_PME))
     {
         /* In case of node-splitting, the PP nodes receive the long-range
          * forces, virial and energy from the PME nodes here.
@@ -1635,7 +1635,7 @@ static void do_force_cutsGROUP(FILE *fplog, t_commrec *cr,
     }
 
 #if GMX_MPI
-    if (!(cr->duty & DUTY_PME))
+    if (!thisRankHasDuty(cr, DUTY_PME))
     {
         /* Send particle coordinates to the pme nodes.
          * Since this is only implemented for domain decomposition
@@ -1723,7 +1723,7 @@ static void do_force_cutsGROUP(FILE *fplog, t_commrec *cr,
                        x, box, fr, &top->idef, graph, fr->born);
     }
 
-    if (DOMAINDECOMP(cr) && !(cr->duty & DUTY_PME))
+    if (DOMAINDECOMP(cr) && !thisRankHasDuty(cr, DUTY_PME))
     {
         wallcycle_start(wcycle, ewcPPDURINGPME);
         dd_force_flop_start(cr->dd, nrnb);
@@ -1844,7 +1844,7 @@ static void do_force_cutsGROUP(FILE *fplog, t_commrec *cr,
         }
     }
 
-    if (PAR(cr) && !(cr->duty & DUTY_PME))
+    if (PAR(cr) && !thisRankHasDuty(cr, DUTY_PME))
     {
         /* In case of node-splitting, the PP nodes receive the long-range
          * forces, virial and energy from the PME nodes here.
@@ -2567,7 +2567,7 @@ void finish_run(FILE *fplog, const gmx::MDLogger &mdlog, t_commrec *cr,
         sfree(nrnb_tot);
     }
 
-    if ((cr->duty & DUTY_PP) && DOMAINDECOMP(cr))
+    if (thisRankHasDuty(cr, DUTY_PP) && DOMAINDECOMP(cr))
     {
         print_dd_statistics(cr, inputrec, fplog);
     }
@@ -2578,7 +2578,7 @@ void finish_run(FILE *fplog, const gmx::MDLogger &mdlog, t_commrec *cr,
      * to task parallelism. */
     int nthreads_pp  = gmx_omp_nthreads_get(emntNonbonded);
     int nthreads_pme = gmx_omp_nthreads_get(emntPME);
-    wallcycle_scale_by_num_threads(wcycle, cr->duty == DUTY_PME, nthreads_pp, nthreads_pme);
+    wallcycle_scale_by_num_threads(wcycle, thisRankHasDuty(cr, DUTY_PME) && !thisRankHasDuty(cr, DUTY_PP), nthreads_pp, nthreads_pme);
     auto cycle_sum(wallcycle_sum(cr, wcycle));
 
     if (printReport)
index cdbf2b9e235dda703b39c4a4b8a81efcd9da2cf9..0345f1edd33a67a330fc5d0645811f23b8d466ff 100644 (file)
@@ -41,6 +41,7 @@
 
 #include "gromacs/math/vectypes.h"
 #include "gromacs/utility/basedefinitions.h"
+#include "gromacs/utility/gmxassert.h"
 #include "gromacs/utility/gmxmpi.h"
 #include "gromacs/utility/real.h"
 
@@ -117,7 +118,9 @@ struct t_commrec {
     /* For domain decomposition */
     gmx_domdec_t *dd;
 
-    /* The duties of this node, see the defines above */
+    /* The duties of this node, see the DUTY_ defines above.
+     * This should be read through thisRankHasDuty() or getThisRankDuties().
+     */
     int                    duty;
 
     gmx_multisim_t        *ms;
@@ -127,6 +130,31 @@ struct t_commrec {
     mpi_in_place_buf_t *mpb;
 };
 
+/*! \brief
+ * Returns the rank's duty, and asserts that it has been initialized.
+ */
+inline int getThisRankDuties(const t_commrec *cr)
+{
+    GMX_ASSERT(cr, "Invalid commrec pointer");
+    GMX_ASSERT(cr->duty != 0, "Commrec duty was not initialized!");
+    return cr->duty;
+}
+
+/*! \brief
+ * A convenience getter for the commrec duty assignment;
+ * asserts that duty is actually valid (have been initialized).
+ *
+ * \param[in] cr    Communication structure pointer
+ * \param[in] duty  A single duty's corresponding DUTY_ flag. Combinations are not supported.
+ *
+ * \returns Whether this duty is assigned to this rank.
+ */
+inline bool thisRankHasDuty(const t_commrec *cr, int duty)
+{
+    GMX_ASSERT((duty == DUTY_PME) || (duty == DUTY_PP), "Invalid duty type");
+    return (getThisRankDuties(cr) & duty);
+}
+
 //! True if this is a simulation with more than 1 node
 #define PAR(cr)        ((cr)->nnodes > 1)
 
@@ -134,7 +162,7 @@ struct t_commrec {
 #define MASTER(cr)     (((cr)->nodeid == 0) || !PAR(cr))
 
 //! True if this is the particle-particle master
-#define SIMMASTER(cr)  ((MASTER(cr) && ((cr)->duty & DUTY_PP)) || !PAR(cr))
+#define SIMMASTER(cr)  ((MASTER(cr) && thisRankHasDuty((cr), DUTY_PP)) || !PAR(cr))
 
 //! The node id for this rank
 #define RANK(cr, nodeid)    (nodeid)
index e96e080667cd31fd803c8f2fb34e07dd73295a3f..4aaca3e28e4dd745db9cf2b1b0ef5561a46e70f1 100644 (file)
@@ -1873,7 +1873,7 @@ double gmx::do_md(FILE *fplog, t_commrec *cr, const gmx::MDLogger &mdlog,
             reset_all_counters(fplog, mdlog, cr, step, &step_rel, ir, wcycle, nrnb, walltime_accounting,
                                use_GPU(fr->nbv) ? fr->nbv : nullptr, fr->pmedata);
             wcycle_set_reset_counters(wcycle, -1);
-            if (!(cr->duty & DUTY_PME))
+            if (!thisRankHasDuty(cr, DUTY_PME))
             {
                 /* Tell our PME node to reset its counters */
                 gmx_pme_send_resetcounters(cr, step);
@@ -1904,7 +1904,7 @@ double gmx::do_md(FILE *fplog, t_commrec *cr, const gmx::MDLogger &mdlog,
         close_trx(status);
     }
 
-    if (!(cr->duty & DUTY_PME))
+    if (!thisRankHasDuty(cr, DUTY_PME))
     {
         /* Tell the PME only node to finish */
         gmx_pme_send_finish(cr);
index 79231d1a8961cc03965f684d9166c937c03dbaa6..c25547d4c25c91c333def6ec08b635d434d37b94 100644 (file)
@@ -853,7 +853,7 @@ int Mdrunner::mdrunner()
                           hwinfo->nthreads_hw_avail,
                           hw_opt.nthreads_omp,
                           hw_opt.nthreads_omp_pme,
-                          (cr->duty & DUTY_PP) == 0,
+                          !thisRankHasDuty(cr, DUTY_PP),
                           inputrec->cutoff_scheme == ecutsVERLET);
 
 #ifndef NDEBUG
@@ -876,7 +876,7 @@ int Mdrunner::mdrunner()
          * across PP ranks on a node, with possible multiple devices
          * or sharing devices on a node, either from the user
          * selection, or automatically. */
-        bool rankCanUseGpu = cr->duty & DUTY_PP;
+        bool rankCanUseGpu = thisRankHasDuty(cr, DUTY_PP);
         gpuTaskAssignment = mapPpRanksToGpus(rankCanUseGpu, cr, hwinfo->gpu_info, hw_opt);
     }
 
@@ -910,7 +910,7 @@ int Mdrunner::mdrunner()
 
     gmx_device_info_t *shortRangedDeviceInfo = nullptr;
     int                shortRangedDeviceId   = -1;
-    if (cr->duty & DUTY_PP)
+    if (thisRankHasDuty(cr, DUTY_PP))
     {
         if (!gpuTaskAssignment.empty())
         {
@@ -956,7 +956,7 @@ int Mdrunner::mdrunner()
     }
 
     snew(nrnb, 1);
-    if (cr->duty & DUTY_PP)
+    if (thisRankHasDuty(cr, DUTY_PP))
     {
         /* Initiate forcerecord */
         fr                 = mk_forcerec();
@@ -1042,7 +1042,7 @@ int Mdrunner::mdrunner()
 
         int nthread_local;
         /* threads on this MPI process or TMPI thread */
-        if (cr->duty & DUTY_PP)
+        if (thisRankHasDuty(cr, DUTY_PP))
         {
             nthread_local = gmx_omp_nthreads_get(emntNonbonded);
         }
@@ -1075,7 +1075,7 @@ int Mdrunner::mdrunner()
             gmx_bcast_sim(sizeof(nTypePerturbed), &nTypePerturbed, cr);
         }
 
-        if (cr->duty & DUTY_PME)
+        if (thisRankHasDuty(cr, DUTY_PME))
         {
             try
             {
@@ -1106,7 +1106,7 @@ int Mdrunner::mdrunner()
         signal_handler_install();
     }
 
-    if (cr->duty & DUTY_PP)
+    if (thisRankHasDuty(cr, DUTY_PP))
     {
         /* Assumes uniform use of the number of OpenMP threads */
         walltime_accounting = walltime_accounting_init(gmx_omp_nthreads_get(emntDefault));