Repository navigation
feat(EPv2): forward a per-token scale row with dispatch - #593
Merged
Merged
Conversation
The a8w4 MoE wants fp8 activations, but dispatch ships bf16 and the receiver quantizes once per received copy while it gathers. Quantizing on the sender instead halves the wire and does the work once per local token -- but it needs the e8m0 scale row (hidden/32 = 224 B at hidden 7168) to travel with the token, and there was no channel for it. This adds one: an opaque, fixed-width per-token row that dispatch forwards to the same recv slot as its token. mori does not quantize or interpret it. Cfg.scaleBytes compile-time width, 0 = off EpArgs.scalesBuf source rows EpArgs.offOutScales destination region Off costs nothing. Render omits default-valued fields, so a scaleBytes=0 Cfg renders byte-identical text to before -- and that text IS the JIT cache key, so existing users neither recompile nor select a different kernel. Both device bodies carry it. The portable body copies straight to the peer, as it already does for the payload. The gfx1250 body stages the row and ships it through TDM as a fourth metadata field, because a 224 B per-(token,destination) transfer is the size TDM is worst at. EpDispatch1250xSlabBytes gives the per-warp LDS slab a floor when scales are on. The slab is sized off the payload dtype and shared with the metadata tile, so an fp8 payload halves it exactly when per-token metadata grows 52 -> 276 B; without the floor the metadata batch collapses ~11x (dispatch 36.7 -> 93.1 us at 512 tokens/rank on gfx1250). Combine never carries scales: it moves post-expert tokens, which are already in the combine dtype. MakeEpCfg zeroes scaleBytes for the combine kernel. plan_api binds an arena region the caller does not carry to offset 0 rather than raising, so a backend that does not use the optional out_scales region needs no placeholder. The kernel's own `if constexpr` is what keeps a 0 offset from being dereferenced. Tested on gfx1250 (EP4, hidden 7168, topk 6) end to end through aiter's MegaMoE: byte-exact against a torch reference on every live row and scale byte. CI gains a step that runs test_op.py with SCALE_DIM=32 on the HIP backend, which is also the only hardware coverage the portable body gets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FieldCount.MatchesEpCfg hardcodes the number that the header's MORI_JIT_ASSERT_FIELD_COUNT already guards at compile time, so adding EpCfg.scaleBytes left the two disagreeing and only the runtime test caught it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cument the staging footprint plan_api: only a region on an explicit optional list may be missing. The blanket `except KeyError -> 0` turned a typo in region_names, or a backend that forgot a required region, into a silent bind to offset 0 -- which aliases the first region, so the kernel would scribble over it. Callers that build their own arena and pass a region_names mapping are exactly the ones able to get this wrong. An optional region also becomes mandatory again when the Request field that enables it is set, which turns "the backend must check" from a comment into a mechanism. ep_cfg: EpDispatch1250xSlabBytes was capped by EpCombine1250xLdsBudget. The two are the same number, but for a shared physical reason, not because dispatch follows combine -- retuning combine would have silently resized the dispatch slab. Added Ep1250xLdsBytes for the physical ceiling and pointed both at it. The bare `* 2` is now EpElemSize(EpDType::Bf16), and the comment says the floor is an empirical result from one shape rather than a general rule. ep_intranode_1250x: the scale staging pool is quadratic in world_size (worldSize * EpMaxRecv, and EpMaxRecv is worldSize * maxTokPerRank) and is a __device__ global, so it is one copy per compiled variant: ~224 MiB at EP8/maxTok 16384/224 B, ~672 MiB for a three-entry schedule. Documented with the numbers, and a static_assert now fails the build at 1 GiB instead of letting a wider EP discover it at launch. Making it world_size-independent needs the staging re-indexed block-locally, which wants hardware validation. hip_backend: reject a per-token scale row that is not a whole number of dwords. The kernel walks the source rows at the padded dword stride, and _scale_i32 rounds up, so the Cfg validator could not see a caller row of, say, 30 B. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jhchouuu
requested review from
kawhil-amd and
zhangfei829
and removed request for
zhangfei829
August 24, 2026 07:49
The bind loop derives a region key from the C++ launch argument
(offOutScales -> "outScales"), but _OPTIONAL_REGIONS and
_REGION_REQUIRED_WHEN were written with the arena's spelling
("out_scales"), so the lookup never matched and the escape hatch
re-raised. A scales-off HIP op -- the default, and what CI runs first --
died in EpDispatchPlan with KeyError: 'out_scales'. Scales-on never hit
it: the arena carries the region then, so offset() does not raise.
Fold both spellings through _camel_to_snake before the lookup. Both
tables, not just the first: with only _OPTIONAL_REGIONS fixed the
enabler lookup would miss and a scaleBytes>0 op with no region would
silently bind offset 0 -- which aliases region 0 -- instead of raising.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aller A scale transfer is a run of consecutive destination slots, so the SLOT stride is what decides whether a run starts on a 128 B boundary -- and TdmWholeOrSplit128 only gives a body to the aligned part of a run. At the natural hidden/32 = 224 B only every 4th run starts aligned; padding to 256 B moved dispatch from 157.7us to 94.8us at 512 tokens/rank. That padding used to be the caller's job, which meant every caller repacked its quantizer's output into a 128 B-strided buffer before handing it over -- a kernel per dispatch to rearrange bytes the transport was about to touch anyway. mori derives the stride instead (EpScaleStride), so Cfg.scaleBytes goes back to meaning "the row the caller has" and the pad is applied where the rows are already being copied one at a time: the gfx1250 staging step, and the portable body's per-token copy. Derived, not a second Cfg field: the field count is unchanged, and a scaleBytes=0 Cfg still renders byte-identical text. The pad is written as zero rather than left as whatever the last launch put there. Nobody reads it, but it crosses into a peer's memory, and "nobody reads it" is not something a buffer diff or a future consumer knows. Padded on every arch, including the portable one that gains nothing from the alignment: a destination layout that changed with the arch would fork every consumer of it, and the pad is ~14% of a side channel that is itself ~3% of an fp8 payload. recv_scales() hides the pad (a strided view of the meaningful dwords), so the existing test compares what it always did. A consumer that takes the base pointer -- a receiving kernel -- asks for scale_stride_bytes() instead. Compile-checked: 6/6 gfx1250 plans and 3/3 portable (gfx950) plans emit .hsaco with MORI_EP_SCALE_BYTES 224 / MORI_EP_SCALE_STRIDE 256. Not yet run.
The scale row exists for fp8/fp4 dispatch, and until now no test had ever run the two together: test_op.py sets one data_type for both legs, and the hip backend refuses an fp8 combine (it sums), so its SCALE_DIM coverage could only ever be bf16. The asymmetric test is where a quantizing wire already lives, so the scale check belongs here. Same reverse-map bijection test_op.py uses: each token's row carries rank*100003 + tok, and the recv side decodes where it should have come from. A row that lands in the wrong slot, or is read at the wrong pitch -- which is what a stride/padding bug looks like -- fails as a mismatch rather than as noise. Verified on 4x gfx1250: fp8 and fp4 at scale_dim=224 (the real MX width at hidden 7168, which mori pads to 256), plus 32 (pads 4x), 256 (already aligned, must not grow) and 260 (not a power of two). bf16 + SCALE_DIM=32 still passes.
The existing scales step can only be bf16: test_op.py gives dispatch and combine one dtype and the HIP backend refuses an fp8 combine, so the row that only a quantizing wire sends was never tested on one. test_asym_dtype.py is where fp8/fp4 dispatch with a bf16 combine already lives, and it now checks the forwarded rows. SCALE_DIM=224 rather than a round number: it is hidden/32 at hidden 7168 and it is NOT a multiple of 128, so the run covers the padding mori adds to the destination stride. An already-aligned row would leave that untested. The runner has no gfx1250, so this exercises the portable body -- the half of the scale transport that cannot be reached by hand on the TDM boxes.
… the contract Follow-ups from review of 033283f. The behaviour is unchanged; what changes is that the things it silently relied on are now either enforced or written down. The two backends no longer lay `out_scales` out identically -- HIP pads each row to 128 B, FlyDSL does not -- while the comment above the region table still said one arena serves either. It does not, and nothing would catch it: a plan binds offsets, never sizes, so an arena sized for FlyDSL and handed to HIP is overrun by its last tokens. The comment now says so and names scale_stride_bytes() as the thing external sizing must go through. scale_stride_bytes() moves to the base op. It exists because a receiving kernel needs the real pitch, and a consumer should not have to branch on the backend to ask; the base answers "the row you gave me", which is exactly right for a backend that does not pad. dispatch()'s docstring gained the other half: out_scales is not always contiguous, so a caller reading it through a raw pointer must stride by the pitch rather than by the shape. The alignment the padding buys rested on two unstated assumptions. The destination one is SymmArena._ALIGN >= EpScaleAlign, two constants in different files with nothing connecting them -- now asserted where the region is sized, because lowering it would surface as a ~1.7x dispatch regression rather than as a failure. The source one is the staging array's base address, which TdmWholeOrSplit128 needs aligned for the same split it uses on both ends; __align__ makes that free and checkable instead of accidental. EpScaleStride's comment claimed the pad costs ~14%. That is true at 224 B and nowhere else: 32 B pads 4x and a 4 B row 32x, and the gfx1250 staging pool is worldSize^2 * maxTok * stride per compiled variant. Single layout is still the right call, and the 1 GiB static_assert bounds the extreme, but the table is in the comment now. The parity test carries scales for the first time -- it is the one place the two backends' physical layouts diverge, and the contract that recv_scales() hides that was untested. Comparing them exposed a second thing: the backends do not agree on WHICH recv slot a token lands in, so the rows must be compared in source-token order via the routing reverse map. Every other check in that test was already order-free (it compares combine's output); a raw row-by-row compare of a dispatch buffer is not, and fails on the ordering rather than on the values. Verified on 4x gfx1250: asym fp8 and fp4 with a 224 B row, test_op bf16 with 32, and parity flydsl-vs-hip with 224 -- byte-exact across the two pitches.
The comments on the scale transport had grown into prose. Trimmed to the parts that are not recoverable from the code: the measured numbers, the constants in other files that this one depends on, and the reasons a reader would otherwise have to rediscover. Net -72 lines, no behaviour change. One real edit came with it: the staging array's length is now a named constexpr, because clang-format could not fit `__device__ __align__(...) unsigned char _cusplit_stgScale[...]` on a line and wrapped it into something unreadable. Re-verified on 4x gfx1250 after rebuilding: asym fp8 and fp4 with a 224 B scale row, backend parity with the same, and test_op bf16 with 32.
#599 reworked the gfx1250 dispatch (six perf changes made unconditional, -233 lines) and touched plan_api's argument binding, which is where both halves of this branch live. Five conflicts in ep_intranode_1250x.hpp, all in the same two categories: * the scale transport itself -- the staging pool, the 4th metadata field, the scale row in perTokM -- kept, since main cannot have it. #599's own edit to perTokM only removed a "(no scale)" comment, and its tokCap constant 384 is 128*3, which this branch already generalised to 128*kMetaFields. * comments #599 deleted that this branch still carried only by not touching those lines. Taken from main. plan_api merged cleanly: the optional-region binding and the new args-struct cache are in different parts of the same loop.
…ormat's reach The TDM header pulls in driver_types.h, which uses hipMemoryType without declaring it, so it only compiles after hip_runtime.h. It used to sit in its own block, which is what kept it there -- #599 removed the comment that separated them, leaving all three includes in one sortable group, and the next clang-format run put it first alphabetically. Every gfx1250 JIT compile then failed with four "unknown type name 'hipMemoryType'" in a ROCm header. Nothing on main is broken today; it is one formatter run away from being broken, which is why this is a blank line and a reason rather than a reordering. Found because a JIT cache wipe turned it into a compile failure -- with a warm cache the kernels never get recompiled and the tree looks fine. Verified from a cold cache: asym fp8 and fp4 with a 224 B scale row, backend parity, and test_op bf16, all on 4x gfx1250.
kawhil-amd
added a commit
to kawhil-amd/mori
that referenced
this pull request
Oct 8, 2026
FieldCount.MatchesEpCfg has been red since the count was last bumped to 12 in ROCm#593. combineFp4, combineFp4F32Scale and selfFirst landed after that and nobody moved it, so the assertion fails on origin/main as it stands -- unrelated to any branch that happens to be in CI when it is noticed. Bumping it to 15 would just reset the same clock. The invariant the assertion exists for is between two independent derivations: FieldCount probes the struct, VisitFields walks the hand-written MORI_FIELD list. A literal is a third number beside them that only ever reports its own staleness, and it hides the real failure -- a field reaching the struct but not the walk renders a Cfg text missing that field, which keys the JIT cache to the wrong kernel. So count the walk. VisitFields calls the visitor once per entry; comparing that to FieldCount tests what the comment above it always claimed. Checked against today's struct: both are 15, and all 15 are walked, so nothing was actually missing -- only the literal was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kawhil-amd
added a commit
to kawhil-amd/mori
that referenced
this pull request
Oct 8, 2026
FieldCount.MatchesEpCfg has been red since the count was last bumped to 12 in ROCm#593. combineFp4, combineFp4F32Scale and selfFirst landed after that and nobody moved it, so the assertion fails on origin/main as it stands -- unrelated to any branch that happens to be in CI when it is noticed. Bumping it to 15 would just reset the same clock. The invariant the assertion exists for is between two independent derivations: FieldCount probes the struct, VisitFields walks the hand-written MORI_FIELD list. A literal is a third number beside them that only ever reports its own staleness, and it hides the real failure -- a field reaching the struct but not the walk renders a Cfg text missing that field, which keys the JIT cache to the wrong kernel. So count the walk. VisitFields calls the visitor once per entry; comparing that to FieldCount tests what the comment above it always claimed. Checked against today's struct: both are 15, and all 15 are walked, so nothing was actually missing -- only the literal was. Co-Authored-By: Claude Opus 5 (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
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.
What
Adds a per-token scale row to EP v2 dispatch: an opaque, fixed-width row that travels with its token to the same recv slot. mori does not quantize or interpret it — it is a forwarding channel.
Why
The MoE wants quantized activations. Today dispatch ships bf16 and the receiver quantizes once per received copy while it gathers — a token routed to
topkpeers is quantizedtopktimes, on the same values. Quantizing on the sender does it once per local token and shrinks the wire, but only if the e8m0 scale row can travel with the token.At hidden 7168 that is a 224 B row, and it buys: bf16 14336 B/token/destination → 7424 with an fp8 payload, 3840 with an fp4 one.
The stride is mori's, not the caller's
A caller states the row it has; mori states the row it lays down, and the two differ by padding to 128 B.
This is not tidiness. A scale transfer is a run of consecutive destination slots, so the slot stride decides whether a run starts on a 128 B boundary — and
TdmWholeOrSplit128only yields abodyfor the part of a run that does; the rest degrades to the scalar global→global fallback. At the natural 224 B only every 4th run starts aligned. Measured at 512 tokens/rank, hidden 7168, EP4 on gfx1250: dispatch 157.7 us at 224 B, 94.8 us at 256 B.Deriving it here rather than making the caller pad also deletes a kernel on the caller side — the padding lands where rows are already copied one at a time (the gfx1250 staging step, the portable body's per-token copy), so it costs nothing there. The pad bytes are written as zero rather than left as whatever the last launch put in the staging pool: nobody reads them, but they cross into a peer's memory, and "nobody reads it" is not something a buffer diff or a future consumer knows.
The padding applies on every arch, including the portable body that gains nothing from the alignment: a destination layout that changed with the arch would fork every consumer of it, and the pad is ~14% of a side channel that is itself ~3% of an fp8 payload.
Anything addressing the region itself must use the stride.
recv_scales()returns a strided view of the meaningful dwords, so a caller that just wants the values never sees the padding; a caller handed the base pointer asksscale_stride_bytes().What "off costs nothing" does and does not mean
Renderomits default-valued fields, so ascaleBytes=0Cfg renders byte-identical text to before. No new specialisation, no behaviour change for anyone not using the feature.It does not mean nobody recompiles. The JIT cache key is
name + toolchain signature + nic + flags + IncludeTreeHash(sourceDeps) + code, andEpSourceDepscoversinclude/mori,src/ops/dispatch_combine_v2andsrc/cco— every header this PR touches. So the first use after upgrading recompiles once, for everyone.That is correct behaviour rather than a cost. This PR inserts two fields into the middle of
EpArgs(offOutScales,scalesBuf), which shifts every offset after them. If the cache key really were just the rendered text, ascaleBytes=0user would get a kernel compiled against the oldEpArgspaired with new host-side offsets, and read silently wrong arguments.IncludeTreeHashis what prevents that.Notes for review
Both device bodies carry it, differently. The portable body copies straight to the peer, as it already does for the payload. The gfx1250 body stages the row and ships it through TDM as a fourth metadata field.
One number in two places is checked, not assumed. The stride reaches the device as a macro (the staging array is at file scope, before
kCfgexists) and is recomputed from the Cfg inside the body; astatic_assertrequires them equal. The same shape of bug — a constant restated where the compiler cannot see both — has already bitten this file once.EpDispatch1250xSlabBytesgives the per-warp LDS slab a floor when scales are on. The slab is sized off the payload dtype and shared with the metadata tile, so an fp8 payload halves it exactly when per-token metadata grows 52 → 276 B. Without the floor the metadata batch collapses ~11x — measured as dispatch 36.7 → 93.1 us at 512 tokens/rank. The floor is an empirical result from that one shape: the mechanism is general, the choice of the bf16 width as the target is not necessarily so for a very different hidden size. Note it is computed fromCfg.hiddenDim, which an fp4 caller has already halved, so an fp4 wire gets half the floor an fp8 one does — worth re-measuring if fp4 becomes the common case.The scale staging pool is quadratic in
world_sizeand is a__device__global, so it is one copy per compiled variant: ~64 MiB at EP4 / maxTok 16384 / 256 B stride, ~256 MiB at EP8 / 16384, and ~672 MiB for a three-entry (block, warp) schedule. The per-peer stride has to be the peer's full recv capacity so the destination slot id indexes it directly, which is also what lets the existingab + cc > recvCapMguard cover the new array —_stgCapdoes not bound it, and the two cross over asworld_sizegrows. Making itworld_size-independent needs the staging re-indexed block-locally, which would turn that guard from "skip this transfer" into "drop these tokens" and wants hardware validation. This PR therefore documents the footprint and adds astatic_assertthat fails the build at 1 GiB rather than letting a wider EP discover it at launch.Combine never carries scales. It moves post-expert tokens, already in the combine dtype;
MakeEpCfgzeroesscaleBytesfor the combine kernel.plan_apionly lets an explicitly optional region be missing. A blanketexcept KeyError -> bind 0would turn a typo inregion_names, or a backend that forgot a required region, into a silent bind to offset 0 — which aliases the first region, so the kernel would scribble over it. An optional region also becomes mandatory again when the Request field that enables it is set (scaleBytesforout_scales), so "the backend must check its arena" is a mechanism rather than a comment.Ep1250xLdsBytesis new.EpDispatch1250xSlabBytesused to be capped byEpCombine1250xLdsBudget: the same number, but for a shared physical reason rather than because dispatch follows combine, so retuning combine would have silently resized the dispatch slab.The source scale row must be a whole number of dwords. The kernel walks it at a dword stride, so the Cfg validator cannot see a caller row of, say, 30 B;
_unsupportedchecksscale_dim * scale_type_sizedirectly.Testing
Hardware, 4x gfx1250 (
test_asym_dtype.py,MORI_V2_KERNEL_BACKEND=hip): fp8 and fp4 dispatch carrying a 224 B scale row, plus widths 32 (pads 4x), 256 (already aligned, must not grow) and 260 (not a power of two) — all byte-exact through the routing reverse map, at 8 / 64 / 512 tokens. bf16 + scales still passes.End to end through aiter's MegaMoE (EP4, hidden 7168, topk 6): moving the quant to the sender is bit-identical — a8w4 scores 0.002174 against a torch fp32 reference on both the bf16 and the fp8 wire, a4w4 scores 0.042041 on both bf16 and fp4. Per-layer wall clock at 16384 tokens/rank, medians of 3 interleaved runs with disjoint ranges: fp8 −5.9% (4654 → 4378 us), fp4 −6.4% (3512 → 3286 us); 512 tokens is parity. Per-kernel attribution puts all of it in dispatch (449 → 197 us/layer for fp4) with the two GEMMs and combine unchanged.
CI gains two things.
test_op.pywithSCALE_DIM=32on the HIP backend — without it the new channel had zero CI coverage. Andtest_asym_dtype.pywithSCALE_DIM=224, fp8 and fp4, becausetest_op.pygives dispatch and combine a single dtype and the HIP backend refuses an fp8 combine — so it can only ever test scales on a bf16 wire, which is not the wire the row exists for. 224 is deliberate: it is hidden/32 at 7168 and it is not 128-aligned, so it exercises the padding.Both CI steps run the portable body: the runners have no gfx1250, so the TDM body can only be exercised by hand.
🤖 Generated with Claude Code