Repository navigation
Offset the per-head bounds mask by the head block in fused MLA RoPE - #6890
Conversation
|
@NVIDIA/mcore-oncall — just marked this Ready. Five one-line mask fixes in |
|
/claude fix |
Signed-off-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
|
🛠️ Claude fix commit
What changed Files changed by Claude
Why DCO Sanitized and posted by |
|
/ok to test 1159749 |
|
❌ Claude fix stopped because exact-SHA CI did not complete in time. View exact-SHA CI. |
1159749 to
2149f1e
Compare
|
/claude fix |
|
❌ Claude fix stopped because a workflow step failed. Inspect the run. |
|
/ok to test |
@wujingyue, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 2149f1e |
wujingyue
left a comment
There was a problem hiding this comment.
The absolute-head masks look right, including using i in the reduction loop. Two points to address before merging; this is based on source inspection, not local GPU validation.
| dK_ptr = dK + pid_m * stride_dk_seq + i * BLOCK_H * stride_dk_nheads | ||
| x_off = tl.arange(0, BLOCK_H)[:, None] * stride_dk_nheads + k_dim | ||
| mask = x_off < head_num * stride_dk_nheads | ||
| mask = (i * BLOCK_H + tl.arange(0, BLOCK_H))[:, None] < head_num |
There was a problem hiding this comment.
Please add other=0 to the four masked tl.load calls below this mask (both REMOVE_INTERLEAVING branches). With a partial final head block, masked-out values are undefined when other is omitted, but these values feed unconditional x_left_accum / x_right_accum additions and then tl.sum. The bounds fix therefore does not guarantee a correct dEMB reduction without zero-filling the invalid lanes. Unlike the other changed sites, a masked store cannot discard them after the reduction.
Triton documents this explicitly: https://triton-lang.org/main/python-api/generated/triton.language.load.html
There was a problem hiding this comment.
Done -- other=0.0 on all four, with a comment on why the reduction needs it where the masked stores elsewhere in this file do not.
One thing to flag rather than let you infer it: on the hardware we have, the added tests pass with and without it. A direct probe (triton 3.7.0, one L4) has tl.load(ptr + off, mask=off < N) returning 0.0 in the masked lanes, so omitting other is indistinguishable from other=0.0 there. We are taking your citation -- the documented value is undefined -- as the reason for the change, not a measurement of our own. Restoring the old mask does fail all 16 of the new tests.
|
|
||
| x_off = tl.arange(0, BLOCK_H)[:, None] * stride_x_nheads + nope_dim | ||
| mask = x_off < head_num * stride_x_nheads | ||
| mask = (pid_head * BLOCK_H + tl.arange(0, BLOCK_H))[:, None] < head_num |
There was a problem hiding this comment.
Could you commit regression coverage that forces a partial head block, e.g. head_num=12, BLOCK_H=8, rather than relying on the autotuner to select that configuration? The existing tests use power-of-two head counts, and the direct-kernel experiments described in the PR are not included as tests. Please cover forward output, backward gradients (especially the KV-split dEMB reduction), and guard-buffer integrity, exercising both interleaving branches. Adding a non-power-of-two head count alone can still miss the defect if autotuning picks a divisor.
There was a problem hiding this comment.
Added TestFusedMLARopePartialHeadBlock. head_num=12 with BLOCK_H pinned to 8 by swapping each kernel's autotuner config list for a single entry, so the tuner cannot fall back on a divisor; BLOCK_H=4 is the control.
16 cases, over both REMOVE_INTERLEAVING branches:
- forward output and every gradient against the unfused reference,
sbhdandthd,inverseboth ways -- this is where the KV-splitdEMBgradient is checked; BLOCK_H=8vsBLOCK_H=4bitwise on all four KV-split tensors, which needs no reference implementation and so catches a stray lane whatever value it held;- guard-buffer integrity: the rotated tensor is a view of the head of a larger buffer with a poisoned tail, and the tail must come back untouched.
All 16 fail with this file restored to main's version and pass with the patch. Before the fix the guard buffer counts 255 elements written past the end of the tensor against the 256 the mechanism predicts -- one bf16 store landed on the poison value it replaced.
Unrelated, so a full-file run does not mislead you: four tests in this file (TestApplyRotaryPosEmbTHD, TestApplyRotaryPosEmbMlaFusionConflict) fail on our box because it has no transformer_engine. They fail identically on unmodified main.
|
Hi @huthvincent |
Resolves the conflict with NVIDIA#5963, which landed in the same two in-place kernels. The kernel merged cleanly and both changes are intact: the absolute-index bounds masks and the zero-filled dEMB loads from this branch, and the load-before-store barriers from NVIDIA#5963. The conflict was in tests/unit_tests/fusions/test_mla_yarn_rope_apply.py, where both sides added code at the same places. Kept both: NVIDIA#5963's race test and this branch's partial-head-block tests. The guarded import binds the union of both sides' names. One change git did not flag: NVIDIA#5963 moved autotuning off `_mla_rope_{fwd,bwd}_inplace_kernel` onto new `_autotuned_*` wrappers, so the BLOCK_H pinning in both this file and the determinism test now targets the wrappers. Without that the pin would read `.configs` off a plain JITFunction and fail. Signed-off-by: Rui Zhu <rui.zhu.rz399@yale.edu>
|
Done — merged |
|
/ok to test f703e6f |
@Phlip79, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test f703e6f |
|
Hey @NVIDIA/ci, we may need an one-off to bump it up in the ci queue and/or the merge queue. It's intended for the 26.10 release but gets stuck in cicd-wait-in-queue. |
The partial head block masking in the four MLA RoPE kernels is fixed on main (NVIDIA#6890), whose tests pin BLOCK_H to 8 and 4 at 12 heads. Triton's autotuner may settle on any of the eight BLOCK_H candidates, and it launches every one of them while benchmarking, so a candidate the pinned tests do not reach can still corrupt a run. Force each candidate in turn at 127 heads, which no candidate above one divides, and compare the query, key and value outputs and the q, kv and k_pos_emb gradients against one-head blocks in both sbhd and thd layouts. The in-place kernels are pinned through their autotuned wrappers, since the raw JIT kernels are now launched directly by the race tests. With the masking fix reverted the test fails at BLOCK_H=2. Signed-off-by: Yan Xu <yxu1@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
All five per-head bounds masks in
megatron/core/fusions/fused_mla_yarn_rope_apply.pyare built from theintra-block head index, while the base pointer has already been advanced to the program's head block:
The comparison reduces to
local_head < head_num, which is true for every lane of every block. The launchgrid is
cdiv(nheads, BLOCK_H)andBLOCK_His autotuned over{1,2,...,128}, so whenever the chosen blocksize does not divide the head count the final program runs unmasked. It then rotates
cdiv(head_num, BLOCK_H) * BLOCK_H - head_numhead rows that do not exist — and becausestride_x_seq == head_num * stride_x_nheads, those rows are the next token's leading heads, or past the endof the tensor for the last token.
So the output depends on the autotuned block size, which is meant to be a pure tiling choice.
This PR bounds the absolute head index instead. That is the idiom this file already uses: #6343 added new
kernels here with
head_idx = pid_head * BLOCK_H + tl.arange(0, BLOCK_H)/head_mask = head_idx < head_numwhile leaving these five legacy sites as unchanged context.
The fifth site (the
dEMBreduction in_mla_rope_bwd_kv_split_kernel) sits insideif pid_head == 0:anditerates
for i in tl.static_range(cdiv(head_num, BLOCK_H)), so it is offset byi, notpid_head— amechanical rewrite there would be a no-op and the
k_pos_embgradient would stay over-accumulated. Goingstride-free also fixes the reuse of a single
maskacrossdKV/dK/dV, whose head strides differ.We did not measure any performance impact of this change
Nothing in this PR is a performance claim, and no timing, throughput or memory figure is asserted anywhere in
it. The measurements below are correctness assertions only, taken on one NVIDIA L4 (PCIe, no NVLink), torch
2.12.0+cu130, triton 3.7.0 — hardware unlike anything this kernel targets. We have no H100 or GB200 access and
have deliberately not compared any number of ours against any number of yours.
How we checked it
BLOCK_Handnum_warps=1are pinned by launching the underlyingJITFunctiondirectly, so the autotunercannot vary the config, and no external reference implementation is involved.
1. The result depends on the block size — same kernel, same seeded input, two
BLOCK_Hvalues:BLOCK_HdividesBLOCK_Hdoes not2. It writes past the end of the tensor — the tensor is a view of the head of a larger buffer whose tail is
poisoned; the tail is compared before and after the launch. The count matches the mechanism's prediction
exactly,
(cdiv(H,BLOCK_H)*BLOCK_H - H) * emb_dim:H=12, BLOCK_H=8H=24, BLOCK_H=16H=48, BLOCK_H=32H=96, BLOCK_H=64H=12, BLOCK_H=4H=16, BLOCK_H=8H=12, BLOCK_H=163. The patch changes nothing where the old code was already correct — bitwise identical before and after
for
H=16/BLOCK_H=8,H=12/BLOCK_H=4,H=32/BLOCK_H=8,H=128/BLOCK_H=64.We could not run
tests/unit_tests/transformer/test_mla_yarn_rope_apply.pyas a whole here (notransformer_engine on this box, and no wheel for it), so the kernels were exercised directly.
Not this bug: #4640 and #5963 report intermittent multi-warp mismatches in these same two in-place kernels
under
remove_interleaving=False. Everything above pinsnum_warps=1specifically so the two cannot beconfused, and the block-size comparison needs no oracle at all.
The strongest objection we know of, unanswered
An independent reviewer we ran this past put it this way, and we think it is fair:
We have not found a shipped MLA recipe that triggers it either. What we can say is that there is no
asserton
nheadsanywhere in the file, thatexamples/gpt3/train_gpt3_175b_distributed.shships--num-attention-heads 96 --tensor-model-parallel-size 8(12 per partition) andNemotron-Mini-4B-Instruct.shships 24, so the shape exists in your own recipes even if not yet under MLA —and that the correct form is already in this file from #6343.
If you would rather have an
assert nheads % BLOCK_H == 0or a constraint onnheads, say so and we will sendthat instead.
Checklist
-ssign-off, and the commit is SSH-signedblack/isort/ruffpertools/autoformat.shon the changed file; the two pre-existingF541hits elsewhere in the tree were left alone
main; no out-of-scope changes