Validate SkipLayerNorm prepacked input shapes - #31676
Validate SkipLayerNorm prepacked input shapes#31676Akshay Sonawane (apsonawane) merged 4 commits into
Conversation
Co-authored-by: Copilot <223556219@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new size-validation code has a signed/unsigned arithmetic issue that should be fixed, and the added checks for beta/bias lack corresponding unit test coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR tightens runtime validation around prepacked (FP16→FP32 converted) inputs for the contrib CPU SkipLayerNormalization / SkipSimplifiedLayerNormalization kernels, so malformed or mismatched prepacked tensors are rejected early with clearer error messages. It also adds unit tests to ensure the new validation is exercised.
Changes:
- Added member state to track prepacked
gamma/beta/biastensor lengths and initialized them in the kernel constructor. - Added explicit size checks in
SkipLayerNorm::Computeto validate prepackedskip/gamma/beta/biaslengths againsthidden_size, returning descriptive failures. - Added two unit tests verifying failures for incorrect prepacked
gammaandskipsizes.
File summaries
| File | Description |
|---|---|
| onnxruntime/test/contrib_ops/skiplayernorm_op_test.cc | Adds unit tests for prepack rejection on invalid gamma and skip sizes. |
| onnxruntime/contrib_ops/cpu/skip_layer_norm.h | Introduces new members to track prepacked tensor sizes. |
| onnxruntime/contrib_ops/cpu/skip_layer_norm.cc | Implements hidden-size validation and prepacked size checks; records prepacked sizes in PrePack. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Rework hidden_size/prepacked skip validation to avoid mixed signed/unsigned arithmetic in the skip-length checks and validate hidden_size before deriving size_t-based comparisons. Add missing negative coverage for short prepacked beta and short prepacked bias, including the simplified op path where bias is provided at input index 3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Review: prepacked length validation for SkipLayerNorm
Verdict: comment / non-blocking suggestions. The change closes a real out-of-bounds read window and I'd be happy to see it merged with a couple of adjustments.
Why the fix is correct: when PrePack converts a constant MLFloat16 initializer, Compute() nulls out the corresponding p_ctx->Input<Tensor>(i), so CheckPotentiallyPrepackedInputs skips CheckSkip/CheckGamma for that input entirely. Because hidden_size comes from the runtime input shape, a model with a dynamic last dim could reach ComputeJob with a prepacked buffer shorter than hidden_size. The new checks restore that validation, and the skip condition (size >= hidden_size && size % hidden_size == 0) is exactly the invariant ComputeJob needs for skip_data + (offset % skip_size) to stay in bounds. Placing the hidden_size > 0 guard before the modulo is also the right ordering.
Main points (details inline):
- The prepacked
skipcheck restores memory safety but not shape semantics — it is strictly weaker thanCheckSkip, so some shapes that are rejected on the non-prepacked path now silently wrap. ORT_RETURN_IFreturnscommon::FAIL, while every sibling shape check inskip_layer_norm_helper.hreturnsINVALID_ARGUMENT. The same user error now yields a different status code depending on whether the tensor happened to be prepacked.
Cross-cutting suggestion (not anchorable to the diff): CheckPotentiallyPrepackedInputs in onnxruntime/contrib_ops/cpu/skip_layer_norm_helper.h already takes prepacked_skip / prepacked_gamma booleans precisely so prepacked-input validation lives in one place. Threading the recorded lengths (or shapes) into that helper instead of validating in Compute() would keep all input validation together and let both paths share the same message text and error code.
Test coverage looks good — all four prepackable tensors plus the simplified bias-at-input-3 variant, and the tests correctly use MLFloat16 + is_initializer=true, which is the only combination that actually reaches the prepack path.
Nit: the PR description says "Added two new unit tests"; the branch has five. Worth refreshing before merge.
Performance/ABI: no hot-path allocation, no C API surface touched, no stream/sync concerns. The added cost is a few integer compares plus one modulo per Compute(), negligible next to the per-row normalization.
Record full prepacked input shapes and reuse the runtime shape validators so packed and unpacked paths return consistent errors without unsafe narrowing or zero-size regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Reviewed the current head after the shape-preservation follow-up. The prepacked path now uses the same skip/gamma/beta/bias shape predicates and INVALID_ARGUMENT behavior as runtime tensors, keeps prepack metadata synchronized with successful conversion, handles the empty-skip case, and validates before output allocation. I found no remaining issues.
Validation: rebuilt onnxruntime_provider_test from this head and ran the seven new SkipLayerNormTest prepack regression tests; all 7 passed.
This pull request makes SkipLayerNorm prepacking preserve the same input-shape validation semantics as the non-prepacked path.
Prepacked input validation:
INVALID_ARGUMENTstatus codes.intbefore narrowing.Unit test coverage:
Packed shape metadata is populated only when conversion actually succeeds, keeping data and metadata state synchronized.