The fp8 A8 GEMM stages its operands through TMA: prefill +2.7% at chunk 1024 and +4.7% at 4096 - #167
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…pair Neroued#167/Neroued#160, dylan wave 2 (7dd98fd), gzenz safety net (08636ed), md fp8-KV campaign, T22-T27 plan
Neroued
left a comment
There was a problem hiding this comment.
This review is posted by an agent on behalf of @Neroued. The comments below have been reviewed by the maintainer.
Reviewed commit: 3d6f7f2e71e729c9235ef35c8e1878224a59145b. This is a static review of the implementation and the submitted evidence; no local compilation, tests, or profiling were performed for this review.
The optimization direction is sound, and the TMA kernel is worth keeping. I would like the following addressed before merging.
-
Make the shared-memory alignment requirement of the swizzle indexing explicit.
In
fp8_a8_tma.cuh,fp8_tma_shared_byte()computes the 64-byte swizzle from relative row coordinates, implicitly assuming a zero swizzle base offset. The shared storage and dynamic shared backing only declare 128-byte alignment. The 64-byte swizzle pattern repeats at a 512-byte boundary; an otherwise valid 128-byte-aligned base can require a nonzero offset. Please either guarantee 512-byte alignment for the relevant backing, keeping the stage/subtile offsets consistent with it, or incorporate the actual shared base offset into the indexing.This is a missing source-level correctness precondition, not a claim that the submitted binary has already produced incorrect output. A particular compiler layout can satisfy the stronger alignment incidentally. See NVIDIA's swizzle offset description.
-
Keep the routing policy compact and distinguish calibration from a performance guarantee.
The wave-count comparison is a reasonable heuristic, and a non-monotonic admission frontier is not inherently a problem. However, the
0.936coefficient is fitted at one shape/width and shared across different K dimensions and epilogues. The model does not represent pipeline fill or epilogue cost, and the floor plus the5120x6144ceiling compensate for measured cases it cannot distinguish.Empirical tuning for the RTX 5090 fits this project's scope; this is not a request for a general model covering other GPUs. Please simplify the large calibration/assertion section in
fp8_a8_schedule.cuh: retain geometry/resource invariants and necessary coverage checks, and move the detailed measurement history to a maintained performance reference. Assertions that a predicate keeps returning a particular answer do not establish that the selected route remains faster. Express the applicable calibration domain and known bounds clearly. -
Reconsider making the old kernel's bitwise behavior a permanent routing constraint.
The new kernel handles a partial trailing token tile, but
fp8_a8_tma_applies()additionally requirestokens % MmaSchedule::kBlockTokens == 0to match the oldFullTokensbranch's observed floating-point contraction behavior. That couples the new route's performance coverage to a private property of the old implementation.Please distinguish a measured performance reason for excluding these widths from a restriction imposed only to simplify
memcmp. The long-term qualification criterion is the production Op against its independent mathematical oracle at the defined semantic boundaries. Bitwise comparison with the previous kernel is useful supporting evidence, but should not become an additional public numerical contract. I am not asking to remove the gate without evidence; I am asking for its long-term justification to follow the Op contract and performance requirements. The comments also currently describe bit identity more strongly than the PR body's toolchain-dependent caveat warrants. -
Reuse the existing mbarrier helpers.
The PR adds
fp8_mbarrier_init,fp8_mbarrier_wait,fp8_mbarrier_arrive, andfp8_mbarrier_arrive_expect_tx, while its base already provides the corresponding functionality insrc/ops/common/mbarrier.cuh, used by the NVFP4 TMA implementation. These operations have no FP8-specific semantics. Please reuse the existing helpers so synchronization fixes and maintenance have one owner. This does not require introducing a generic GEMM framework.
The performance explanation should also describe the actual combined change: TMA delivery, a larger output tile, and a different warp decomposition. It is more than an operand-copy substitution:
- The output tile changes from
64x128to256x128. - The consumer warp tile changes from
32x32to64x64, increasing accumulator storage and operand reuse. - Two stages of K=128 become four stages of K=64: the nominal buffered K extent remains 256.
- Dedicated producer/consumer synchronization replaces the old K-loop's repeated CTA-wide synchronization pattern.
These changes provide credible mechanisms for the reported speedup, while also explaining the occupancy and small-shape tradeoffs. They can change repeated operand requests and cache behavior, so unchanged logical inputs do not prove unchanged physical memory traffic. Likewise, bandwidth derived from assumed bytes divided by time is not a DRAM counter measurement; the unreestablished peak denominator and the cold-operator versus inference discrepancy limit the roofline/bottleneck conclusions. Please narrow those claims accordingly. This does not by itself invalidate the reported elapsed-time improvements.
The reported default-chunk prefill gain is worth pursuing. The shared kernel body with Op-specific epilogue/output policies, the explicit paired SwiGLU handling, guarded tail accesses, and reuse of the existing workspace are good aspects of the implementation. Keeping the old route for widths where it is needed is justified. Per-launch descriptor construction and repeated device queries can remain follow-up optimization opportunities; this review has no evidence that they should block this change.
Add a second implementation of the FP8 row-scaled A8 GEMM and route to it where a cost model finds it cheaper. The arithmetic is unchanged: same accumulator, same m16n8k32, same scale application, same epilogue and output policies. Five call sites reach it - linear, linear_add, linear_swiglu, attn_input_proj, gdn_input_proj - all at prefill widths. What differs is the CTA's shape and how it is fed. The output tile grows from 64 to 256 tokens and the consumer warp tile from 32x32 to 64x64, so the MMA issued per ldmatrix goes from 8 to 32. Feeding that tile from one CTA needs a deeper, narrower pipeline - four stages of K=64 in place of two of K=128, the buffered K extent unchanged at 256 - and a dedicated producer warp issuing cp.async.bulk.tensor under an mbarrier pair, in place of the CTA-wide barrier the old K loop takes on every stage. Occupancy falls from two CTAs of 256 threads to one of 288, which is why narrow shapes are declined. The 64-byte TMA swizzle is a function of the shared-memory address, not of an offset relative to the destination, so a tile base must sit on a 512-byte swizzle atom. Measured: the same tile loaded 128 bytes past an atom boundary de-swizzles wrongly in all 1024 of its bytes. That alignment is declared on the storage, the union, the outer storage and the allocation, with static asserts that both stage strides and a paired block's second branch are whole atoms. Routing is a comparison of two quantised costs - waves needed times the work one SM carries through a wave - with one empirical constant for this part, plus a width floor and one per-geometry ceiling the model cannot express. The sweeps behind all three, the alignment measurement, and what each bound costs are in docs/maintainer/fp8-a8-tma-route.md. Measured on an RTX 5090 (sm_120a, 170 SMs, 525 W) against the route it replaces, three repetitions with the arms alternating inside each: operator, linear: 0.877 to 0.991 where the route is taken operator, linear_add: 0.895 to 0.978 where the route is taken end to end, qwen3.8-27b prefill: +2.70% at chunk 1024, +4.69% at 4096, +3.96% at 8192 decode: +0.03% over four repetitions, and the route launches zero times in a decode-only kernel census taken with CUDA graphs expanded Output is not bitwise identical to the route it replaces. Where the width is a whole number of the old kernel's token tiles the two match byte for byte; at other widths they round differently by construction, and the numerical tests check those against a host reference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3d6f7f2 to
0159162
Compare
|
Thank you. All four are addressed, and the branch is rebased onto The first item turned out to be a defect rather than a documentation gap, so I will lead with that. 1. Shared-memory alignment of the swizzle indexingYou were right, and it is stronger than "the source does not state the precondition": the swizzle And
2. Routing policy: calibration separated from the codeThe measurement history is out of the header and into You are right that the assertions could not carry what they looked like they carried. The ones that The constant's description now says what the model represents — wave quantisation, and nothing else: 3. The multiple-of-tile condition is goneMeasured, and the answer is not the one I expected. Swept over 1024..1520 in steps of 16 on 32 widths, 8 of them whole cp.async tiles. The condition declined the other 24:
The 8 whole tiles were never its business: the model takes 1024, 1408 and 1472 and declines 1088, It never rescued a width the model would have wrongly taken, and it cannot: the model's only Confirmed on the shipped predicate rather than inferred. Re-swept after the removal, the same 32 That admits between 1134 and 5292 further widths per geometry over 1024..8192. They are not The consequence for the description: this change is no longer bitwise identical to the route it 4. mbarrier helpersRemoved. The route uses Worth recording why this was more than redundancy: the copies used plain labels ( The description of what changesYou are right that "the operand copy is staged differently" is not the change. Four things move
The wider warp tile is the arithmetic change: an The same table is the cost. Resident warps per SM fall from 16 to 9, so a shape with too few token Claims withdrawn
None of that touches the elapsed times. RebaseFour files conflicted on One thing that is not mine to fix
The number of failing cases is not stable across runs — 34 in one, 36 in another — while their form |
A second implementation of the FP8 row-scaled A8 GEMM, and a cost model that routes to it where it
is cheaper. The arithmetic is untouched: same accumulator, same
m16n8k32, same scale application,same epilogue and output policies. Five call sites reach it —
linear,linear_add,linear_swiglu,attn_input_proj,gdn_input_proj— all at prefill widths.This revision answers the four items in your review; the reply below says what changed for each. It
is rebased onto
487f8977and every number here was re-taken on that base.What actually changes
Not one operand-copy mechanism swapped for another. Four things move together and only the last is
about TMA:
The wider warp tile is the arithmetic change: an
ldmatrixof A now feeds eight N fragments insteadof four and a B fragment feeds four M fragments instead of two, so MMA issued per operand load goes
from 8 to 32. The deeper, narrower pipeline and the dedicated producer are what let one CTA keep
that tile fed without the per-stage CTA-wide barrier.
The same table is the cost. Resident warps per SM fall from 16 to 9, so a shape with too few token
tiles to fill the machine pays that and gets none of the reuse back. That is why narrow widths are
declined, and it is what the cost model has no term for.
Per SM the route uses fewer of both budgeted resources than the one it replaces: 98816 B of shared
against 100352 B, and 166 x 288 registers against 94 x 512.
Measured
One RTX 5090, sm_120a, 170 SMs, 525 W. Clock locking is not available on this machine and the card
idles at 180 MHz, so every table is taken with the arms alternating inside each repetition.
Operator, ratio of this route to the route it replaces, three repetitions, per-repetition spread
under 0.012 throughout:
5120x17408linear_add5120x6144linear_add5120x17408linear5120x6144linear14336x5120linear16384x5120linear34816x5120linearA cell at 1.000 is a width the predicate declines, so both arms run the same kernel — the bounds are
visible in the same numbers that measure the gain.
End to end. Of the published artifacts only qwen3.8-27b carries
FP8_E4M3FN_ROW_BF16Sweights; qwen3.6-27b is nvfp4 and qwen3.6-35b-a3b is groupwise-int end to end, so this route does
not run on either and measuring there returns 1.000 by construction.
ninfer_bench -p 16384, sixrepetitions alternating:
±is the sample standard deviation over the six runs; each percentage is the mean of the sixpaired ratios, so it does not reproduce exactly from the rounded cells.
Decode is not on this path, and that is a census rather than an assumption.
-n 512with--cuda-graph-trace=nodeso graph contents are visible: 29 distinct kernels execute andfp8_a8_tma_kernelis not among them, 0 launches. Decode throughput on the same artifact moves+0.03% over four repetitions, ratios 0.9999 to 1.0006.
Suite and tools, on the tree this pushes:
ctest: 113 of 114 pass. The one failure isninfer_attn_input_proj_test, and every one ofits failing cases is
W8 DFlash2 A16 ... graph phase=1; it fails the same way on clean487f8977. No FP8 case fails in either.compute-sanitizer, ten runs, 0 errors in every one:memcheckat 1024, 1345, 4001 and 4096— the last two are widths that are not a whole cp.async token tile, which is where the partial
trailing tile reads past the token extent;
racecheck,synccheckandinitcheckat 256 and321, narrow because they instrument every shared access and a full-width run does not finish.
4096, both arms in both orders: 12 identical, 0 different, 0 invalid. Read that narrowly:
these prompts are chunked into widths that are whole cp.async token tiles, which is where the two
kernels agree byte for byte anyway. It is evidence that nothing else moved, not evidence of
bit-identity in general — see item 3 in the reply.
Routing
A comparison of two quantised costs — waves needed times the work one SM carries through a wave —
with one empirical constant, plus a width floor and one per-geometry ceiling the model cannot
express. The constant is for this part and is not portable; what travels is the shape of the
comparison.
The sweeps behind the constant and both bounds are in
docs/maintainer/fp8-a8-tma-route.md, addedby this change and indexed from
docs/README.md. That document also carries the shared-memoryalignment measurement and the sweep that removed the multiple-of-tile condition, and it records two
things the bounds cost: the floor gives up four measured gains of 8 to 12 percent to avoid one loss
of 2, and the ceiling on
5120x6144costs plainlinear3 to 5 percent while being correct forlinear_add, which is that geometry's production call site.What this does not claim
unchanged physical traffic, and I did not measure traffic.
it is the column
ninfer_linear_benchpublishes, and that is a model, not a counter.machine, and the cold-operator bench does not reproduce the inference-time picture.
token tile the outputs still match byte for byte; at other widths the two kernels round
differently by construction, and the numerical tests check those against a host reference.
Notes
The rebase kept the six-schedule ladder added to
fp8_attn_input_a8.cuafter this branch's originalbase; the TMA route is the wide arm of that ladder, beside
Prefill, which is the schedule thepredicate compares against. The three conflicting test files keep their existing cases and append.
ninfer_attn_input_proj_testfails on487f8977without this branch, in the W8 DFlash2 graphreplay. Reproduced from clean master with both arms run from one build directory; the number of
failing cases varies between runs, which is itself worth knowing. Details in the reply below.
🤖 Generated with Claude Code