fix(true-on-policy): match SGLang's row-linear k-tile reduction order#65
Open
zihaow211 wants to merge 1 commit into
Open
fix(true-on-policy): match SGLang's row-linear k-tile reduction order#65zihaow211 wants to merge 1 commit into
zihaow211 wants to merge 1 commit into
Conversation
_sglang_row_parallel_matmul combined the 128-wide K partials with a plain
binary tree, but SGLang's matmul_tp_inv accumulates FIRST_LEVEL_BLOCK partials
sequentially into one accumulator before carrying into the binary levels above
it. The two orders coincide only when the k-tile count is a power of two, so
Qwen3-4B fc2/down_proj at TP=2 (K=4864, T=38, FIRST_LEVEL_BLOCK=19) diverged
from rollout in bf16 even though every partial product matched.
Add _sglang_kernel_order_sum to reproduce the kernel's accumulation order.
_fixed_tree_sum_tensors is unchanged and still used for the cross-rank
reduction, where it matches SGLang's tree_all_reduce_sum over a power-of-two
world size.
The existing test compared sglang_reference_matmul against the function it
dispatches to, so it asserted the function equals itself; its K=256 also gave
T=2, inside the power-of-two blind spot. The new tests assert the association
order directly and need no GPU.
Verified bitwise identical to torch.ops.tp_inv_ops.matmul_tp_inv on B200 for
T in {4,6,8,12,16,19,24,32,38,40,48,64}, the bias path, and 3-D inputs.
Fixes radixark/miles#1485
Co-Authored-By: Claude Opus
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.
Problem
_sglang_row_parallel_matmulcombines the 128-wide K partials with a plain binary tree(
_fixed_tree_sum_tensors), but SGLang'smatmul_tp_invdoes not reduce that way. Itskernel accumulates
FIRST_LEVEL_BLOCKpartials sequentially into one registeraccumulator before carrying into the binary levels above it
(
table_value = FIRST_LEVEL_BLOCK if level == 0 else 2), andFIRST_LEVEL_BLOCKcomesfrom halving the k-tile count while it stays even:
The two orders coincide only when
FIRST_LEVEL_BLOCK == 2, i.e. whenTis a power oftwo. For Qwen3-4B
fc2/down_projat TP=2 the localK = 4864givesT = 38, soFIRST_LEVEL_BLOCK = 19and SGLang computes(p0 + ... + p18) + (p19 + ... + p37)while Megatron computes a full binary tree over all 38. Accumulation is in bf16
(
acc_dtype = A.dtypeunlessfp32_accum), so the association order is visible in theoutput bits and
down_projdiverges from rollout even though every partial product isidentical.
Fix
Add
_sglang_kernel_order_sum, which accumulates each first-level block sequentially andthen reduces the blocks with the existing binary tree. The block count is always
2**(LEVEL_K - 1), so the tree is exact and no partial is left unpaired._fixed_tree_sum_tensorsis left as-is: it is still the right shape for the cross-rankreduction, where it matches SGLang's
tree_all_reduce_sumover a power-of-two worldsize. Only the k-tile reduction inside
_sglang_row_parallel_matmulchanges. Theaddition count is unchanged (
n - 1either way); only the association differs.The existing
test_sglang_row_parallel_matmul_uses_fixed_k_block_ordercould not havecaught this — it compares
sglang_reference_matmul(..., row_parallel=True)against_sglang_row_parallel_matmul, which is the function it dispatches to, so it asserts thefunction equals itself. Its
K=256also givesT=2, inside the power-of-two blind spot.The new tests assert the association order directly.
Validation
Compared against the real
torch.ops.tp_inv_ops.matmul_tp_invtriton kernel on 1×B200(sm_100), torch 2.7 / triton 3.3, bf16,
M=128, N=2560.At
T=38(K=4864, the Qwen3-4Bdown_projshard at TP=2):Sweeping
Tconfirms the power-of-two rule, and that the fix holds everywhere:FIRST_LEVEL_BLOCKAfter the fix
_sglang_row_parallel_matmulis bitwise identical tomatmul_tp_invforevery
Tabove, for the bias path, and for 3-D inputs. New unit tests pass; they need noGPU and no SGLang.
Out of scope
_sglang_rollout_partition_row_parallel_matmulalso reduces with_fixed_tree_sum_tensors,but that is correct: it runs only when SGLang's
should_use_tp_invariant_row_lineargatefails, where SGLang itself falls back to one plain GEMM per rollout rank plus the tree
all-reduce. I checked it against that equivalent over 100 configurations
(
K ∈ {9728, 2560}, train/rollout TP∈ {1,2,4}×{8,16},M ∈ {1, 8, 128, 2048, 8192},bf16 and fp16) — all bitwise identical, including the non-contiguous slices it hands
cuBLAS. Unchanged here.
Separately,
RowParallelLinearaddsbiasafter the TP reduce while SGLang fuses it intorank 0's GEMM before the reduce, which is not bitwise equivalent in bf16. Qwen3 is
unaffected (
down_projisbias=False, released Qwen3 configs setattention_bias: false), ando_projat TP=2 has 16 k-tiles so it is untouched by thisPR. Filed separately.
Fixes radixark/miles#1485
Thanks to @MeouSker77 for the report and the root-cause analysis.