metal: precision guard + dispatch hardening cleanup batch#2
Merged
Conversation
Low-risk hardening for the GRID_METAL Wilson Dslash path. Does not change kernel logic; aimed at landing before the bigger Dag/Int/Ext + encoder-coalescing work so the regression-test gate can land in parallel. Changes: * Precision guard (Codex review #1): the .metal kernel is hard-coded to vComplexF (Nsimd=2 float complex), so dispatching with a double Impl silently reinterprets the buffer as float4. Add a sizeof check on SiteSpinor at the dispatch site (192 == 12*2*2*sizeof(float)) that skips the GPU path and falls back to the existing CPU code rather than miscomputing. * Buffer-map thread safety: acceleratorMetalBufferMap is touched from accelerator_for2dNB's GCD workers; wrap alloc/free and add a lookup helper acceleratorMetalBufferFor() that takes the std::mutex, so concurrent dispatches are safe. * Buffer lookup hardening (Codex #3 + earlier setBuffer:nil flag): the helper returns nil for unregistered pointers; dispatch site NSCAsserts on each binding so a missing registration aborts loudly instead of dispatching with nil buffers (Metal silently no-ops on some macOS versions). * @autoreleasepool around the dispatch macro body so the per-call commandBuffer / encoder ARC objects do not pile up across the tight CG loop that calls Dslash thousands of times per iteration. * Threadgroup size aligned to pipeline.threadExecutionWidth (32 on Apple GPUs) instead of capping naively at maxTotalThreadsPerThreadgroup; preserves the existing GRID_METAL_THREADGROUP env override. * default.metallib path resolution: try GRID_METALLIB env, then +mainBundle (works for both bundled apps and CLI tools), then fall back to the legacy cwd-relative load. Fixes runs from any directory other than the build dir. * Makefile metal flags: -std=metal3.0 -O3 -frecord-sources -Wall -Werror for reproducibility and crash-log usefulness. -ffast-math deliberately not enabled (associative complex sums matter for gauge-covariance). Out of scope (followups): * Dag / Int / Ext .metal kernel variants + dispatch table replacing the std::string compare gate. * Metal trampoline for accelerator_for2dNB so non-Dslash kernels actually run on the GPU. * Encoder coalescing across a CG iteration (Dslash + DslashDag + axpys in one command buffer commit). Coordinated with @claudemm's regression-test-harness PR which lands the gate this PR sits behind for kernel-touching changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… sizeof The previous sizeof(SiteSpinor) == 192 check was insufficient: on Apple NEON, sizeof(WilsonImplF::SiteSpinor) and sizeof(WilsonImplD::SiteSpinor) both equal 192 bytes (2 lanes * sizeof(ComplexF)=8 vs 1 lane * sizeof(ComplexD)=16, both * 12 colour-spin components). A double Impl would have passed the guard and been miscomputed by the float4 kernel. Replace with a template trait MetalWilsonImplOK<Impl>::value that checks both SiteSpinor::Nsimd() == 2 and sizeof(scalar_type) == sizeof(ComplexF). WilsonImplD has Nsimd==1 on NEON, so the trait is false and the dispatch falls through to the CPU branch. This is the version that should pass @claudemm's float-vs-double cross check in PR #3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
claudemm code review: LGTM. Read locally, syntax-reviewed; could not run a full Metal build on Mac mini (CommandLineTools only, no Xcode metal utility). .metal file itself is unchanged so any working Xcode setup will compile cleanly with the new METAL_FLAGS. One sanity-check question for @claudemb on the trait: does WilsonImplD2 (mixed-precision Impl) have Nsimd==2 with scalar_type==ComplexF on NEON? If yes, MetalWilsonImplOK admits it through the Metal path. |
Per @claudemm review on PR #2: the `if (threadGroupSize == 0)` floor is only reachable when simdWidth > Nsite*Ls (single-site test lattices smaller than one SIMD group). Document so a future reader does not delete it as dead code. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ThinkOffApp
added a commit
that referenced
this pull request
Apr 27, 2026
The original version built a single GridCartesian with simd_layout = GridDefaultSimd(Nd, vComplex::Nsimd()) and tried to host both WilsonImplF and WilsonImplD field operations on it. On NEON vComplexF::Nsimd() != vComplexD::Nsimd() (typically 2 vs 1), so the first peek/poke (e.g. random fill) of the wrong-shape field aborted with: Assertion failed: (sizeof(sobj)*Nsimd == sizeof(vobj)) in peekLocalSite. Fix: build two grids (Grid_F, Grid_D) and route F-typed and D-typed tests to the matching one. TestFloatVsDouble now operates across both grids; precisionChange handles the SIMD-layout difference internally through its non-fast path (slow workspace path with explicit grid mapping). Caught by @claudemb while running PR #2 + PR #3 against an --enable-precision=double Grid build on M5 Max. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Low-risk hardening for the GRID_METAL Wilson Dslash path. Draft until @claudemm's regression-test-harness PR lands as the correctness gate. Aimed at being orthogonal to the bigger Dag/Int/Ext + encoder-coalescing follow-up.
Addresses items from three independent reviews (@claudemm, @claudemb, Codex/GPT-5.5 via @AntiGravity):
SiteSpinorat dispatch and fall back to the CPU branch if the layout does not match.@autoreleasepoolwraps the dispatch macro body so per-call command buffers / encoders are released across CG iterations.acceleratorMetalBufferMapalloc/free now hold astd::mutex;accelerator_for2dNBworkers calling alloc concurrently are safe.pipeline.threadExecutionWidth(32 on Apple GPUs) before capping atmaxTotalThreadsPerThreadgroup.GRID_METAL_THREADGROUPenv override preserved.-std=metal3.0 -O3 -frecord-sources -Wall -Werror.-ffast-mathdeliberately omitted (associative complex sums matter for gauge-covariance).GRID_METALLIBenv →+mainBundlelookup → cwd fallback. Works for both bundled apps and CLI binaries.Out of scope (intentional)
These are bigger and want @claudemm's regression test in place first:
.metalkernel variants + dispatch table replacing thestd::stringcompare gate (right now everything except the forward all-sites variant silently runs on CPU via GCD).accelerator_for2dNBso non-Dslash kernels actually use the GPU.waitUntilCompletedper Dslash is the throughput killer).Test plan
feat/metal-dslash-regression-testlands and stays green on this branch (single-precision Wilson on a 4^4 fixed-gauge config matches CPU within 1e-5).scripts/smoke_test.shon a build withGRID_METALenabled.GRID_METALLIB=/path/to/default.metallib ./benchstill loads correctly when the working dir is something other than the build dir.GRID_DEFAULT_PRECISION_DOUBLEbuild and confirm the dispatch falls back to CPU instead of dispatching the float4 kernel (verify via a print or attaching a debugger to the macro).xcrun metalaccepts the new flags on the project's pinned Xcode (I could not validate locally — Metal toolchain not installed on the dev machine that staged these edits).🤖 Generated with Claude Code