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

Kokkos port for GRANULAR #988

Merged
merged 39 commits into from Oct 12, 2018

Conversation

Projects
None yet
5 participants
@valleymouth
Copy link
Collaborator

valleymouth commented Jul 5, 2018

Purpose

This pull request allows the use of GRANULAR's pair gran/hooke/history and fix freeze with Kokkos.

Author(s)

Denis Taniguchi (Newcastle University)

Backward Compatibility

This pull request doesn't break backward compatibility.

Implementation Notes

Implementation was based on the bench/in.chute case, i.e. all the classes involved in running that case were ported.
Validation of the porting was also performed based on the bench/in.chute case. Running that case using Kokkos or not doesn't give exact same results but the overall behaviour is maintained (see attached log files).
AtomVecSphereKokkos was implemented to include radius and rmass atom attributes. NPairKokkos was modified to take into account size. FixNeighHistoryKokkos was also ported to account for the displacement history.
We still need to profile the code and check for simple optimisations opportunities, such as shear history memory access pattern. Since the original GRANULAR code relied exclusively on half neighbour list, the same was made in this implementation. Maybe implementing full list support could be an interesting thing to avoid atomic operations, which could lead to better performance specially on old GPUs.

Post Submission Checklist

Please check the fields below as they are completed

  • The feature or features in this pull request is complete
  • Suitable new documentation files and/or updates to the existing docs are included
  • One or more example input decks are included
  • The source code follows the LAMMPS formatting guidelines

Further Information, Files, and Links

chute-regular.txt
chute-kokkos.txt

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Jul 5, 2018

@valleymouth thanks for your contribution, but please note, that your pull request fails to compile with KOKKOS enabled for OpenMP.

@valleymouth valleymouth force-pushed the valleymouth:granular-kokkos branch from 75a5dc2 to 13efc1b Jul 5, 2018

@@ -95,8 +97,14 @@ action fix_deform_kokkos.cpp
action fix_deform_kokkos.h
action fix_eos_table_rx_kokkos.cpp fix_eos_table_rx.cpp
action fix_eos_table_rx_kokkos.h fix_eos_table_rx.h
action fix_freeze_kokkos.cpp

This comment has been minimized.

@stanmoore1

stanmoore1 Jul 5, 2018

Contributor

This and similar files need to depend on the corresponding file in the GRANULAR package.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 5, 2018

@valleymouth thanks for all the work on this. In general it looks good.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 5, 2018

@valleymouth I'm running this through our internal Kokkos regression tests.

Denis Taniguchi
@valleymouth

This comment has been minimized.

Copy link
Collaborator

valleymouth commented Jul 6, 2018

@stanmoore1 Thanks for all your support.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 6, 2018

@valleymouth the Kokkos OpenMP regression tests had some failures. Some were "invalid free", some were segfaults, and some were LAMMPS errors. Here are the failing tests:

[examples/bench', 'chute']
[examples/srd', 'srd.mixture']
[examples/pour', 'pour.2d.molecule']
[examples/pour', 'pour']
[examples/pour', 'pour.2d']
[examples/colloid', 'colloid']
[examples/ASPHERE/dimer', 'dimer.mp']
[examples/ASPHERE/dimer', 'dimer']
[examples/ASPHERE/poly', 'poly.mp']
[examples/ASPHERE/poly', 'poly']
[examples/ASPHERE/ellipsoid', 'ellipsoid.mp']
[examples/ASPHERE/ellipsoid', 'ellipsoid']
[examples/ASPHERE/box', 'box.mp']
[examples/ASPHERE/box', 'box']
[examples/ASPHERE/star', 'star.mp']
[examples/ASPHERE/star', 'star']
@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 6, 2018

GPU tests also had several failures with this error: Cuda const random access View using Cuda texture memory requires Kokkos to allocate the View's memory

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 6, 2018

OK all the OpenMP tests now seem to be running expect pour. @valleymouth can you take a look at that? I'm going to look at GPU now.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Jul 6, 2018

@valleymouth if necessary you can put in a meaningful error message for fix pour if it can't work correctly (no need to port fix pour to Kokkos right now).

@valleymouth

This comment has been minimized.

Copy link
Collaborator

valleymouth commented Jul 6, 2018

@stanmoore1 Ok, I will look into fix pour problem.

@akohlmey akohlmey added this to the Stable Release Summer 2018 milestone Jul 16, 2018

Denis Taniguchi and others added some commits Jul 16, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Aug 2, 2018

Removed conflicts due to the documentation refactoring currently happening in the master branch.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Aug 4, 2018

@stanmoore1 is this ready to be included or would it be better to wait until after the upcoming stable release? Please let me know by COB on Monday. From then on, only bugfix changes will be accepted until after the stable release.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Aug 6, 2018

@valleymouth is currently working on some performance optimizations. @valleymouth what are your thoughts?

@valleymouth

This comment has been minimized.

Copy link
Collaborator

valleymouth commented Aug 6, 2018

@stanmoore1 @akohlmey I think we can wait for the CommKokkos to be able to handle ghost velocities and exchange of fixes data, since it has a major impact in the overall performance.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Aug 6, 2018

@valleymouth that sounds good to me. Thanks for the input.

@rbberger

This comment has been minimized.

Copy link
Member

rbberger commented Aug 21, 2018

Did another merge with master to remove the conflicts

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 27, 2018

@stanmoore1 can you please comment on the status of this? is the needed change that is waiting for going to be available soon? that is, before the next planned stable release in mid/late november?

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Oct 3, 2018

@akohlmey I'd like to get this merged by next month if possible. I've been really busy but now finally have some time to work on this. @valleymouth was optimizing performance by keeping more data on the GPU but had some questions on path forward. I'm going to take a look and get back to him.

Denis Taniguchi added some commits Oct 5, 2018

@valleymouth valleymouth requested a review from sjplimp as a code owner Oct 10, 2018

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Oct 10, 2018

Thanks @valleymouth. @akohlmey I'm running this through the Kokkos regression tests, as soon as those pass this should be ready to merge.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Oct 10, 2018

For some reason the coreshell example hangs with the branch, looking into it.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

stanmoore1 commented Oct 12, 2018

Regression tests now look good, I believe this is ready to merge.

@stanmoore1 stanmoore1 merged commit 00c75ec into lammps:master Oct 12, 2018

5 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/kokkos_omp head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details
@danielque

This comment has been minimized.

Copy link

danielque commented on src/USER-VTK/dump_vtk.cpp in f394ed9 Nov 8, 2018

This preprocessor condition will never be met

This comment has been minimized.

Copy link
Collaborator

valleymouth replied Nov 8, 2018

Because there is no VTK 9?

This comment has been minimized.

Copy link
Member

akohlmey replied Nov 8, 2018

This preprocessor condition will never be met

please explain why.

This comment has been minimized.

Copy link

danielque replied Nov 8, 2018

See #1196

This comment has been minimized.

Copy link
Collaborator

valleymouth replied Nov 8, 2018

You are right! My bad!

This comment has been minimized.

Copy link
Member

akohlmey replied Nov 8, 2018

See #1196

thanks. why not do this right away?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment