Improve PME testing

The change to PME coordinate management uglified these tests even
further. Creation semantics are now more clear.

Change-Id: I309a487cdd218d85dbe4620e1d94e2f04f809a8d

diff --git a/src/gromacs/ewald/tests/pmegathertest.cpp b/src/gromacs/ewald/tests/pmegathertest.cpp
index 0d4e94db6c93d0f514ad6861435eb10c6205c760..d2afc1e324f2022165d1ec2e3c66ce86491edc23 100644 (file)
--- a/src/gromacs/ewald/tests/pmegathertest.cpp
+++ b/src/gromacs/ewald/tests/pmegathertest.cpp
@@ -388,13 +388,12 @@ class PmeGatherTest : public ::testing::TestWithParam<GatherInputParameters>
             TestReferenceData refData;
             for (const auto &context : getPmeTestEnv()->getHardwareContexts())
             {
-                std::shared_ptr<StatePropagatorDataGpu> stateGpu;
                 CodePath   codePath       = context->getCodePath();
                 const bool supportedInput = pmeSupportsInputForMode(*getPmeTestEnv()->hwinfo(), &inputRec, codePath);
                 if (!supportedInput)
                 {
                     /* Testing the failure for the unsupported input */
-                    EXPECT_THROW(pmeInitAtoms(&inputRec, codePath, nullptr, nullptr, inputAtomData.coordinates, inputAtomData.charges, box, stateGpu), NotImplementedError);
+                    EXPECT_THROW(pmeInitWrapper(&inputRec, codePath, nullptr, nullptr, box), NotImplementedError);
                     continue;
                 }
 
@@ -408,8 +407,9 @@ class PmeGatherTest : public ::testing::TestWithParam<GatherInputParameters>
                                           (inputForceTreatment == PmeForceOutputHandling::ReduceWithInput) ? "with reduction" : "without reduction"
                                           ));
 
-                PmeSafePointer pmeSafe = pmeInitAtoms(&inputRec, codePath, context->getDeviceInfo(),
-                                                      context->getPmeGpuProgram(), inputAtomData.coordinates, inputAtomData.charges, box, stateGpu);
+                PmeSafePointer         pmeSafe  = pmeInitWrapper(&inputRec, codePath, context->getDeviceInfo(), context->getPmeGpuProgram(), box);
+                StatePropagatorDataGpu stateGpu = makeStatePropagatorDataGpu(*pmeSafe.get());
+                pmeInitAtoms(pmeSafe.get(), &stateGpu, codePath, inputAtomData.coordinates, inputAtomData.charges);
 
                 /* Setting some more inputs */
                 pmeSetRealGrid(pmeSafe.get(), codePath, nonZeroGridValues);

diff --git a/src/gromacs/ewald/tests/pmesplinespreadtest.cpp b/src/gromacs/ewald/tests/pmesplinespreadtest.cpp
index a1c6eb4c1dc6c7147769a50e3d07f00c2d1e826e..136c08b69733a400c73d56c4831f89fb0e357d1f 100644 (file)
--- a/src/gromacs/ewald/tests/pmesplinespreadtest.cpp
+++ b/src/gromacs/ewald/tests/pmesplinespreadtest.cpp
@@ -128,7 +128,7 @@ class PmeSplineAndSpreadTest : public ::testing::TestWithParam<SplineAndSpreadIn
                 if (!supportedInput)
                 {
                     /* Testing the failure for the unsupported input */
-                    EXPECT_THROW(pmeInitAtoms(&inputRec, codePath, nullptr, nullptr, coordinates, charges, box, stateGpu), NotImplementedError);
+                    EXPECT_THROW(pmeInitWrapper(&inputRec, codePath, nullptr, nullptr, box), NotImplementedError);
                     continue;
                 }
 
@@ -146,8 +146,9 @@ class PmeSplineAndSpreadTest : public ::testing::TestWithParam<SplineAndSpreadIn
 
                     /* Running the test */
 
-                    PmeSafePointer pmeSafe = pmeInitAtoms(&inputRec, codePath, context->getDeviceInfo(),
-                                                          context->getPmeGpuProgram(), coordinates, charges, box, stateGpu);
+                    PmeSafePointer         pmeSafe  = pmeInitWrapper(&inputRec, codePath, context->getDeviceInfo(), context->getPmeGpuProgram(), box);
+                    StatePropagatorDataGpu stateGpu = makeStatePropagatorDataGpu(*pmeSafe.get());
+                    pmeInitAtoms(pmeSafe.get(), &stateGpu, codePath, coordinates, charges);
 
                     const bool     computeSplines = (option.first == PmeSplineAndSpreadOptions::SplineOnly) || (option.first == PmeSplineAndSpreadOptions::SplineAndSpreadUnified);
                     const bool     spreadCharges  = (option.first == PmeSplineAndSpreadOptions::SpreadOnly) || (option.first == PmeSplineAndSpreadOptions::SplineAndSpreadUnified);

diff --git a/src/gromacs/ewald/tests/pmetestcommon.cpp b/src/gromacs/ewald/tests/pmetestcommon.cpp
index 61b28e587ab4cfae47f4e1f7cf19a214ae970cf9..c3e538eb00372ce1a7724e985749ec28c5854aea 100644 (file)
--- a/src/gromacs/ewald/tests/pmetestcommon.cpp
+++ b/src/gromacs/ewald/tests/pmetestcommon.cpp
@@ -106,15 +106,14 @@ uint64_t getSplineModuliDoublePrecisionUlps(int splineOrder)
     return 4 * (splineOrder - 2) + 2 * sineUlps * splineOrder;
 }
 
-//! PME initialization - internal
-static PmeSafePointer pmeInitInternal(const t_inputrec         *inputRec,
-                                      CodePath                  mode,
-                                      const gmx_device_info_t  *gpuInfo,
-                                      PmeGpuProgramHandle       pmeGpuProgram,
-                                      const Matrix3x3          &box,
-                                      real                      ewaldCoeff_q = 1.0F,
-                                      real                      ewaldCoeff_lj = 1.0F
-                                      )
+//! PME initialization
+PmeSafePointer pmeInitWrapper(const t_inputrec         *inputRec,
+                              const CodePath            mode,
+                              const gmx_device_info_t  *gpuInfo,
+                              PmeGpuProgramHandle       pmeGpuProgram,
+                              const Matrix3x3          &box,
+                              const real                ewaldCoeff_q,
+                              const real                ewaldCoeff_lj)
 {
     const MDLogger dummyLogger;
     const auto     runMode       = (mode == CodePath::CPU) ? PmeRunMode::CPU : PmeRunMode::Mixed;
@@ -156,7 +155,7 @@ static PmeSafePointer pmeInitInternal(const t_inputrec         *inputRec,
 
 //! Simple PME initialization based on input, no atom data
 PmeSafePointer pmeInitEmpty(const t_inputrec         *inputRec,
-                            CodePath                  mode,
+                            const CodePath            mode,
                             const gmx_device_info_t  *gpuInfo,
                             PmeGpuProgramHandle       pmeGpuProgram,
                             const Matrix3x3          &box,
@@ -164,59 +163,58 @@ PmeSafePointer pmeInitEmpty(const t_inputrec         *inputRec,
                             real                      ewaldCoeff_lj
                             )
 {
-    return pmeInitInternal(inputRec, mode, gpuInfo, pmeGpuProgram, box, ewaldCoeff_q, ewaldCoeff_lj);
+    return pmeInitWrapper(inputRec, mode, gpuInfo, pmeGpuProgram, box, ewaldCoeff_q, ewaldCoeff_lj);
     // hiding the fact that PME actually needs to know the number of atoms in advance
 }
 
+//! Make a GPU state-propagator manager
+StatePropagatorDataGpu
+makeStatePropagatorDataGpu(const gmx_pme_t &pme)
+{
+    // TODO: Pin the host buffer and use async memory copies
+    return StatePropagatorDataGpu(pme_gpu_get_device_stream(&pme),
+                                  pme_gpu_get_device_context(&pme),
+                                  GpuApiCallBehavior::Sync,
+                                  pme_gpu_get_padding_size(&pme));
+}
+
 //! PME initialization with atom data
-PmeSafePointer pmeInitAtoms(const t_inputrec                        *inputRec,
-                            CodePath                                 mode,
-                            const gmx_device_info_t                 *gpuInfo,
-                            PmeGpuProgramHandle                      pmeGpuProgram,
-                            const CoordinatesVector                 &coordinates,
-                            const ChargesVector                     &charges,
-                            const Matrix3x3                         &box,
-                            std::shared_ptr<StatePropagatorDataGpu>  stateGpu
-                            )
+void pmeInitAtoms(gmx_pme_t               *pme,
+                  StatePropagatorDataGpu  *stateGpu,
+                  const CodePath           mode,
+                  const CoordinatesVector &coordinates,
+                  const ChargesVector     &charges)
 {
-    const index     atomCount = coordinates.size();
+    const index  atomCount = coordinates.size();
     GMX_RELEASE_ASSERT(atomCount == charges.ssize(), "Mismatch in atom data");
-    PmeSafePointer  pmeSafe = pmeInitInternal(inputRec, mode, gpuInfo, pmeGpuProgram, box);
-    PmeAtomComm    *atc     = nullptr;
+    PmeAtomComm *atc = nullptr;
 
     switch (mode)
     {
         case CodePath::CPU:
-            atc              = &(pmeSafe->atc[0]);
+            atc              = &(pme->atc[0]);
             atc->x           = coordinates;
             atc->coefficient = charges;
-            gmx_pme_reinit_atoms(pmeSafe.get(), atomCount, charges.data());
+            gmx_pme_reinit_atoms(pme, atomCount, charges.data());
             /* With decomposition there would be more boilerplate atc code here, e.g. do_redist_pos_coeffs */
             break;
 
         case CodePath::GPU:
             // TODO: Avoid use of atc in the GPU code path
-            atc              = &(pmeSafe->atc[0]);
+            atc              = &(pme->atc[0]);
             // We need to set atc->n for passing the size in the tests
             atc->setNumAtoms(atomCount);
-            gmx_pme_reinit_atoms(pmeSafe.get(), atomCount, charges.data());
+            gmx_pme_reinit_atoms(pme, atomCount, charges.data());
 
-            // TODO: Pin the host buffer and use async memory copies
-            stateGpu = std::make_unique<StatePropagatorDataGpu>(pme_gpu_get_device_stream(pmeSafe.get()),
-                                                                pme_gpu_get_device_context(pmeSafe.get()),
-                                                                GpuApiCallBehavior::Sync,
-                                                                pme_gpu_get_padding_size(pmeSafe.get()));
             stateGpu->reinit(atomCount, atomCount);
             stateGpu->copyCoordinatesToGpu(arrayRefFromArray(coordinates.data(), coordinates.size()), gmx::StatePropagatorDataGpu::AtomLocality::All);
-            pme_gpu_set_kernelparam_coordinates(pmeSafe->gpu, stateGpu->getCoordinates());
+            pme_gpu_set_kernelparam_coordinates(pme->gpu, stateGpu->getCoordinates());
 
             break;
 
         default:
             GMX_THROW(InternalError("Test not implemented for this mode"));
     }
-
-    return pmeSafe;
 }
 
 //! Getting local PME real grid pointer for test I/O

diff --git a/src/gromacs/ewald/tests/pmetestcommon.h b/src/gromacs/ewald/tests/pmetestcommon.h
index b51551c29a6041c558c74435010682209d96ef77..dd27865ab7034b957190fef5afe0ce91aae13cf3 100644 (file)
--- a/src/gromacs/ewald/tests/pmetestcommon.h
+++ b/src/gromacs/ewald/tests/pmetestcommon.h
@@ -118,6 +118,14 @@ uint64_t getSplineModuliDoublePrecisionUlps(int splineOrder);
 
 // PME stages
 
+//! PME initialization
+PmeSafePointer pmeInitWrapper(const t_inputrec         *inputRec,
+                              CodePath                  mode,
+                              const gmx_device_info_t  *gpuInfo,
+                              PmeGpuProgramHandle       pmeGpuProgram,
+                              const Matrix3x3          &box,
+                              real                      ewaldCoeff_q = 1.0F,
+                              real                      ewaldCoeff_lj = 1.0F);
 //! Simple PME initialization (no atom data)
 PmeSafePointer pmeInitEmpty(const t_inputrec *inputRec,
                             CodePath mode = CodePath::CPU,
@@ -125,16 +133,15 @@ PmeSafePointer pmeInitEmpty(const t_inputrec *inputRec,
                             PmeGpuProgramHandle pmeGpuProgram = nullptr,
                             const Matrix3x3 &box = {{1.0F, 0.0F, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, 0.0F, 1.0F}},
                             real ewaldCoeff_q = 0.0F, real ewaldCoeff_lj = 0.0F);
+//! Make a GPU state-propagator manager
+StatePropagatorDataGpu
+makeStatePropagatorDataGpu(const gmx_pme_t &pme);
 //! PME initialization with atom data and system box
-PmeSafePointer pmeInitAtoms(const t_inputrec                        *inputRec,
-                            CodePath                                 mode,
-                            const gmx_device_info_t                 *gpuInfo,
-                            PmeGpuProgramHandle                      pmeGpuProgram,
-                            const CoordinatesVector                 &coordinates,
-                            const ChargesVector                     &charges,
-                            const Matrix3x3                         &box,
-                            std::shared_ptr<StatePropagatorDataGpu>  stateGpu
-                            );
+void pmeInitAtoms(gmx_pme_t               *pme,
+                  StatePropagatorDataGpu  *stateGpu,
+                  CodePath                 mode,
+                  const CoordinatesVector &coordinates,
+                  const ChargesVector     &charges);
 //! PME spline computation and charge spreading
 void pmePerformSplineAndSpread(gmx_pme_t *pme, CodePath mode,
                                bool computeSplines, bool spreadCharges);

diff --git a/src/gromacs/mdtypes/state_propagator_data_gpu.h b/src/gromacs/mdtypes/state_propagator_data_gpu.h
index cf20737bce95cd67031ba8a2cf84dcf0e1a39fec..a7cda77663c5adb0d9df190aa29137e79fd51d0c 100644 (file)
--- a/src/gromacs/mdtypes/state_propagator_data_gpu.h
+++ b/src/gromacs/mdtypes/state_propagator_data_gpu.h
@@ -105,7 +105,11 @@ class StatePropagatorDataGpu
                                const void        *gpuContext,
                                GpuApiCallBehavior transferKind,
                                int                paddingSize);
-
+        //! Move constructor
+        StatePropagatorDataGpu(StatePropagatorDataGpu &&other) noexcept;
+        //! Move assignment
+        StatePropagatorDataGpu &operator=(StatePropagatorDataGpu &&other) noexcept;
+        //! Destructor
         ~StatePropagatorDataGpu();
 
         /*! \brief Set the ranges for local and non-local atoms and reallocates buffers.
@@ -217,7 +221,7 @@ class StatePropagatorDataGpu
     private:
         class Impl;
         gmx::PrivateImplPointer<Impl> impl_;
-
+        GMX_DISALLOW_COPY_AND_ASSIGN(StatePropagatorDataGpu);
 };
 
 }      // namespace gmx

diff --git a/src/gromacs/mdtypes/state_propagator_data_gpu_impl.cpp b/src/gromacs/mdtypes/state_propagator_data_gpu_impl.cpp
index dd0aa76f8002c53b8d0d8fc7882f240cfaf1ea2a..9a119566494fe10a76c63318c608aa468d949ec9 100644 (file)
--- a/src/gromacs/mdtypes/state_propagator_data_gpu_impl.cpp
+++ b/src/gromacs/mdtypes/state_propagator_data_gpu_impl.cpp
@@ -62,9 +62,11 @@ StatePropagatorDataGpu::StatePropagatorDataGpu(const void *       /* commandStre
 {
 }
 
-StatePropagatorDataGpu::~StatePropagatorDataGpu()
-{
-}
+StatePropagatorDataGpu::StatePropagatorDataGpu(StatePropagatorDataGpu && /* other */) noexcept = default;
+
+StatePropagatorDataGpu &StatePropagatorDataGpu::operator=(StatePropagatorDataGpu && /* other */) noexcept = default;
+
+StatePropagatorDataGpu::~StatePropagatorDataGpu() = default;
 
 void StatePropagatorDataGpu::reinit(int  /* numAtomsLocal */,
                                     int  /* numAtomsAll   */)

diff --git a/src/gromacs/mdtypes/state_propagator_data_gpu_impl_gpu.cpp b/src/gromacs/mdtypes/state_propagator_data_gpu_impl_gpu.cpp
index d2c1e5d54f30efd919ba5a824dd81b155ba5bce8..66cdfda9e950e45321d50a54a772178ed9a604b8 100644 (file)
--- a/src/gromacs/mdtypes/state_propagator_data_gpu_impl_gpu.cpp
+++ b/src/gromacs/mdtypes/state_propagator_data_gpu_impl_gpu.cpp
@@ -294,6 +294,10 @@ StatePropagatorDataGpu::StatePropagatorDataGpu(const void        *commandStream,
 {
 }
 
+StatePropagatorDataGpu::StatePropagatorDataGpu(StatePropagatorDataGpu && /* other */) noexcept = default;
+
+StatePropagatorDataGpu &StatePropagatorDataGpu::operator=(StatePropagatorDataGpu && /* other */) noexcept = default;
+
 StatePropagatorDataGpu::~StatePropagatorDataGpu() = default;