Repository navigation
Fix out-of-bounds row access in inference v2 layer_norm/rms_norm kernels for hidden sizes <= 128 - #8886
Fix out-of-bounds row access in inference v2 layer_norm/rms_norm kernels for hidden sizes <= 128#8886rapsealk wants to merge 2 commits into
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e64191f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (DtypeEnum.fp16, 128), (DtypeEnum.bf16, 128)]) | ||
| @pytest.mark.parametrize("do_residual", [True, False]) | ||
| def test_rms_small_channels(rows: int, dtype: DtypeEnum, channels: int, do_residual: bool) -> None: | ||
| if dtype.value not in get_dtypes(): |
There was a problem hiding this comment.
Prevent the dtype cache from skipping fp32 cases
Under the full inference-v2 CI invocation in .github/workflows/nv-a6000.yml:61-63, collection reaches test_bias_activation.py:66-68 before this module, and that decorator calls get_dtypes(include_float=False). Since inference_test_utils.py:25-37 caches the resulting half-only list globally, this check skips both explicit DtypeEnum.fp32 cases even on fp32-capable hardware; only the targeted three-file command happens to seed the cache with fp32 first. This leaves normal CI without fp32 coverage of the new row bounds, so test optional bf16 support directly (or make the cache respect include_float) instead of using the shared cached list here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and it is wider than this test: under the directory-level CI invocation test_bias_activation.py seeds the cache first with include_float=False, so every later get_dtypes() caller (test_post_ln, test_pre_ln, test_rms_dtypes, test_gated_activation, test_post_ln_module, test_ragged_embed, test_logits_gather) lost float32 as well. Fixed in dd9f108 by caching the full list in get_dtypes() and filtering per call, so this skip check now only drops bf16 where it is unsupported. Locally kernels/core_ops goes from 110 passed + 8 skipped to 136 passed. Note for CI: the fp32 cases of test_ragged_embed and test_logits_gather will run for the first time; I could not run them here (no aarch64 deepspeed-kernels).
dd9f108 to
bbef33f
Compare
|
Pushed bbef33f after an adversarial review pass (two independent reviewers plus the Codex finding above). Kernel diff is unchanged; the evidence around it was the weak part:
Branch: 157 passed. Same tests on master's kernels: 78 failed / 79 passed. |
…els for hidden sizes <= 128 For elems_per_row <= 128 the v2 norm launchers pack several rows into one 256-thread block, but the kernels had no row bound: the last, partially filled block loaded and stored rows past the end of every input and output. This is reachable through CUDARMSNorm with a small `channels`, e.g. the per-head Q/K RMSNorm of EXAONE4 / EXAONE 4.5 (head_size 64 or 128, rows = tokens * heads). Pass `rows` to rms_norm, pre_rms_norm, fused_ln and fused_residual_ln, derive the row index from the expression already used for block_offset and mask the global loads/stores of rows past the end. Those threads still take part in the block reductions, so the grid/block shape is unchanged. Same change as deepspeedai#8830 for the v1 kernels. Tests allocate every kernel output in front of a full block of canary rows so a write past the last row fails under plain pytest, and add sub-block shapes (17-257 rows, 4-128 channels) that leave the last block partially filled for every threads-per-row dispatch (1-16); the combinations that fit in one block act as controls. fused_ln (CUDAFPLN) had no direct test and gets one. The generic bf16 tolerance (rtol 0.48) accepts a row normalized with its neighbour's statistics, so the norm tests use (3e-2, 1e-2), which the kernels pass with a 5x margin, and every norm test also checks that each row of a batched launch is bitwise equal to that row run alone. get_dtypes() cached whichever list its first caller asked for, so under the directory-level CI invocation test_bias_activation's include_float=False call dropped float32 for every later test module, including the norm tests above. Cache the full list and filter per call instead. Fixes deepspeedai#8831 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jeongseok Kang <jskang@lablup.com>
bbef33f to
5e2f6ea
Compare
|
Pushed 5e2f6ea after a second independent review pass. Kernel diff still unchanged.
Branch: 200 passed, 3 skipped. Same tests on master's kernels: 108 failed / 92 passed, all on the canary. Note that no CI workflow has run on this PR yet ( |
…rop the get_dtypes change - assert_rows_independent compares a batched row with the same row run alone under the dtype's norm tolerance instead of bitwise, so it checks the norm contract rather than the current reduction schedule. - One sub-block case per threads-per-row dispatch (rows = one more than fit in a block) instead of a full rows x channels grid. - The get_dtypes() cache fix moves to its own PR; the fp32 rms cases skip on bf16 support directly so they no longer depend on the cached list. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jeongseok Kang <jskang@lablup.com>
Fixes #8831
Problem
For
elems_per_row <= 128the inference v2 norm launchers pack several rows into one 256-thread block, but the kernels have no row bound. Whenrowsis larger than the rows per block and not a multiple of it, the last, partially filled block loads and stores rows past the end of every input and output. Reachable throughCUDARMSNormwith a smallchannels, e.g. the per-head Q/K RMSNorm of EXAONE4 / EXAONE 4.5 (head_size 64 or 128, rows = tokens * heads).Fix
rowsto therms_norm,pre_rms_norm,fused_lnandfused_residual_lnkernels.block_offset, and mask the global loads and stores of rows past the end.Same change as #8830 makes for the v1 kernels in
csrc/transformer/inference.Tests
test_rms_norm.py,test_post_ln.py,test_pre_ln.pyand the newtest_ln.py(CUDAFPLNhad no test in-tree) is allocated in front of 256 canary rows. A write past the last row fails under plain pytest, without compute-sanitizer.assert_rows_independent: each row of a batched launch must match the same row run alone, within the dtype tolerance. The generic bf16 tolerance (rtol 0.48) is loose enough to accept a row normalized with its neighbour's statistics, so the norm tests use (3e-2, 1e-2) for bf16, measured with a 5x margin over all five kernels and the existing large shapes. No global tolerance change.Test results
Hardware: 1x NVIDIA GB10 (sm_121, aarch64), driver 580.126.09, CUDA 13.0, torch 2.14.1+cu130, Python 3.13.2.
master(67ec196) + this branch's testsMeasured on 5e2f6ea. The current revision (3f70aeb) trims the shape matrix and relaxes the row check to tolerance; the re-run on the same machine is pending and this table will be updated. Every failure on
masteris a sub-block case failing on the canary assertion. The skips are fp32 with 65-128 channels (#8827, see below).compute-sanitizer memcheck (reads as well as writes)
Each of the four kernels on exactly sized
[rows, channels]tensors withPYTORCH_NO_CUDA_MEMORY_CACHING=1undercompute-sanitizer --tool memcheck, 11 shapes covering every threads-per-row dispatch and the in-place path:masterkernelsInvalid __global__ read of size 16 bytesinrms_norm/fused_ln/fused_residual_ln, 262 errors over the 11 shapes, then a sticky launch failureERROR SUMMARY: 0 errorsThroughput (GB10, median of 200 CUDA-event timings, master -> branch, microseconds)
All within +-1%, inside round-to-round noise. In-range outputs of all five wrappers were compared bitwise against
masterover 2430 (kernel, dtype, channels, rows) combinations: no differences.Compatibility matrix
Not a duplicate
No open PR references #8831. #8830 (OOB rows), #8743 (
rows == 0) and #8827 (fp32 65-128 channels launches no kernel) change only the v1 kernels undercsrc/transformer/inference/csrc/. The v1 kernels, the v2 rms and layer norm kernels, and the other v2 kernels underkernels/{core_ops,ragged_ops}were checked; only the v1 and v2 norm kernels pack rows per block this way.Related v2 defects not fixed here (same on master)
threads_per_group == 32falls through the sub-block dispatch and no kernel is launched (v2 sibling of Fix inference layer_norm/rms_norm launching no kernel for fp32 hidden sizes 65-128 #8827). The LN tests skip that cell with an explicit reason.rows == 0divides by zero in all four v2 launchers (v2 sibling of Return early from inference norm launchers when rows == 0 #8743).rms_norm.cpphas nodim() == 2or contiguity check;layer_norm.cppflattens 3-D inputs, so the two bindings disagree.block_offsetisintin all four kernels and overflows past 2^31 elements. The newrowindex adds no overflow of its own.csrc/quantization/quantize.cu(cached_quantization) uses the same row packing with bounded stores but unbounded loads.get_dtypes()in the v2 test utils caches the first caller's filtered list, so under the directory-level CI invocation later callers lose fp32 (found by the Codex review here). Split out into its own PR so this one stays a kernel fix.AI assistance
This change was written with AI assistance (Claude Code). The submitter reviewed every changed line and ran the tests above on the hardware listed.
🤖 Generated with Claude Code