Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged more code into common platform #4346

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Conversation

peastman
Copy link
Member

@peastman peastman commented Dec 8, 2023

cc @ex-rzr @philipturner

This is another step in my ongoing project to eliminate as much duplicated code as possible by putting it into the common platform. This PR replaces platform-specific implementations of two more classes with common implementations: BondedUtilities and UpdateStateDataKernel. It eliminates close to 1000 lines of duplicated code. It should allow eliminating a similar amount of code from the HIP and Metal platforms. To make the corresponding changes to them,

  1. Add getAllContexts() to the ComputeContext subclass.
  2. Replace the BondedUtilities subclass with a trivial stub as done for CUDA and OpenCL, or if you don't care about outside code referring to the old class just eliminate it and create a BondedUtilities directly.
  3. Change the kernel factory to create a CommonUpdateStateDataKernel.

@peastman peastman merged commit 5739788 into openmm:master Dec 12, 2023
12 of 15 checks passed
@peastman peastman deleted the common branch December 12, 2023 15:51
@egallicc
Copy link
Contributor

egallicc commented Apr 6, 2024

@ex-rzr :

I built and successfully tested openmm-hip against openmm:master with minor modifications to support this PR. How can I ask for your review and contribute the changes to StreamHPC:develop_stream of openmm-hip? I see that PR #14 still needs to be merged by amd/openmm-hip.

(It looks like AMD is missing in action. Is there any chance we can host the HIP platform code here, @peastman? I understand that we won't be able to distribute HIP-enabled conda packages, but at least we will be able to continue support for HIP for a little bit longer. Interested users can build from sources.)

@ex-rzr
Copy link
Contributor

ex-rzr commented Apr 9, 2024

@egallicc

Do you have a branch with your modifications? I'd like to run benchmark to check if this universal bonded force kernel is slower than the kernel from OpenMM-HIP with some additional optimizations.

@egallicc
Copy link
Contributor

@ex-rzr sorry for the late reply. See https://github.com/Gallicchio-Lab/openmm-hip/tree/butil_common
Note that I wasn't looking to optimize the bonded force. The changes are simply to fix compilation errors of the develop_stream branch against the current OpenMM master branch. (Should this conversation be moved to the openmm-hip repo?) Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants