Add upper bound validation for DQ block_size in MatMulNBits fusion - #31678
Conversation
The DQ->MatMulNBits blockwise fusion selector now rejects models with block_size > 256, mirroring the existing [16, 256] range enforced by ComputeEffectiveBlockSize for the session-option path. Without this cap, a model-supplied block_size >= 2^32 truncates to 0 via static_cast<int> in the MLAS transpose kernel, causing an integer divide-by-zero (SIGFPE) at CreateSession time before any inference. Changes: - qdq_selectors.cc: extend the existing power-of-two check to also reject block_size > 256; invalid models skip the fusion cleanly. - qdq_actions.cc: GetEffectiveBlockSize caps the raw model attribute at kMaxBlockSize (256) as defense-in-depth, so no oversized value can reach the MLAS kernel even if the selector is bypassed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens MatMulNBits fusion in the QDQ transformer by tightening validation of the DequantizeLinear block_size attribute so that only kernel-supported values reach the MLAS transpose path, preventing session-creation crashes from unsafe attributes.
Changes:
- Enforced
block_sizeto be a power-of-two within[16, 256]in MatMulNBits fusion eligibility checks. - Added an additional “safety guard” in block size computation to avoid unsafe values reaching the MLAS transpose call.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| onnxruntime/core/optimizer/qdq_transformer/selectors_actions/qdq_selectors.cc | Tightens selector validation for blockwise DQ block_size and documents the safety rationale. |
| onnxruntime/core/optimizer/qdq_transformer/selectors_actions/qdq_actions.cc | Adds extra guarding when deriving the effective block size used by the fusion action/MLAS transpose. |
Align block_size comments with actual supported constraints, make GetEffectiveBlockSize fail fast for invalid blockwise values instead of silently clamping, and add regression coverage that rejects block_size > 256 on both CPU and CUDA transformer test paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Review summary
Correct and well-scoped change. 256 is the right upper bound: MLAS GetQNBitGemmVariant only accepts BlkLen in {16, 32, 64, 128, 256} (onnxruntime/core/mlas/lib/qnbitgemm.cpp:64), so rejecting larger values in the selector prevents emitting a MatMulNBits node that MLAS cannot service. Replacing the earlier "cap to 256" approach with a hard rejection is also the right call — silently clamping would have desynchronised block_size from the already-materialised scale/zero-point shapes and risked out-of-bounds reads in the MLAS transpose.
Tests look genuinely effective. With weight_shape = {37, 12} and block_size = 512, the harness derives scale_shape[0] = ceil(37/512) = 1, which satisfies the existing scale-shape consistency check; the node is also power-of-two, axis = 0, rank-2 and 4-bit. So it would have been fused before this PR — the new cases fail without the production change rather than passing for an unrelated reason. Nice that both the CPU and CUDA variants were covered.
No blocking issues. A few suggestions inline, plus two notes that do not map onto changed lines:
-
Stale doc comments describing the old contract:
qdq_selectors.cc:731—// - Blockwise: axis=0, block_size >= 16 and power-of-2, scale/zp rank 2qdq_selector_action_transformer.cc:304—// DQ is block-quantized along axis 0, with block_size >= 16 and as 2's power.
Both should now say
[16, 256]. -
PR description rationale is inaccurate. It states the
> 256rejection avoids "integer overflow";512is perfectly representable inint32_t/int64_t. The in-code comment was already corrected to cite kernel support — worth syncing the description so the merge commit message reflects the real reason. -
(optional, follow-up) MLAS restricts 2-bit weights further:
BlkBitWidth == 2only supportsBlkLenin{32, 64, 128}(qnbitgemm.cpp:82). The new[16, 256]guard is bit-width agnostic, soint2/uint2DQ nodes withblock_size16 or 256 still pass. Pre-existing and out of scope here, but adjacent to this PR's goal. -
(optional) Models that previously fused with
block_size > 256now silently keepDQ -> MatMul. Correct trade-off, but a VERBOSE-level log explaining the skipped fusion would help users diagnose the resulting perf change.
Share the supported block-size contract between selectors and actions, and return a graceful failure for invalid model attributes during minimal-build action replay. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please fix CI |
Graph::Resolve populates the schema default of 0 for DequantizeLinear nodes that omit block_size, so checking for attribute presence alone also caught per-tensor and per-channel DQ nodes and rejected them. Match the selector by treating only a positive value as blockwise; the derived effective block size is still validated afterwards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This pull request hardens the DQ-to-MatMulNBits fusion against unsupported model-provided
block_sizevalues.Validation changes:
[16, 256], matching the MLAS QNBit GEMM variants.block_sizeas untrusted during minimal-build replay, where the selector is not present.StatusfromProcessNewNodebefore transposing weights or invoking MLAS instead of relying onORT_ENFORCE.Tests and documentation:
[16, 256]range.This avoids silent clamping, which could make the fused block size inconsistent with scale and zero-point tensor shapes.