Clarify force buffer setup code in do_force

Refactored code and made conditionals non-nested to improve
the ease of understanding when is a common or separate buffer used for
the forces when direct virial contribution is computed.

Also add subcounter for force buffer clearing which also helps annotate
code that should be conditional on whether any of these buffers are used
to accumulate or only to copy into (e.g. with everything offloaded to a
GPU).

Refs #2802

Change-Id: I3fa5a3e4e4adf5cfe0eb417f0c1c3d0ed4a96769

diff --git a/src/gromacs/mdlib/sim_util.cpp b/src/gromacs/mdlib/sim_util.cpp
index d386490a943300d55f5fac19d9d75ffeba41c4eb..23d290e8d9443aca614526b6c003f8c78df0456c 100644 (file)
--- a/src/gromacs/mdlib/sim_util.cpp
+++ b/src/gromacs/mdlib/sim_util.cpp
@@ -1075,43 +1075,50 @@ static void do_force_cutsVERLET(FILE *fplog,
         wallcycle_stop(wcycle, ewcROT);
     }
 
-    /* Temporary solution until all routines take PaddedRVecVector */
-    rvec *const f = as_rvec_array(force.unpaddedArrayRef().data());
-
     /* Start the force cycle counter.
      * Note that a different counter is used for dynamic load balancing.
      */
     wallcycle_start(wcycle, ewcFORCE);
 
-    gmx::ArrayRef<gmx::RVec> forceRef = force.unpaddedArrayRef();
+    // Set up/clear the different force buffers
+    wallcycle_sub_start(wcycle, ewcsCLEAR_FORCE_BUFFER);
+
+    /* Temporary solution until all routines take PaddedRVecVector */
+    rvec *const f = as_rvec_array(force.unpaddedArrayRef().data());
     if (bDoForces)
     {
-        /* If we need to compute the virial, we might need a separate
-         * force buffer for algorithms for which the virial is calculated
-         * directly, such as PME.
-         */
-        if ((flags & GMX_FORCE_VIRIAL) && fr->haveDirectVirialContributions)
-        {
-            forceRef = *fr->forceBufferForDirectVirialContributions;
-
-            /* TODO: update comment
-             * We only compute forces on local atoms. Note that vsites can
-             * spread to non-local atoms, but that part of the buffer is
-             * cleared separately in the vsite spreading code.
-             */
-            clear_rvecs_omp(forceRef.size(), as_rvec_array(forceRef.data()));
-        }
         /* Clear the short- and long-range forces */
         clear_rvecs_omp(fr->natoms_force_constr, f);
     }
 
+    /* If we need to compute the virial, we might need a separate
+     * force buffer for algorithms for which the virial is calculated
+     * directly, such as PME. Otherwise, forceWithVirial uses the
+     * the same force (f in legacy calls) buffer as other algorithms.
+     */
+    const bool  useSeparateForceWithVirialBuffer = (bDoForces && ((flags & GMX_FORCE_VIRIAL) != 0 && fr->haveDirectVirialContributions));
+
+
     /* forceWithVirial uses the local atom range only */
-    gmx::ForceWithVirial forceWithVirial(forceRef, (flags & GMX_FORCE_VIRIAL) != 0);
+    gmx::ForceWithVirial forceWithVirial(useSeparateForceWithVirialBuffer ?
+                                         *fr->forceBufferForDirectVirialContributions : force.unpaddedArrayRef(),
+                                         (flags & GMX_FORCE_VIRIAL) != 0);
+
+    if (useSeparateForceWithVirialBuffer)
+    {
+        /* TODO: update comment
+         * We only compute forces on local atoms. Note that vsites can
+         * spread to non-local atoms, but that part of the buffer is
+         * cleared separately in the vsite spreading code.
+         */
+        clear_rvecs_omp(forceWithVirial.force_.size(), as_rvec_array(forceWithVirial.force_.data()));
+    }
 
     if (inputrec->bPull && pull_have_constraint(inputrec->pull_work))
     {
         clear_pull_forces(inputrec->pull_work);
     }
+    wallcycle_sub_stop(wcycle, ewcsCLEAR_FORCE_BUFFER);
 
     /* We calculate the non-bonded forces, when done on the CPU, here.
      * We do this before calling do_force_lowlevel, because in that
@@ -1594,6 +1601,7 @@ static void do_force_cutsGROUP(FILE *fplog,
         wallcycle_stop(wcycle, ewcROT);
     }
 
+
     /* Temporary solution until all routines take PaddedRVecVector */
     rvec *f = as_rvec_array(force.unpaddedArrayRef().data());
 

diff --git a/src/gromacs/timing/wallcycle.cpp b/src/gromacs/timing/wallcycle.cpp
index 9655d9886800b415eb5dc5b5515063dfd578aedf..7aeb422a1285a1261e72dee278440d6eb0435a4a 100644 (file)
--- a/src/gromacs/timing/wallcycle.cpp
+++ b/src/gromacs/timing/wallcycle.cpp
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 1991-2000, University of Groningen, The Netherlands.
  * Copyright (c) 2001-2008, The GROMACS development team.
- * Copyright (c) 2013,2014,2015,2016,2017,2018, by the GROMACS development team, led by
+ * Copyright (c) 2013,2014,2015,2016,2017,2018,2019, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -133,6 +133,8 @@ static const char *wcsn[ewcsNR] =
     "Ewald F correction",
     "NB X buffer ops.",
     "NB F buffer ops.",
+    "Clear force buffer",
+    "Test subcounter",
 };
 
 /* PME GPU timing events' names - correspond to the enum in the gpu_timing.h */

diff --git a/src/gromacs/timing/wallcycle.h b/src/gromacs/timing/wallcycle.h
index cbf7596544203e8576efc03fad67001c69b51778..fc8266f750a766d8e3e5a6ea0c5acc112c3fb8b0 100644 (file)
--- a/src/gromacs/timing/wallcycle.h
+++ b/src/gromacs/timing/wallcycle.h
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 1991-2000, University of Groningen, The Netherlands.
  * Copyright (c) 2001-2008, The GROMACS development team.
- * Copyright (c) 2013,2014,2015,2017,2018, by the GROMACS development team, led by
+ * Copyright (c) 2013,2014,2015,2017,2018,2019, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -79,6 +79,8 @@ enum {
     ewcsEWALD_CORRECTION,
     ewcsNB_X_BUF_OPS,
     ewcsNB_F_BUF_OPS,
+    ewcsCLEAR_FORCE_BUFFER,
+    ewcsTEST,
     ewcsNR
 };