[BugFix] Support rms_norm for stacked 2D weights on XPU - #579
Conversation
## What is fixed Fixes vllm-project#573. XPU's rms_norm kernel computes a batch_idx to offset input/out per outer row (used by the 3D/4D q/k-norm shapes, e.g. DFlash/DFlash2 speculative-decoding K-norm fusion), but never used batch_idx to offset weight. When a caller passes a stacked 2D weight of shape [num_rows, hidden_size] (one row per layer) to normalize multiple layers in a single kernel launch, every row silently read weight row 0 instead of its own row -- producing wrong results with no error, and no shape validation to catch the misuse either. ## How it is fixed Ported CUDA's existing fix for the same op (csrc/libtorch_stable/layernorm_kernels.cu) exactly, so XPU now matches CUDA's contract for this op: - In the rms_norm() entry point, derive a weight_stride local variable: 0 when weight is 1D (unchanged legacy behavior), or weight.stride(0) when weight is 2D, after validating weight.size(0) == input.size(0) and weight.size(-1) == input.size(-1). weight.dim() not in {1, 2} now raises a TORCH_CHECK error. The public rms_norm op schema is unchanged -- weight_stride is purely an internal implementation detail threaded through the kernel launch, never exposed to Python/PyTorch callers. - Threaded weight_stride through call_rms_norm_kernel into all three kernel paths that read weight: the vectorized rms_norm_kernel, its scalar-fallback (VEC_SIZE==0) specialization, and rms_norm_multi_row_kernel (the small-hidden-size fast path actually hit by DFlash's hidden_size=128 shape -- easy to fix one kernel and miss this one). - Each kernel reuses its existing batch_idx (already computed for input/out offsetting) to compute weight_row = weight + batch_idx * weight_stride. When weight_stride == 0 (the 1D-weight case), weight_row == weight for every row, so behavior for all existing callers is bit-for-bit unchanged. - gemma_rms_norm's call site is updated to pass weight_stride=0 unchanged, since no vLLM caller stacks weights through it today. ## What tests have been run - New tests/test_batched_weight_rms_norm.py (12 parametrized cases): batched-weight correctness vs. a per-row reference loop across 5 shapes x 2 dtypes covering all three kernel paths (multi-row, generic vectorized, scalar fallback for non-power-of-two hidden size), a direct regression test reproducing the issue's zero-weight-row repro, and a shape-validation test for the new TORCH_CHECK errors. All pass. - Full existing tests/test_layernorm.py (1152 tests) plus tests/test_fused_qk_norm_rope.py, tests/test_fused_input_norm.py, and tests/test_fused_norm_quant.py (808 tests) -- zero regressions on the standard 1D-weight path. - Cross-validated against vLLM's own tests/kernels/core/test_batched_weight_rms_norm.py via a locally patched (not committed) copy enabling XPU -- 13/13 pass, including float32. - Verified the issue's exact repro script directly: previously out[1] read layer 0's weight (wrong, e.g. 3.79); now reads its own (zeroed) weight row, giving out[1] == 0.0 as expected. - Benchmarked with benchmark/benchmark_rmsnorm.py (standard 1D-weight path, both with and without residual): correctness still matches HuggingFace-naive, and vLLM's kernel remains ~9-13x faster than HuggingFace-naive and ~1.2-1.6x faster than torch.compile across the full head_num/batch_size/seq_len sweep -- confirming no performance regression on the common path. - All tests run on XPU hardware with ZE_AFFINITY_MASK=1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Huanxing <huanxing.shen@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues to address (nullptr pointer arithmetic UB in the no-weight kernel path, missing weight dtype validation, and a likely-flaky exact-equality test).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes incorrect behavior in the XPU rms_norm custom op when callers pass a stacked 2D weight ([num_rows, hidden_size]): previously all rows read weight row 0, silently producing wrong results. The change threads a weight_stride through the RMSNorm kernel implementations so the kernel can select the correct per-outer-row weight, and adds dedicated tests to prevent regression.
Changes:
- Add 2D (stacked) weight support to XPU
rms_normby passingweight_stridethrough the RMSNorm kernel launch and selectingweight_rowbased on the outer index. - Add shape validation for 1D vs 2D weights in the
rms_normentry point. - Add a new test module covering correctness and regression reproduction for batched-weight RMSNorm.
File summaries
| File | Description |
|---|---|
csrc/layernorm.cpp |
Implements per-outer-row weight selection for 2D weights via weight_stride and adds weight shape validation. |
tests/test_batched_weight_rms_norm.py |
Adds regression + correctness tests for stacked 2D weight behavior in torch.ops._C.rms_norm. |
Review details
Suppressed comments (2)
csrc/layernorm.cpp:215
- Same nullptr pointer-arithmetic issue as above: when
HasWeightis false,weightcan be nullptr, so computingweight_row = weight + ...is undefined behavior. Compute the offset only whenHasWeightis true.
seq_idx * input_stride_d3 + head_idx * input_stride_d2;
}
const scalar_t* weight_row = weight + batch_idx * weight_stride;
csrc/layernorm.cpp:348
- Same nullptr pointer-arithmetic issue in the multi-row kernel: when
HasWeightis false,weightmay be nullptr, soweight + batch_idx * weight_strideis undefined behavior even if the pointer is never dereferenced. Guard the offset computation withif constexpr (HasWeight).
seq_idx * input_stride_d3 + head_idx * input_stride_d2;
}
const scalar_t* weight_row = weight + batch_idx * weight_stride;
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jikunshang
left a comment
There was a problem hiding this comment.
thanks for fixing.
cc @zufangzhu
## What is addressed Three review comments from the automated PR review on vllm-project#579: 1. rms_norm() did not validate that weight's dtype matches input's dtype, allowing a mismatched-dtype weight tensor to be reinterpreted through the wrong scalar_t and produce incorrect results or memory errors. 2. weight_row was computed as `weight + batch_idx * weight_stride` even when HasWeight is false, performing pointer arithmetic on a null weight pointer. weight_stride is always 0 in that case so the offset is always 0, meaning this never manifested as an observed failure, but it is technically undefined behavior per the C++ standard. 3. tests/test_batched_weight_rms_norm.py compared the batched call against a per-row reference loop with atol=0/rtol=0. The batched call can take a different kernel path (e.g. the multi-row fast path) than the per-row reference, so a bitwise match is not guaranteed even when weight-row selection is correct, risking test flakiness. ## How it is addressed 1. Added a TORCH_CHECK in rms_norm() requiring weight->scalar_type() == input.scalar_type(), matching the existing pattern already used by gemma_rms_norm/fused_add_gemma_rms_norm. 2. Guarded all three weight_row computations (rms_norm_kernel's vectorized and scalar-fallback specializations, and rms_norm_multi_row_kernel) with `if constexpr (HasWeight)`, defaulting weight_row to nullptr otherwise. The later reinterpret_cast of weight_row (e.g. into v_w) remains fine when null since reinterpret_cast on a null pointer is well-defined and yields a null pointer of the destination type; the guard only needed to cover the pointer arithmetic step. 3. Changed the test's tolerance from atol=0/rtol=0 to atol=1e-2/rtol=1e-2, matching the tolerance already used throughout tests/test_layernorm.py. ## What tests have been run - Rebuilt the extension (VLLM_XPU_ENABLE_XE3P=OFF via build_kernel.sh) and reinstalled the wheel. - ZE_AFFINITY_MASK=1 pytest tests/test_batched_weight_rms_norm.py tests/test_layernorm.py -q: 1164 passed, 0 failed (covers both the batched 2D-weight paths and the has_weight=False path exercised by test_rms_norm's HAS_WEIGHT=[False, True] parametrization). - pre-commit run --files csrc/layernorm.cpp tests/test_batched_weight_rms_norm.py: all hooks passed with no reformatting needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Huanxing <huanxing.shen@intel.com>
d557a67 to
8e2c0b0
Compare
|
I have addressed the copilot review comments. Please help to review. @zufangzhu Thanks a lot. |
Fix #573
XPU's rms_norm kernel does not support stacked 2D weights. When callers pass a stacked 2D weight of shape [num_rows, hidden_size] (one row per layer) to normalize multiple layers in a single kernel launch, every row silently read weight row 0 instead of its own row, producing wrong results with no error.
How it is fixed:
Test Result