Conversation
…hake The host pack/unpack pair for the SHAKE unconstrained coordinates brings xshake to the host before reading it and claims the host side after writing it. The device pair did neither: the pack read the device copy without bringing it up to date, and the unpack wrote the ghost coordinates there without claiming them. Today the sequence in post_force() hides this, because unconstrained_update() claims the device side immediately before the communication, so the device copy happens to be the current one and the sync that follows the communication has nothing to do. Nothing in either routine relies on that, though, and if the host side is the current one on entry the pack sends whatever the device copy last held and the sync after the communication then discards the ghosts the unpack just wrote. Results are unchanged on a host-only build, where the two sides share an allocation.
ModifyKokkos::energy_couple() and ModifyKokkos::energy_global() marked the atom data listed in a fix's datamask_modify as modified after calling compute_scalar(), but they were the only ModifyKokkos wrappers that did not sync that fix's datamask_read first. When the data was still dirty on the host, marking it modified on the device triggered: Kokkos::DualView::modify_device ERROR: Concurrent modification of host and device views in DualView "atom:f" This is reached with "fix langevin" plus "econserve" in thermo_style and a "minimize" command: MinKokkos::setup() turns auto_sync off, then ModifyKokkos::setup() marks all atom data as modified on the host for the internal MINIMIZE/kk fix (it uses the default Host execution space and ALL_MASK datamasks), and the following thermo output calls energy_couple() which marks atom->f as modified on the device without syncing. Add the missing sync to both routines, matching all other ModifyKokkos wrappers.
FixMinimizeKokkos left datamask_read and datamask_modify at the Fix defaults of ALL_MASK, with the default Host execution space. The fix has setmask() == 0 and overrides none of the callbacks Modify invokes, and it syncs the atom data it does touch itself (reset_coords() syncs X_MASK; grow_arrays(), copy_arrays() and the exchange routines only touch its own vectors). So the masks describe atom data the fix never accesses through Modify. The consequence is that ModifyKokkos::setup(), which loops over all fixes, syncs all atom data to the host and then marks all of it as modified on the host for this fix. MINIMIZE/kk is added by Min::init() and is therefore always the last fix in the list, so nothing in that loop syncs the data back, and MinKokkos::setup() has turned auto_sync off. Every atom array is then left dirty on the host for the rest of minimization setup, which is how the concurrent host/device modification of atom->f reported in issue lammps#5080 came about. Declare both masks EMPTY_MASK, as all other Kokkos fixes that manage their own syncing do.
…tructors The atom_vec_*_kokkos classes cache the per-atom arrays of the corresponding atom->... fields and refresh them in grow_pointers(). Their constructors left those members uninitialized, so the pointers are indeterminate if the object is destroyed or re-grown before grow_pointers() first runs. Coverity flags each constructor with UNINIT (uninitialized pointer field). Initialize the members to nullptr in the constructor initializer lists, in declaration order. AtomVecHybridKokkos already did this for stylesKK. Fixes lammps#5055
PairTIP4PKokkos::prepare() called ev_init() with the default alloc = 1, so Pair::ev_setup() allocated the plain base-class eatom and vatom arrays. prepare() then replaced those pointers with Kokkos dual views via destroy_kokkos()/create_kokkos(); since destroy_kokkos() only clears the pointer, the plain allocations were orphaned and never freed. On a later reallocation Pair::ev_setup() would in addition call memory->destroy() on a Kokkos-owned pointer. Pass alloc = 0, as all other KOKKOS pair styles do, so the per-atom arrays are managed only through k_eatom/k_vatom. This affects tip4p/cut/kk, tip4p/long/kk, lj/cut/tip4p/cut/kk and lj/cut/tip4p/long/kk. Verified with valgrind on all four styles with per-atom energy and virial output enabled; forces, energies and stresses are unchanged. The corresponding leak in pace/kk was already fixed separately. Fixes lammps#5062
FixPropertyAtomKokkos left datamask_read and datamask_modify at the Fix defaults of ALL_MASK, with the default Host execution space, the same way the internal MINIMIZE/kk fix did. The fix has setmask() == 0 and overrides none of the callbacks Modify invokes, and the per-atom arrays it owns are synced through AtomKokkos::sync() and AtomKokkos::modified(), which call into its own sync() and modified() rather than consulting the datamasks. So the masks describe atom data the fix never accesses through Modify. ModifyKokkos::setup() loops over all fixes and therefore marks every atom array as modified on the host for this fix. When "fix property/atom" is the last fix defined, nothing later in that loop syncs the data back, and the atom data stays dirty on the host through thermo output. Reading econserve then marks atom->f as modified on the device and aborts with a concurrent host/device modification, the same failure as issue lammps#5080 but reachable from a plain run, with no minimize involved. Declare both masks EMPTY_MASK, as all other Kokkos fixes that manage their own syncing do.
…ixes
The KOKKOS fixes that keep the per-atom virial in a Kokkos dual view
first let the base class allocate the plain vatom array and then
replaced the pointer with destroy_kokkos()/create_kokkos(). Since
destroy_kokkos() only clears the pointer, the plain allocation was
orphaned and never freed. This is the same defect as in the tip4p/kk
and pace/kk pair styles, where Pair::ev_setup() already takes an alloc
argument to suppress the allocation.
Give Fix::ev_setup() and Fix::v_setup() (and the ev_init()/v_init()
wrappers) the same alloc argument, defaulting to 1 so that all other
fixes are unaffected, and pass alloc = 0 from fix shake/kk, fix
efield/kk and fix wall/region/kk.
For the fix wall styles the per-atom virial was additionally allocated
before FixWall::post_force() called v_init(), i.e. before vflag_atom
was known. On the first invocation with per-atom virial output the
dual view was therefore still empty while the kernels tallied into it,
which segfaulted:
fix wf all wall/lj93 zlo -9.0 1.0 1.0 6.0 units box
compute st all stress/atom NULL fix
Add a v_setup_peratom() hook to FixWall that the accelerated styles
override, so the dual view is (re)allocated after v_init() has set the
flags. fix wall/harmonic/kk, wall/lj93/kk, wall/lj126/kk,
wall/lj1043/kk and wall/morse/kk now reproduce the per-atom virial of
their CPU counterparts exactly.
Also drop a duplicated v_init() call in fix wall/region/kk and correct
two copy-and-paste memory labels in fix shake/kk.
Verified with valgrind on all eight styles: no bytes definitely lost,
no invalid reads or writes. Per-atom and global virial, energy and
thermo output are identical to the plain CPU styles, and the KOKKOS
results of examples/{melt,micelle,indent,min,crack} are unchanged.
…dforce/kk Three defects in fix addforce/kk, found while auditing the per-atom virial handling of the KOKKOS fixes: - The kernels never called v_tally(), which was therefore dead code. Neither the global nor the per-atom virial of the added force was tallied, so the pressure was wrong and compute stress/atom returned zero for the fix contribution. - The every keyword was ignored: post_force() was missing the ntimestep % nevery test, so the force was added on every step. - The sforce array was only grown for varflag == ATOM. With an atom-style energy variable and equal-style force components (varflag == EQUAL, estyle == ATOM) it kept its initial length of one atom while compute_atom() wrote nlocal entries into it, and the values it did receive were never synced to the device. Tally the virial as FixAddForce::post_force() does, honor nevery, and grow and sync sforce for estyle == ATOM as well. Also correct a copy-and-paste memory label. Constant force, equal-style and atom-style force variables, atom-style energy variables and every N now all reproduce the pressure, energy and per-atom stress of fix addforce exactly, and valgrind reports no invalid accesses for the atom-style energy variable case, which previously overflowed the sforce allocation.
FixHalt::~FixHalt() had no copymode guard, but halt/kk runs its bondmax reduction with parallel_reduce(*this): the functor copies' destructor chain freed idvar and dlimit_path (the latter is always allocated), so any "fix halt ... bondmax" run corrupted the heap. Also widen the KK_FLOAT reduction result to double before the MPI_MAX allreduce, which passed a float buffer as MPI_DOUBLE in single/mixed precision builds.
The group variants read the per-atom mask on the device but synced only X_MASK, so a mask changed on the host was not seen by the kernel. The press/berendsen/kk and press/langevin/kk remap() with "dilate partial" are the first callers of these variants.
spring/self/kk, ti/spring/kk, and gjf/kk store their per-atom state in DualViews that are modified on the device during atom exchange, but the base-class pack_restart()/unpack_restart() read and write the host copy. A restart written after a device-side exchange therefore contained stale values. Wrap both restart hooks with the same sync/modify discipline as the exchange hooks. Also drop the bogus modify_host() from the host pack_exchange() wrappers: the base method only reads the array, and claiming a host write forced a needless transfer (flagged by Copilot on the PR). Move the tstyle enum of fix gjf into the class header so the KK variant no longer hardcodes the ATOM enumerator value.
- accumulate the per-bin scatter sums in double regardless of the KOKKOS precision setting (atomic float adds degraded single/mixed builds) - keep persistent host mirrors instead of allocating mirrors every call - bin triclinic boxes on the device with the same kernel: the previous host fallback called bin_assign(), which reads coordinates on the host between the device-side x2lamda/lamda2x conversions and thus operated on stale data on GPU builds - copy the device bin assignment back to the host bin[] array (nlocal ints) so the inherited per-atom remove_bias()/restore_bias() used by non-KOKKOS thermostats see the bins from the last temperature call - replicate compute_array() in the KK class over bin_average_kk() for the same reason
Calling the host-facing restore_bias_all() on a kokkosable temperature compute forced a device-to-host velocity sync every step in temp/csvr/kk and langevin/kk; use the _kk variant like temp/berendsen and temp/rescale do. When the internal temperature compute is recreated because the fix was requested with an explicit /kk suffix, create it with the same host/device flavor as the fix itself instead of always temp/kk.
The port synced X/IMAGE to the host every step for group->xcm() and group->gyration(). Replace both with device parallel_reduce kernels (total mass + mass-weighted unwrapped coordinates, then the gyration sum), eliminating all per-step host transfers. Align the early-return guard with the CPU style (rg == 0 or masstotal == 0) and remove the shadowed atomKK/execution_space members here and in addtorque/group/kk, the two files missed by the earlier shadowed-member cleanup.
fix move, restrain, and deform/pressure are on the do-NOT-port list, so their destructor guards are dead code; fix heat and indent keep theirs (heat is ported now, indent is a planned port).
Also remove the stale claim that fix gjf is not compatible with the KOKKOS package.
compute temp/ramp/kk follows the temp/partial/kk pattern: templated scalar/vector reductions plus device bias removal/restore kernels; the ramp bias is recomputed from the coordinate, stored per atom for the restore. fix heat/kk reduces the group momentum and kinetic energy in one device kernel and applies the velocity rescale/shift on the device; the region keyword and atom-style heat-flux variables produce an error (documented), and the heat_region test skips the KOKKOS variants.
fix indent remains the open Group F target; the device group-reduction machinery for addtorque/group and compute temp/rotate is queued for a follow-up.
fix shake/kk reported zero per-atom energy and virial where fix shake reports real values, and the per-atom virial was additionally lost whenever more than one thread was used. - min_post_force() only accumulated the global virial. Tally the per-atom energy and virial of the substituted bond restraints as FixShake::bond_force() does, through a new device ev_tally() that mirrors Fix::ev_tally(). The energy and virial of a restraint are shared out over the atoms of the pair that this processor owns, so the image indices the kernel uses for the positions are first mapped back onto their owners; a cluster reaching across a periodic boundary is then credited to both atoms, as on the CPU. - post_force() and min_post_force() never contributed the duplicated per-atom virial scatter view back into the dual view, so with more than one OpenMP thread compute stress/atom saw only zeros. - min_post_force() dispatched the kernel on vflag rather than evflag, which skipped the reduction, and with it the restraint energy, whenever only per-atom energy was requested. Verified against the non-accelerated style with 1, 2 and 4 OpenMP threads, for both the MD and the minimization path: global pressure, global energy, and the per-atom energy and stress of every atom agree to round-off (largest relative deviation 3e-11), and valgrind reports no leaks or invalid accesses. The Kokkos kernels do not compute the centroid virial of the constraint forces, but the style inherited centroidstressflag = CENTROID_AVAIL from FixShake and so silently contributed zeros to compute centroid/stress/atom. Set CENTROID_NOTAVAIL, as the KOKKOS angle and dihedral styles do, so that combination is rejected with a clear error instead, and document the restriction.
RegBlockKokkos and RegSphereKokkos kept the list of contacts between a particle and the region surface in a single view owned by the region. surface_kokkos() is called from inside a parallel region, once per particle, so every thread wrote into the same list and read back whichever contacts happened to survive. fix wall/region/kk therefore computed wrong forces as soon as more than one thread was used, and the result was not reproducible from run to run. With four OpenMP threads, 637 of the 1827 per-atom force and stress components of a small test differed from the non-accelerated style, by up to 170%. Pass the contact list in from the caller instead, so that it can live on the stack of the thread that uses it, and add a MAXCONTACT constant to each region class for its length. surface_kokkos(), surface_interior_kokkos(), surface_exterior_kokkos() and the helpers they call no longer modify the region and are now const, so that a shared buffer cannot be reintroduced unnoticed. The device-side contact views and the destructors that freed them are gone. Verified against the non-accelerated style with 1, 2 and 4 OpenMP threads: forces and per-atom stresses of every atom are now bit for bit identical for a block and a sphere region, inside and outside, with open faces, with a moving region, and for the harmonic wall style, and valgrind reports no leaks or invalid accesses. The same race affects the GPU backends, which could not be tested here.
- min quickmin/kk: sync the per-type masses to the device in init() - fix wall/harmonic/outside/kk: size the per-atom virial dual view in v_setup_peratom() with alloc = 0, after v_init() set vflag_atom - compute reaxff/atom/kk: TAG_MASK/EMPTY_MASK datamasks and a host sync of the tags before compute_local(), instead of the ALL_MASK defaults - compute temp/sphere/kk: sync and claim around the bias compute's remove_bias_all()/restore_bias_all(), as temp/deform/kk does - pair mliap/kk: launch the comm packing kernels in RangePolicy<DeviceType> so the host instantiation does not run them on the device - fix nvt/nph/npt/sphere/kk: remove leftover debug output - comm_style tiled (plain and KOKKOS): reverse_comm(Pair*) honored the size argument backwards; match CommBrick - fix shake/kk: pack_forward_comm() no longer claims a host write it does not make; drop the now unused i0 - pair uf3/kk: size the centroid virial with maxcvatom and free it in the destructor - fix propel/self/kk: v_init(vflag,0), the per-atom virial is a dual view - pair eam/kk: size the HIP team scratch with sizeof(KK_FLOAT) - fix addtorque/atom/kk, settorque/atom/kk: release the base class array before the dual view takes it over - fix qeq/reaxff/kk: struct/class tag mismatch on a friend declaration
- fix cmap/kk: tally the global virial and the per-atom energy and virial the way FixCMAP::post_force() does through Fix::ev_tally(): each crossterm split over its five atoms, only the owned ones counted. The per-atom arrays are dual views sized after ev_init(eflag,vflag,0). Before this the fix's virial was zero and compute pe/atom and stress/atom silently omitted the CMAP term. - pair hybrid/scaled under KOKKOS: a pair style that runs on the device but does not implement the KOKKOS packing communicated through a null KokkosBase pointer; both comm classes now take the host path for it - remap/kk: wait for all receives before copying the shared scratch buffer to the device (the grid3d fix), fence the pack kernel before MPI reads the device buffer, copy only the message's own stretch of the host send buffer, and MPI_STATUSES_IGNORE for the Waitall - pair dpd/kk: cache update->ntimestep on the host instead of dereferencing the Update object inside the team kernel - pair pod/kk: drop the commented-out debugging dumpers, whose fwrite of a KK_FLOAT mirror with sizeof(double) was wrong in single precision
Kokkos 5.2 still supports the legacy view implementation; it goes away in 5.3, not before. So there is no need yet to give up 64-bit offsets through Kokkos_ENABLE_IMPL_VIEW_LEGACY, and none of the work that replaced them view by view is needed either. This puts every file back to where it was before that work started, in one commit so that the history of what was tried, and why, stays intact: 4464ab5 use 64-bit index views for neighbor lists, ReaxFF, and short neighbor lists a161e64 stop forcing the legacy Kokkos view implementation 050be9c size the SNAP beta and ninside arrays by the chunk 5d12962 correct the beta and ninside member comments 00dbc72 reject a SNAP chunk size whose arrays cannot be indexed ccb7997 use 64-bit index views for the SNAP zlist and CPU u arrays ed47fe0 use 64-bit index views for the two newly ported short neighbor lists 8cc20ad KOKKOS: let a dual view be indexed with 64-bit arithmetic 1c99065 use 64-bit index dual views for neighbor history and MLIAP 67d8640 let a TransformView wrap a dual view with 64-bit indexing 8be1467 widen the per-atom arrays that overflow at 250 million atoms per rank a28bf7f drop the workaround markers from the 64-bit dual view declarations ace182e, 20f0524 comment fixes on the above 72ce0b1 enforce disabling legacy views with Kokkos 5.2.x The bundled Kokkos_DualView.hpp is byte-identical to Kokkos 5.2.1 again, and KOKKOS.cmake is back to enabling the legacy views. Changes that reached these files from develop in the meantime are kept. The SNAP beta and ninside chunk-sizing, which stands on its own as a memory fix, follows separately.
An option() can be overridden from the command line; a forced cache entry cannot, and this one must not be: without the legacy view implementation a view of more than 2^31 entries is indexed incorrectly. Kokkos 5.2 deprecates that implementation ahead of removing it in 5.3, so also turn off the deprecation warnings it would otherwise emit on every build.
…m list PairSNAPKokkos chunks its per-atom work: SNAKokkos::grow_rij() sizes every array by the padded chunk size and every kernel indexes them with a chunk-relative atom index. beta and ninside were the exceptions -- sized by the full local atom count, a habit inherited from the CPU pair style, where beta really is filled for the whole list in one pass before the force loop. Here ComputeBeta runs inside the chunk loop and compute_yi reads the result in the same iteration, and nothing ever adds chunk_offset to a beta or ninside index, so the rows beyond the chunk are never touched. With the default chunk size of 32768 the allocation was inum/32768 times too large: 22 GB for beta alone at 1e8 atoms per rank and twojmax 8, next to a chunking scheme that exists to keep memory bounded. The same padding_factor that makes the grow_rij() arrays safe for the routines that process yi_batch atoms per thread applies unchanged. Ported from the reverted 050be9c; the results are unaffected.
…r CUDA UVM Kokkos 5.2 no longer has the Kokkos_ENABLE_CUDA_UVM option (deprecated in 4.0 in favor of Kokkos::SharedSpace, removed in 5.2). The equivalent whole-allocation switch is now the internal option Kokkos_ENABLE_IMPL_CUDA_UNIFIED_MEMORY, which allocates all GPU memory as CUDA managed memory. Point the build and Kokkos speed docs at it, note the CUDA 12.2 and Pascal-or-later requirements and that Kokkos treats the option as internal, and mention that it also gives host code direct access to device data for debugging.
…n is set CMake accepts unknown -D settings silently, so a build script that still passes Kokkos_ENABLE_CUDA_UVM=on would configure without unified memory and give no indication of it. Fail early and name the replacement option instead.
- fix spring/kk, fix wall/flow/kk, compute ave/sphere/atom/kk: sync the per-type masses to the device before reading them, instead of relying on the integrator having done it - compute temp/profile/kk: sync the legacy host side of the masses, which is what the host loop reads; sync<LMPHostType>() refreshes the other side - atom map: map_one() claims its host write; map_clear() releases both sides before it overwrites the whole array - compute inertia/kk: the host sync before inertia_extended() now covers the radius, the ellipsoid indices and the bonus data it reads - fix nh/sphere/kk, compute temp/sphere/kk: sync the host copies before the radius/mask loops of init() and dof_compute() - fix rigid/small/kk and its Nose-Hoover variants: v_init(vflag,0), the per-atom virial is a dual view the fix zeroes itself - pair gran/hooke/kk, gran/hooke/history/kk, gran/hertz/history/kk: stop claiming and copying a per-atom energy that is never written - comm_style tiled/kk: MPI_STATUSES_IGNORE for Waitall, and take the send pointer after the pack that may resize the buffer - NBin gets a copymode guard, set by nbin/kk and nbin/ssa/kk while a copy of the object lives in a device functor; likewise MLIAP_SO3Kokkos - region ellipsoid/kk: rotate() locals no longer shadow the semi-axes - pair lj/gromacs/kk: drop the cut_inner dual views nothing reads
Update Bundled Kokkos library in LAMMPS to v5.2.0
pair_style hybrid/scaled/kk kept its accumulators as plain host arrays, so every sub-style needed the forces (and torques) copied back and forth between host and device on each timestep. Do the work in Kokkos kernels instead: - fsum/tsum become dual views and the save/clear/scale-add/restore steps run as parallel_for kernels - init_style() picks the memory space for the sum: the device when every sub-style runs there, so the forces stay resident on the device, and the Kokkos host space otherwise - virial_fdotr is only synced to the device when it is actually needed Per-atom scale factors from an atom-style variable are now communicated through KokkosBase::pack/unpack_forward_comm_kokkos. Previously the forward communication of a Device pair style dispatched to the device path and dereferenced a null KokkosBase, which crashed in compute() and in single(). The variable itself is still evaluated on the host, so the local values are uploaded and the ghost values filled in by the exchange; single() and born_matrix() read them back from the dual view. Verified against pair_style lj/cut with sub-style scale factors summing to one (constant, equal-style, and spatially varying atom-style), with atom_style sphere for the torque path, with a mix of /kk and plain sub-styles, and against the CPU hybrid/scaled for compute group/group. The force-style unit tests pass in double/legacy-layout and in mixed-precision/default-layout builds.
Respa::init() called into the styles' KOKKOS execution spaces itself and refused any force style on the device. Move the decision into KokkosLMP::respa_check(), so that respa.cpp keeps a single guarded call and the dummy interface a no-op, and make the criterion what rRESPA actually needs: every participant on the host side of the atom data. That admits a build without a device backend as before, and now also a device build whose styles all use the /kk/host suffix with comm, sort and atom/map on the host; it refuses a style, fix or compute in the device execution space and device-side communication, sorting or atom map, and names what to change. Drop the versionchanged directive from the run_style documentation: the KOKKOS package never supported rRESPA, so nothing changed for users.
KOKKOS: fix host/device coherence bugs and include-group binning
…pass The pack functor of fix spring/self/kk ignored the "final" flag of the Kokkos parallel_scan it runs in. The scan invokes the functor once to accumulate the buffer offsets and once more to do the actual packing. Since the functor also refilled the slot of the departing atom i with the data of atom j in the first pass, the final pass then packed the reference coordinates of atom j instead of those of atom i, and the receiving process tethered the migrated atom to the wrong position. With the OpenMP backend, 2 or more MPI ranks and "package kokkos comm device" (or "comm host"), a tethered LJ liquid of 2048 atoms at T=3 blew up within 50 steps, with a spring energy per atom of 21.5 instead of 0.28. Do the packing and the refill only when "final" is set, as fix shake/kk and fix neigh/history/kk already do. The results with 1, 2 and 4 ranks and all "comm" settings now agree with the non-accelerated fix to round-off. Same fix as maintenance commit fb24de6.
The test driver did not finalize Kokkos, so it was torn down by static destructors at program exit. With the Serial backend the default execution space instance then frees its scratch memory through a fence on the execution space registry, which has already been destroyed at that point, and the test segfaulted after all its checks had passed. Finalize the KOKKOS package explicitly after running the tests, like the force-style, c-library and FFT3d test drivers do.
…into collected-small-changes
Collected small changes and fixes
Update version strings for stable release candidate
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.