GROMACS

Gerrit received a related patchset '5' for Issue #2817.
Uploader: Szilárd Páll (pall.szilard@gmail.com)
Change-Id: gromacs~master~Ib87dabd74a02727898681249691ac9786b8ac65c
Gerrit URL: https://gerrit.gromacs.org/9169

Gerrit received a related patchset '3' for Issue #2817.
Uploader: Szilárd Páll (pall.szilard@gmail.com)
Change-Id: gromacs~master~I80c95438e44167b6a9d9d74c27709379f6665867
Gerrit URL: https://gerrit.gromacs.org/9170

Regarding splitting this up: I am starting to work on a new patch which implements only the GPU Force buffer ops, without the PME reduction.

Gerrit received a related patchset '1' for Issue #2817.
Uploader: Alan Gray (alang@nvidia.com)
Change-Id: gromacs~master~Ice984425301d24bac1340e883698244489cd686e
Gerrit URL: https://gerrit.gromacs.org/9275

Alan Gray wrote:

Regarding splitting this up: I am starting to work on a new patch which implements only the GPU Force buffer ops, without the PME reduction.

Have you considered also decoupling the GPU-side reduction with CPU forces? This would allow the change to go in independently from all other changes required. Also, if you keep the reduction, do consider changes I76e4b954b4ef045f299a8496b4975497720f4b89 (https://gerrit.gromacs.org/#/c/9126/) and Ie49c0fc483b274ac17e6ace9ca495c11dc719532 (must be a draft).

OK, thanks - I will think about this and adjust the new patch accordingly.

I tried to add links to Szilard's text, but one of the patches is a draft

Mark Abraham wrote:

I tried to add links to Szilard's text, but one of the patches is a draft

Yes, this was one of the set of changes I uploaded as suggested improvements to the (then single) buffer ops change. The mini-branch of recommendations were kept as drafts and were never intended to undergo code review, so they would just be noise on the already noise gerrit site. If there is interest I can share them wider, but the content should anyway end up in new changes intended fr review.

F buffer ops (without PME reduction) patch now updated to include vectorization within kernel and use of haveCpuForces flag, which is currently always set to "true" such that H2D transfer is activated before buffer op, awaiting force workload patch for avaiability of (haveSpecialForces || haveCpuBondedWork).

I considered further splitting this to separate the buffer ops and reduction, but it doesn't really improve isolation because haveCpuForces would still be required to determine if an extra reduction across GPU forces and CPU forces is required.

Now awaiting review.

Now rebased such that (haveSpecialForces || haveCpuBondedWork) is available. But we can't use it just yet to make the H2D transfer conditional until the PME reduction is also integrated into the buffer ops. So still setting haveCpuForces=true for now.

• on virial steps PME forced are needed separately
• for DD separate short-range forces are needed separately

For this reason, I suggest to add in a separate change, a code-path/kernel flavor that does out of place short-/long-range force reduction. It would be good to have a child F buf ops redmine too track technical issues with the feature.

• on virial steps for now turn off GPU buffer ops instead of shuffling around data to cater for the CPU-side virial reduction
• next, port the virial reduction to the GPU (likely best called from / fused with the reduction kernel); see current code in sim_util.cpp:calc_virial().

- the transform+reduce kernels can use simple or atomic accumulation into a reduced f output buffer; the former will require exclusive access to the target force buffer (need to wait for the completion of any kernel that produces forces into it) while the latter would only require a wait on the source force buffer(s) to be reduced into the target (e.g. GPU NB and/or CPU force buffer).
- consider inline transform function for on-the-fly transform within the nonbonded kernel; in particular for high parallelization the performance hit in the nonbonded kernel may be less than the cost of launching an extra kernel.

Have these points been moved somewhere or considered and somehow ruled out?

Related TODOs:

• Ideally the force-reduction should not be called from a method of the nonbonded module (especially due to the complexities of CPU/GPU code-paths) - consider reorganizing reductions

Has this been moved elsewhere?

I had already moved the TODO into https://redmine.gromacs.org/issues/3370 ("Force buffer op and reduction cleanup/improvement" section) - I've now also pasted the other points there too. These are still relevant and hadn't been forgotten, on Monday Artem and I actually discussed re-working of the force buffer ops and the need for a new purpose built object that can perform reductions on an arbitrary number of input buffers, wait on an arbitrary number of events etc. There is an open question as to whether this object should also handle the transform or that should be separate. The latter has the disadvantage of a separate kernel call, unless we go for the "on-the-fly" option which might be the best solution. Your thoughts are very welcome on this.