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

Initial changes to compile on AMD #1346

Merged
merged 15 commits into from
Aug 5, 2022
Merged

Conversation

jglaser
Copy link
Contributor

@jglaser jglaser commented Jun 13, 2022

Description

Make HOOMD compile on AMD architectures with HIP

Compiles, but not yet functional

Motivation and context

Resolves #1101

How has this been tested?

Change log


Checklist:

@jglaser jglaser requested review from a team, b-butler and Charlottez112 and removed request for a team June 13, 2022 21:30
Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many seemingly unrelated casts, as well as many places where code is commented rather than removed.

hoomd/md/ForceComposite.h Outdated Show resolved Hide resolved
hoomd/md/EvaluatorTersoff.h Outdated Show resolved Hide resolved
hoomd/md/EvaluatorTersoff.h Outdated Show resolved Hide resolved
@jglaser
Copy link
Contributor Author

jglaser commented Jun 27, 2022

There are many seemingly unrelated casts, as well as many places where code is commented rather than removed.

removed the obsolete commented code for now, which pertained to external library depencies such as hipsparse (for cusparse). Will have to see if unit tests complete for the distance constraint force in this case, and if not, we can add back any needed rocM dependencies later.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale There has been no activity on this for some time. label Jul 18, 2022
@jglaser
Copy link
Contributor Author

jglaser commented Jul 18, 2022

Can this be merged? Do you need me to address any more comments?

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jens! I like the nice clean CMake code.

The OPLS unit test is failing. I hope my suggested change fixes the test. Aside from that, are there any updates needed to the documentation on HIP support?

hoomd/md/OPLSDihedralForceCompute.cc Outdated Show resolved Hide resolved
Co-authored-by: Joshua A. Anderson <joaander@umich.edu>
@jglaser
Copy link
Contributor Author

jglaser commented Jul 19, 2022

Thanks Jens! I like the nice clean CMake code.

The OPLS unit test is failing. I hope my suggested change fixes the test. Aside from that, are there any updates needed to the documentation on HIP support?

We might have to document/enforce the minimum CMake version for HIP language support (3.21).
https://cmake.org/cmake/help/latest/command/enable_language.html

And, only the MD module has been tested.

@joaander
Copy link
Member

Thanks Jens! I like the nice clean CMake code.
The OPLS unit test is failing. I hope my suggested change fixes the test. Aside from that, are there any updates needed to the documentation on HIP support?

We might have to document/enforce the minimum CMake version for HIP language support (3.21). https://cmake.org/cmake/help/latest/command/enable_language.html

OK, let's do that. Does the ROCm minimum requirement also need updating?

And, only the MD module has been tested.

That's fine. This PR is just making the initial changes. We will do more testing in future work.

@joaander
Copy link
Member

pre-commit.ci autofix

@joaander joaander added validate Execute long running validation tests on pull requests and removed stale There has been no activity on this for some time. labels Jul 19, 2022
@joaander
Copy link
Member

I added the CMake variable HOOMD_GPU_PLATFORM which lets the user choose between CUDA and HIP. Users may have HIP installed on a system but prefer to use the CUDA backend.

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This is ready to merge with the final tweaks I made to the CMake scripts and documentation.

@joaander joaander requested review from b-butler and removed request for Charlottez112 July 21, 2022 14:45
Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a couple comments.

hoomd/md/BondTablePotential.cc Outdated Show resolved Hide resolved
CMake/hoomd/HOOMDHIPSetup.cmake Outdated Show resolved Hide resolved
jglaser and others added 2 commits July 29, 2022 11:47
Co-authored-by: Brandon Butler <butlerbr@umich.edu>
Co-authored-by: Brandon Butler <butlerbr@umich.edu>
@joaander joaander requested a review from b-butler August 1, 2022 14:53
@joaander joaander enabled auto-merge August 3, 2022 17:43
@joaander
Copy link
Member

joaander commented Aug 4, 2022

@b-butler Do you approve?

@joaander joaander merged commit 6019b70 into glotzerlab:trunk-minor Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility of HOOMD with the newest ROCm
3 participants