The q8 mul_mm double-buffer stamps derive from the tensor template; a census cell pins each stamp's stage width - #4018
Conversation
…the K chunk width and the staging tile are its constants, the two Db stamps override them and keep their dispatch names, and the emitted globals unify on the tensor kernel's name; the AIR of all 299 Metal kernels is identical before and after, and ledger row 143 goes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rough the emitter's census, the double-buffer gate sets the threadgroup length from the stamp it dispatches, the Metal ledger's fold census drops the landed item, and five checklists fix what applying them surfaced: a template constant fed to a helper is the sanctioned shape, a named ledger item counts as a row, an emission-identical in-body change needs no board rows and no parity run, the exit rule reaches non-timing reports, a gate reading another stamp's threadgroup global updates with it, and the per-step chunk is a shape constant rather than the reduction width Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a non-attaching mid-file //! doc comment that violates the repo’s comment-hygiene convention and should be changed to //.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors the dasLLAMA Metal q8 mul_mm double-buffer stamps to reuse the tensor template (parameterized by K chunk width and staging tile size), and adds a device-free test cell that pins each stamp’s stage width via the emitter census string. The PR also updates relevant review/checklist docs to reflect stamp-level threadgroup-memory coupling and emitter routing rules.
Changes:
- Folded the q8
mul_mmdouble-buffer template intoMetalQ8MulMmTensorTvia new@template_constantparameters (BK,TWB), and updated dispatch/pso wiring to the new emitted_t_mslglobals. - Updated the GEMM gate to set threadgroup-memory length from the specific stamp being dispatched, and added a census-based test to assert each stamp’s stage width.
- Refined multiple module checklists/ledgers/architecture notes to codify the refactor’s invariants and review expectations.
File summaries
| File | Description |
|---|---|
| modules/REVIEW.md | Broadens shader-emitter review routing to include kernel-generating classes and member declarations. |
| modules/REVIEW_SHADER_EMITTERS.md | Clarifies runtime reduction width vs per-step chunk as a shape constant for tmm2d_*. |
| modules/dasLLAMA/tests/test_metal_gemm_kernels.das | Updates q8 double-buffer gate dispatch globals/tgmem selection; adds census-based width test. |
| modules/dasLLAMA/tests/REVIEW_KERNEL_CELLS.md | Adds rule for stamp-specific @workgroup size changes affecting gates that read another stamp’s *_tgmem. |
| modules/dasLLAMA/REVIEW.md | Tightens wording and clarifies ledger-row deletion/editing rule; minor checklist phrasing tweaks. |
| modules/dasLLAMA/REVIEW_GPU.md | Clarifies GPU checklist routing and defines when parity evidence is required for emission-identical kernel-body refactors. |
| modules/dasLLAMA/followup_metal.md | Removes the now-addressed q8 mul_mm double-buffer fold item reference. |
| modules/dasLLAMA/followup_general.md | Deletes the completed follow-up ledger row describing the q8 mul_mm template fold. |
| modules/dasLLAMA/dasllama/dasllama_metal_prefill.das | Introduces BK/TWB constants in tensor template; rehomes Db stamps onto it; updates PSO compilation globals. |
| modules/dasLLAMA/benchmarks/REVIEW.md | Clarifies which changes require benchmark record updates and strengthens “no results => non-zero exit” rule. |
| modules/dasLLAMA/benchmarks/matmul/occupancy_report.das | Updates the q8 dbuf stamp to use the new emitted _t_msl globals. |
| modules/dasLLAMA/ARCHITECTURE_GPU.md | Updates architecture text to reflect Db forms as stamps (BK=128) rather than a separate staging template. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… does not attach Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level GPU kernel stamp/template structure and associated dispatch/gate wiring, which is high-impact even with strong evidence of byte-identical emission.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Why. The q8 mul_mm tensor and double-buffer kernel classes carried the same eight-line body, one stage width apart, so every fix had to land twice.
What changes.
Observable behavior. None - refactor only; emitted kernels are byte-identical.
Where to look. The two
override BK/override TWBstamps indasllama_metal_prefill.dasand the new width cell intest_metal_gemm_kernels.das.Validation, claims, ledger
Validation
_tgmemcompanions read 0x4800 on both Db stamps and 0x3000 on the tensor stamps after the fold, the values the deleted template carried.run.das -- --suite kernels --arm width,gemm: green; the gemm corpus cell dispatches both Db stamps (kdim 256, 288 and 512, the 288 arm exercising the tail chunk).run.das -- --changed(llm, infra): 97 files, green but fortest_vulkan_dec_tailandtest_vulkan_tier, the M5's standing Vulkan class-rail declines (32 KB shared-memory cap, same 2+3 errors on master).override TWB = 6144on the 128-row Db stamp reds the gemm corpus cell; droppingoverride BK = 128ureds the new width cell and nothing else.static_ifguard; the skipped lanes (docs, cpp, interp, jit, aot) ran green once each.requirescontract for kdim % 64) is a false positive: the tensor form handles a partial last chunk, and the gemm cell's kdim 288 arm proves it against the fp64 oracle.benchmarks/matmul/occupancy_report.dashas no test reaching it; it compiles clean with the renamed globals.Not done
REVIEW.dasgate for a C++ module'sModule::requirematching its CMake links, and the kernels-suite pinned-gate registry moving out oftests/REVIEW.mdinto a cited section.modules/has no ARCHITECTURE.md for its checklist's opening slot; whether to add one is an owner's call.