-
Couldn't load subscription status.
- Fork 3.5k
Add int8/uint8 support for Max and Min operators #26386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
This PR was created by Github Copilot CLI. I am trying to use it demonstrate how to use Github Copilot to speed up our work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can commit the suggested changes from lintrunner.
Fixes #26382 This commit adds support for int8, uint8, int16, and uint16 data types to the Max and Min operators for opset 12 and later, bringing the implementation into compliance with the ONNX specification. Changes: - Updated type registration for Max and Min operators (opset 12+) - Updated type dispatchers in Min_8::Compute and Max_8::Compute - Added comprehensive unit tests for all new data types The existing Eigen-based implementation already handles all numeric types generically, so only type registration and dispatching needed updates. This change is fully backward compatible.
|
Do any real models need the 16-bit types? Typically we only add required types to keep binary size growth minimal. |
|
@Honry , could you help answer this question? |
|
The issue is captured when I tested @skottmckay, Do you specify the non-necessity of supporting 16-bit types? That'd be fine to me. |
|
@skottmckay, we have a pipeline to monitor binary size, and there is no change for Android minimal build's binary size. There should be some increases in the default build, but it should be fine? |
|
it's still code that we have to test and maintain, and if there's no real usage of it that seems to be of limited value. |
Address review feedback from skottmckay: remove int16/uint16 types as they are not commonly used in real models. Keep only int8/uint8 support which was the original requirement from the WebNN EP testing. Changes: - Removed int16_t and uint16_t from type registration for Max/Min opset 12 - Removed int16_t and uint16_t from MLTypeCallDispatcher in Compute methods - Removed all int16/uint16 test cases (4 tests removed) - Updated documentation to reflect supported types The change reduces code maintenance burden while still providing the necessary int8/uint8 support for quantized models.
Description
This PR adds support for int8, uint8, int16, and uint16 data types to the Max and Min operators for opset 12 and later, resolving issue #26382.
Issue
Fixes #26382
Users encountered the error
Could not find an implementation for Max(13) node with name '_0'when trying to use Max or Min operators with int8 or uint8 data types, even though these types are required by the ONNX specification for opset 12+.Changes
Core Implementation (
onnxruntime/core/providers/cpu/math/element_wise_ops.cc)int8_t,int16_t,uint8_t, anduint16_tto the supported type listsMin_8::ComputeandMax_8::Computeto dispatch to implementations for the new typesTests (
onnxruntime/test/providers/cpu/math/element_wise_ops_test.cc)Added comprehensive unit tests for all new data types:
Max_12_Int8,Max_12_UInt8,Max_12_Int16,Max_12_UInt16Min_12_Int8,Min_12_UInt8,Min_12_Int16,Min_12_UInt16Each test verifies:
Technical Details
ONNX Specification Compliance
This implementation now fully complies with the ONNX specification for Max and Min operators at opset 12+, which requires support for:
tensor(uint8),tensor(uint16),tensor(uint32),tensor(uint64)tensor(int8),tensor(int16),tensor(int32),tensor(int64)tensor(float16),tensor(float),tensor(double),tensor(bfloat16)Impact
Testing
./onnxruntime_test_all --gtest_filter="MathOpTest.Max_12_*8*:MathOpTest.Min_12_*8*:MathOpTest.Max_12_*16*:MathOpTest.Min_12_*16*"Checklist