Fix AMD wave barrier in HIP kernels; add NequIP-OAM-L test problems - #217
Draft
asglover wants to merge 2 commits into
Draft
Fix AMD wave barrier in HIP kernels; add NequIP-OAM-L test problems#217asglover wants to merge 2 commits into
asglover wants to merge 2 commits into
Conversation
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.
Fixes #211 (see also mir-group/nequip#610).
Problem
On AMD (MI250X / gfx90a via ROCm), the weight gradient of NequIP-OAM-L's 23-instruction main-layer
TensorProductConvwas wrong in float32 atomic mode (relative error ~0.5 on one instruction,32x2e x 1x3o -> 32x3o uvu). Energies were unaffected, so the symptom was non-conservative forces in MD. CUDA, the other NequIP-OAM sizes, float64, and deterministic mode were all fine.Root cause (found by @kavanase on Frontier): on HIP,
__syncwarp()was rewritten to__threadfence_block(), which is a memory fence, not an execution barrier. The compiler was free to reorder shared-memory accesses across it inside one unrolled backward segment.Fix
jinja_utils.py: on HIP,syncwarpnow expands to a release fence,__builtin_amdgcn_wave_barrier(), and an acquire fence, keeping the memory-ordering semantics and adding a real wavefront barrier.Test
The failing tensor product was not in the test suite. This PR adds:
NequIPTPPinbenchmark/problems.py, taken from nequip'sInteractionBlock. It emits the same instruction set asChannelwiseTPPbut in nequip's original order.ComputationSchedulenever sorts instructions and greedily cuts segments in the given order, so the order determines segment boundaries and the generated kernel. Using nequip's order reproduces the exact kernel that miscompiled.nequip_oam_problems(): the three distinct NequIP-OAM-L problems (first layer, main layers, last layer), built from theLpreset innequip/model/nequip_models.py. Instruction 16 of the main-layer problem is32x2e x 1x3o -> 32x3o uvu, matching the reporter's breakdown.TestProductionModelsinbatch_test.pyandconv_test.py. The conv suite (atomic mode, F32) is the one that reproduces the report.Status
jinja_utils.pychange and pass after; that has not been run here.🤖 Generated with Claude Code