Repository navigation
Fix real-space symmetry filtering in GPU contractions - #16
Merged
PeizeLin merged 11 commits intoOct 24, 2026
Merged
Conversation
Apply fac_add_Ds while merging downloaded GPU contributions through the existing tensor accumulation helper. Remove the unused GPU result map and locks; the result merge runs outside the OpenMP packing regions. Add regression coverage for signed, zero and fractional coefficients, seeded outputs, repeated calls and combined contraction labels.
In the four a01b2_a2b01 contractions, Aa2 is an intermediate atom summed into the (Aa01, Ab01) output. Applying filter_for2 to (Aa01, Aa2) drops valid terms. Apply it once Ab01 is available. Add standalone CPU/DDLA regression coverage against unfiltered CPU contractions masked independently by output atom pairs and cells. Cover all 18 labels and their sum, real/complex tensors, empty and seeded outputs, and signed/zero/fractional increments. Validated CPU-only and DDLA v0.0.4/CUDA builds on V100 with 1 and 4 OpenMP threads. The two preceding symmetry fixes are cherry-picked from maki49; this regression additionally catches the four remaining incorrect contraction paths. (cherry picked from commit 2781215)
Contributor
|
Thanks for completing the symmetry fix. The GPU loop-order table below highlights the last four
My backports had mistakenly marked the early The following table shows the behavior of symmetry filters comprehensively for all 18 labels in
|
(cherry picked from commit fe27bb1)
(cherry picked from commit 6534722)
Use the right atom list to bound the density loop and the left atom list to select mu in the CVC loop. Add scalar-reference regressions for distinct and unequal atom partitions.
Copy the input when initializing an empty result with unit coefficient. Tensor assignment shares storage, so subsequent in-place additions otherwise change the input and other accumulators. Cover independent outputs and complex accumulation coefficients.
Upstream now contains the original PeizeLin commits already imported here. Keep the independent const-input accumulator and corrected Hartree atom-list indexing. The merged tree is identical to the previously validated PR head.
Integrate the upstream PeizeLin implementation and the validated accumulator and atom-list corrections while retaining all symmetry filtering changes.
Contributor
Why the CPU behavior is unchanged
|
3 tasks
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.
GPU contractions with real-space symmetry can abort on a valid contraction label or lose contributions. GPU batching uses a different loop order from CPU contractions, so the shared symmetry callbacks must identify the actual output atom pair and relative cell at each call site.
The fix addresses three cases:
a01b01_a2b2contractions so the callback screens(Aa01, Ab01).a01b2_a2b01contractions, move the check inside theAb01loop. Screening intermediateAa2as an output atom had discarded valid terms from the sum.The first two changes backport maki49's initial fix and extension, preserving authorship. The additional intermediate-sum correction has a numerical regression covering all 18 contraction labels and their combined sum, real/complex tensors, periodic cells, restricted/empty output and seeded accumulation. Its reference is an unrestricted CPU contraction with an independent mask on the final output keys.
Both CPU implementations retain their existing filtering rules. For the newly supported label/argument-type combinations, CPU calls different overloads; the symmetry delta leaves the CPU contraction loops unchanged.
This PR includes the accumulator and atom-list corrections from #15. Symmetry changes relative to #15.
The original ABACUS failure was:
Original before/after records show four GPU-symmetry ABACUS cases aborting before the fix and passing after it. Among the 18 standalone contractions, the original version had 10 exceptions and four incorrect outputs; the two backports alone left four incorrect sums; the complete fix passes all 18.
The version merged with current
masterand #15 (7025df6) was validated again:Hardware: NVIDIA V100-SXM2 16 GB GPUs, GCC 13.3/OpenMP and DDLA v0.0.4/CUDA. ABACUS
b30b66a5auses only the test patch removing its GPU-symmetry rejection so execution reaches LibRI; no LibRI patches are applied. The ABACUS fixture has zero forces by symmetry, so it does not establish nonzero-force behavior. MAGMA, BSE numerical results and multi-node execution were not tested.Complete merged-version build and runtime logs. Standalone reproduction. Related: deepmodeling/abacus-develop#8169.