From: Artem Zhmurov Date: Sun, 15 Aug 2021 13:40:55 +0000 (+0000) Subject: Do not allow AtomLocality::All for coordinates and velocities transfer. X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?a=commitdiff_plain;h=63e96b8597cc4619c6b03524e3e9a7d5915c9f69;p=alexxy%2Fgromacs.git Do not allow AtomLocality::All for coordinates and velocities transfer. 1. Use AtomLocality:Local instead of AtomLocality::All in PME-only ranks and in PME tests. 2. Assert on using AtomLocality::All for positions and velocities transfers, print verbose message on assert. This reduces the number of possible streams used in StatePropagatoerData, hence reduces its complexity. In preparation for buffer state tracking. Refs. #3323 Refs. #3020 --- diff --git a/src/gromacs/ewald/pme_only.cpp b/src/gromacs/ewald/pme_only.cpp index 0e67991867..8e33845218 100644 --- a/src/gromacs/ewald/pme_only.cpp +++ b/src/gromacs/ewald/pme_only.cpp @@ -759,7 +759,8 @@ int gmx_pmeonly(struct gmx_pme_t* pme, pme_gpu_prepare_computation(pme, box, wcycle, stepWork); if (!pme_pp->useGpuDirectComm) { - stateGpu->copyCoordinatesToGpu(gmx::ArrayRef(pme_pp->x), gmx::AtomLocality::All); + stateGpu->copyCoordinatesToGpu(gmx::ArrayRef(pme_pp->x), + gmx::AtomLocality::Local); } // On the separate PME rank we do not need a synchronizer as we schedule everything in a single stream // TODO: with pme on GPU the receive should make a list of synchronizers and pass it here #3157 diff --git a/src/gromacs/ewald/tests/pmetestcommon.cpp b/src/gromacs/ewald/tests/pmetestcommon.cpp index 3d611ca0d0..3eda383e7a 100644 --- a/src/gromacs/ewald/tests/pmetestcommon.cpp +++ b/src/gromacs/ewald/tests/pmetestcommon.cpp @@ -212,7 +212,7 @@ void pmeInitAtoms(gmx_pme_t* pme, stateGpu->reinit(atomCount, atomCount); stateGpu->copyCoordinatesToGpu(arrayRefFromArray(coordinates.data(), coordinates.size()), - gmx::AtomLocality::All); + gmx::AtomLocality::Local); pme_gpu_set_kernelparam_coordinates(pme->gpu, stateGpu->getCoordinates()); break; diff --git a/src/gromacs/mdtypes/locality.h b/src/gromacs/mdtypes/locality.h index 709fcfa3ac..0ddfa883ea 100644 --- a/src/gromacs/mdtypes/locality.h +++ b/src/gromacs/mdtypes/locality.h @@ -62,11 +62,17 @@ enum class AtomLocality : int Count = 3 //!< The number of atom locality types }; -/*! \brief Descriptive strings for atom localities */ -static const EnumerationArray c_atomLocalityNames = { - { "local", "non-local", "all" } -}; - +/*! \brief Get the human-friendly name for atom localities. + * + * \param[in] enumValue The enum value to get the name for. + */ +[[maybe_unused]] static const char* enumValueToString(AtomLocality enumValue) +{ + static constexpr gmx::EnumerationArray atomLocalityNames = { + "Local", "Non-local", "All" + }; + return atomLocalityNames[enumValue]; +} /*! \brief Interaction locality indicator: local, non-local, all. * * Used for calls to: @@ -79,10 +85,17 @@ enum class InteractionLocality : int Count = 2 //!< The number of interaction locality types }; -/*! \brief Descriptive strings for interaction localities */ -static const EnumerationArray c_interactionLocalityNames = { - { "local", "non-local" } -}; +/*! \brief Get the human-friendly name for interaction localities. + * + * \param[in] enumValue The enum value to get the name for. + */ +[[maybe_unused]] static const char* enumValueToString(InteractionLocality enumValue) +{ + static constexpr gmx::EnumerationArray interactionLocalityNames = { + "Local", "Non-local" + }; + return interactionLocalityNames[enumValue]; +} /*! \brief Convert atom locality to interaction locality. * 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 93a0839b9e..28d60f8613 100644 --- a/src/gromacs/mdtypes/state_propagator_data_gpu_impl_gpu.cpp +++ b/src/gromacs/mdtypes/state_propagator_data_gpu_impl_gpu.cpp @@ -85,11 +85,11 @@ StatePropagatorDataGpu::Impl::Impl(const DeviceStreamManager& deviceStreamManage // Note, that nullptr stream is used here to indicate that the copy is not supported. xCopyStreams_[AtomLocality::Local] = updateStream_; xCopyStreams_[AtomLocality::NonLocal] = nonLocalStream_; - xCopyStreams_[AtomLocality::All] = updateStream_; + xCopyStreams_[AtomLocality::All] = nullptr; vCopyStreams_[AtomLocality::Local] = updateStream_; vCopyStreams_[AtomLocality::NonLocal] = nullptr; - vCopyStreams_[AtomLocality::All] = updateStream_; + vCopyStreams_[AtomLocality::All] = nullptr; fCopyStreams_[AtomLocality::Local] = localStream_; fCopyStreams_[AtomLocality::NonLocal] = nonLocalStream_; @@ -121,7 +121,7 @@ StatePropagatorDataGpu::Impl::Impl(const DeviceStream* pmeStream, // This it temporary measure to make it safe to use this class in those cases. xCopyStreams_[AtomLocality::Local] = pmeStream_; xCopyStreams_[AtomLocality::NonLocal] = nullptr; - xCopyStreams_[AtomLocality::All] = pmeStream_; + xCopyStreams_[AtomLocality::All] = nullptr; vCopyStreams_[AtomLocality::Local] = nullptr; vCopyStreams_[AtomLocality::NonLocal] = nullptr; @@ -311,7 +311,12 @@ DeviceBuffer StatePropagatorDataGpu::Impl::getCoordinates() void StatePropagatorDataGpu::Impl::copyCoordinatesToGpu(const gmx::ArrayRef h_x, AtomLocality atomLocality) { - GMX_ASSERT(atomLocality < AtomLocality::Count, "Wrong atom locality."); + GMX_ASSERT(atomLocality < AtomLocality::All, + formatString("Wrong atom locality. Only Local and NonLocal are allowed for " + "coordinate transfers, passed value is \"%s\"", + enumValueToString(atomLocality)) + .c_str()); + const DeviceStream* deviceStream = xCopyStreams_[atomLocality]; GMX_ASSERT(deviceStream != nullptr, "No stream is valid for copying positions with given atom locality."); @@ -376,7 +381,11 @@ GpuEventSynchronizer* StatePropagatorDataGpu::Impl::xUpdatedOnDevice() void StatePropagatorDataGpu::Impl::copyCoordinatesFromGpu(gmx::ArrayRef h_x, AtomLocality atomLocality) { - GMX_ASSERT(atomLocality < AtomLocality::Count, "Wrong atom locality."); + GMX_ASSERT(atomLocality < AtomLocality::All, + formatString("Wrong atom locality. Only Local and NonLocal are allowed for " + "coordinate transfers, passed value is \"%s\"", + enumValueToString(atomLocality)) + .c_str()); const DeviceStream* deviceStream = xCopyStreams_[atomLocality]; GMX_ASSERT(deviceStream != nullptr, "No stream is valid for copying positions with given atom locality."); @@ -408,7 +417,11 @@ DeviceBuffer StatePropagatorDataGpu::Impl::getVelocities() void StatePropagatorDataGpu::Impl::copyVelocitiesToGpu(const gmx::ArrayRef h_v, AtomLocality atomLocality) { - GMX_ASSERT(atomLocality < AtomLocality::Count, "Wrong atom locality."); + GMX_ASSERT(atomLocality == AtomLocality::Local, + formatString("Wrong atom locality. Only Local is allowed for " + "velocity transfers, passed value is \"%s\"", + enumValueToString(atomLocality)) + .c_str()); const DeviceStream* deviceStream = vCopyStreams_[atomLocality]; GMX_ASSERT(deviceStream != nullptr, "No stream is valid for copying velocities with given atom locality."); @@ -428,7 +441,11 @@ void StatePropagatorDataGpu::Impl::copyVelocitiesToGpu(const gmx::ArrayRef h_v, AtomLocality atomLocality) { - GMX_ASSERT(atomLocality < AtomLocality::Count, "Wrong atom locality."); + GMX_ASSERT(atomLocality == AtomLocality::Local, + formatString("Wrong atom locality. Only Local is allowed for " + "velocity transfers, passed value is \"%s\"", + enumValueToString(atomLocality)) + .c_str()); const DeviceStream* deviceStream = vCopyStreams_[atomLocality]; GMX_ASSERT(deviceStream != nullptr, "No stream is valid for copying velocities with given atom locality.");