Optimize the GPU pairlist of the selfcoordnum kernel - #954
Merged
Conversation
Assisted-by: Claude Code v2.1.233 (Opus 5)
Assisted-by: Claude Code v2.1.233 (Opus 5)
HanatoK
force-pushed
the
opt_selfcoordnum_gpu_pairlist
branch
from
September 1, 2026 21:48
d516bc1 to
2163fcd
Compare
jhenin
approved these changes
Sep 3, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The uninitialized pair mask can omit valid interactions, and the GPU test lacks intended gradient validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Optimizes GPU selfCoordNum pairlists with warp-sized tile bitmasks and adds a 104-atom regression test. Depends on PR #940.
Changes:
- Adds 32/64-lane tile masks and tiled GPU pairlist processing.
- Integrates tile-mask allocation and kernel dispatch.
- Adds large-pairlist trajectory, state, and force references.
File summaries
| File | Description |
|---|---|
tests/selfcoordnum-pairlist-large.in |
Defines the large pairlist test. |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/test.in |
Configures the functional test; missing debugGradients on (moderate). |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/AutoDiff/test_out.colvars.traj |
Adds trajectory reference data. |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/AutoDiff/test_out.colvars.state |
Adds state reference data. |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/AutoDiff/test_out_forces_4.dat |
Adds step-4 force references. |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/AutoDiff/test_out_forces_3.dat |
Adds step-3 force references. |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/AutoDiff/test_out_forces_2.dat |
Adds step-2 force references. |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/AutoDiff/test_out_forces_1.dat |
Adds step-1 force references. |
tests/input_files/selfcoordnum-pairlist-large_harmonic-fixed/AutoDiff/test_out_forces_0.dat |
Adds step-0 force references. |
tests/build_tests.sh |
Registers the new test. |
src/cuda/colvarcomp_coordnums_kernel.h |
Defines tile masks and extends kernel APIs. |
src/cuda/colvarcomp_coordnums_kernel.cu |
Implements tiled processing; pairMaskSelf is used uninitialized (critical). |
src/colvarcomp_coordnums.h |
Tracks pairlist enablement. |
src/colvarcomp_coordnums.cpp |
Manages tile-mask memory and GPU execution. |
Review details
Suppressed comments (1)
src/cuda/colvarcomp_coordnums_kernel.cu:822
- On pairlist rebuilds,
pairMaskreaches the bit-update at line 877 without any prior assignment. Because each update reads the previous value, the flattened tile mask stored at line 890 is indeterminate; initialize it to zero before populating it so subsequent reuse cannot skip valid pairs.
PairMaskT pairMask;
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
It looks like that the Github stacked PRs do not support PRs across forks, so I have to create this PR targeting
master. This PR depends on #940.This PR uses bit fields to store the pairlist data of
selfcoordnum, and implements the pairlist in a tile-based manner. Specifically, the atoms are grouped in tiles, and the tile size matches the warp size (NVIDIA) or wavefront size (AMD) of the GPU. The following struct (seecolvarcomp_coordnums_kernel.h) is used to record the interacting atom pairs between two tiles:This design brings two benefits:
To ensure the correctness, this PR also adds a new test of
selfcoordnumwith a large atom group "Protein" that has 104 atoms, and enablepairListFrequency 2to verify the generation and use of pairlist.