Add int8/uint8 CPU support for SpaceToDepth and int8 for DepthToSpace#29154
Conversation
SpaceToDepth previously supported only float/double on CPU, and DepthToSpace supported float/double/uint8. Extend the CPU kernels so both ops also accept 8-bit integer tensors, which is needed for running quantized models. uint8_t and int8_t are routed through a single template instantiation: the ops only shuffle 8-bit elements, so the signedness of the data is irrelevant and we avoid the binary-size cost of a second instantiation. SpaceDepthOpCpuImpl now takes raw pointers so the shared branch can feed it via DataRaw(). Adds typed unit tests covering uint8/int8 for SpaceToDepth and int8 for DepthToSpace, and updates docs/OperatorKernels.md. Fixes microsoft#21287
|
@microsoft-github-policy-service agree |
tianleiwu
left a comment
There was a problem hiding this comment.
Clean, well-scoped change. Routing uint8_t and int8_t through a single SpaceDepthOpCpuImpl<uint8_t> instantiation (fed via DataRaw()/MutableDataRaw()) is the right call and honors the binary-size guidance from #21287 — the op only shuffles 8-bit elements, so signedness is irrelevant. Kernel registrations, the unsupported-type error messages, docs/OperatorKernels.md, and the tests are all updated consistently, and alignment guarantees are preserved since DataRaw() returns the same tensor buffer as Data<T>().
One minor, low-priority suggestion on the reused test data range (inline). No correctness, security, or performance concerns.
There was a problem hiding this comment.
Pull request overview
Adds missing 8-bit integer CPU support for ONNX SpaceToDepth and DepthToSpace, enabling better execution of quantized models on the CPU EP while keeping binary size impact low by sharing a single 8-bit implementation.
Changes:
- Extend CPU kernel type constraints:
SpaceToDepthnow supportsuint8+int8;DepthToSpacenow supportsint8(in addition to existinguint8support). - Refactor the CPU shuffle helper to accept raw pointers so
uint8/int8can share a single template instantiation viaDataRaw()/MutableDataRaw(). - Add/extend unit tests for the new integer types and update
docs/OperatorKernels.mdto reflect the new supported types.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| onnxruntime/test/unittest_util/conversion.h | Adds ConvertFloatToInt8_t helper for producing int8_t test vectors. |
| onnxruntime/test/providers/cpu/tensor/space_depth_ops_test.cc | Expands typed tests to include int8_t and adds coverage for SpaceToDepth integer inputs. |
| onnxruntime/core/providers/cpu/tensor/space_depth_ops.cc | Updates CPU kernel registrations and routes uint8_t/int8_t through a shared 8-bit shuffle implementation. |
| docs/OperatorKernels.md | Documents updated CPU kernel type support for SpaceToDepth/DepthToSpace. |
- Add opset 13 typed tests for SpaceToDepth and DepthToSpace so the opset 13 integer kernel registrations are exercised (existing tests only ran at opset 7/11). - Note the intentional out-of-range int8_t wrap in DepthToSpaceTest_3/4 (the op is a pure permutation, so the same conversion on input and expected output cancels out). - Reword the element-type and QNN-exclusion comments to avoid implying the CPU kernel supports MLFloat16.
tianleiwu
left a comment
There was a problem hiding this comment.
Re-reviewed after commit feaa3d6. The follow-up fully addresses the round-1 feedback: the intentional out-of-range int8 wrap in DepthToSpaceTest_3/_4 is now documented, and two new opset-13 typed tests (SpaceToDepthTest_int_opset13, DepthToSpaceTest_int_opset13) exercise the opset-13 type-constraint registrations with small in-range data so the float→int8 conversions are exact. I hand-verified both new expected outputs and they match (SpaceToDepth [0,2,8,10,1,3,9,11,4,6,12,14,5,7,13,15]; DepthToSpace DCR [0,4,1,5,8,12,9,13,2,6,3,7,10,14,11,15]). The reworded MLFloat16/QNN comments are accurate. No remaining concerns. LGTM.
The TensorRT CI pipelines failed on the new int8 (signed char) typed test cases. Unlike uint8, which TensorRT rejects up front and falls back to CPU, TensorRT accepts the int8 node and then fails at engine build time because int8 I/O without Q/DQ layers requires a calibrator or a set dynamic range. These ops' 8-bit integer support targets the CPU EP, so exclude the TensorRT EP for the int8 type parameter only (via if constexpr), preserving the existing float/uint8 TensorRT coverage. The CPU kernel itself is unchanged and all 38 SpaceToDepth/DepthToSpace tests pass.
|
@tianleiwu apologies for invalidating your approval. The two TensorRT CI pipelines failed on the new int8 (signed char) test cases, so I pushed commit e4be642 to fix them. Root cause: unlike uint8, which TensorRT rejects up front and then falls back to CPU, TensorRT accepts the int8 SpaceToDepth/DepthToSpace node and then fails at engine build time, because int8 input/output without Q/DQ layers requires a calibrator or a set dynamic range. The fix is test-only: it excludes the TensorRT EP for the int8 type parameter (via if constexpr), which preserves the existing float and uint8 TensorRT coverage. The CPU kernel is unchanged, and all 38 SpaceToDepth/DepthToSpace tests still pass locally. This follows the same pattern other CPU op tests use (for example cast_op_test and bitcast_op_test). Could you please re-review when you have a moment? Thank you again for the careful review. |
|
@tianleiwu Heads up on the one remaining red check, "Windows GPU Kernel Documentation Validation": it is a transient infrastructure failure, not related to this change. The job failed while CMake FetchContent was downloading the onnx dependency, because GitHub returned an HTTP 500: The build aborted there, so the doc validation step never actually ran. The other 84 checks are green, including all of the TensorRT jobs that were failing before. |
|
@ArsalanShakil, |
Description
Add CPU support for 8-bit integer tensor types to the
SpaceToDepthandDepthToSpaceoperators:SpaceToDepth(opsets 1–12 and 13): previously supported onlyfloat/double; now also acceptsuint8andint8.DepthToSpace(opsets 11–12 and 13): previously supportedfloat/double/uint8; now also acceptsint8.uint8_tandint8_tare routed through a single template instantiation. These ops only shuffle 8-bit elements around, so the signedness of the data is irrelevant — handling both in one branch avoids the binary-size cost of a second instantiation (per the guidance in #21287).SpaceDepthOpCpuImplnow takes raw pointers so the shared branch can feed it viaDataRaw()/MutableDataRaw().Also adds typed unit tests covering
uint8/int8forSpaceToDepthandint8forDepthToSpace, and updatesdocs/OperatorKernels.md.Motivation and Context
Fixes #21287. The
DepthToSpace/SpaceToDepthONNX operators support integer types, but the ORT CPU kernels did not fully implement them, which is needed for running quantized models. This completes the integer support on CPU (SpaceToDepthhad none;DepthToSpacewas missingint8).The CUDA kernels are already generic and could be extended similarly in a follow-up; this PR focuses on the CPU EP.