-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add Qlinearconcat op #2476
Add Qlinearconcat op #2476
Conversation
This build is not recommended to merge 🔴 |
🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
e562b9a
to
484aa06
Compare
6b3f2ff
to
bc838bd
Compare
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.
Thank you.
if((args_size < 5) or ((args_size - 2) % 3 != 0)) | ||
MIGRAPHX_THROW("QLINEARCONCAT: missing inputs"); |
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.
Aren't the inputs supposed to be tuples? As in each input with type TV
is a tuple of (Tensor, Scale, ZeroPoint)
. The spec says (3 - inf)
inputs, so that's what I would expect.
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.
I've checked the models in the description and those inputs are parsed just a simple sequence of tensors, I can't see any tuple type for shapes there.
For MaskRCNN-int8 I can see the following inputs for QLinearConcat operator:
shape: float_type, {1}, {0}
shape: uint8_type, {1}, {0}
shape: uint8_type, {1000, 4}, {4, 1}
shape: float_type, {1}, {0}
shape: uint8_type, {1}, {0}
shape: uint8_type, {1000, 4}, {4, 1}
shape: float_type, {1}, {0}
shape: uint8_type, {1}, {0}
shape: uint8_type, {1000, 4}, {4, 1}
shape: float_type, {1}, {0}
shape: uint8_type, {1}, {0}
shape: uint8_type, {1000, 4}, {4, 1}
shape: float_type, {1}, {0}
shape: uint8_type, {1}, {0}
shape: uint8_type, {507, 4}, {4, 1}
shape: float_type, {1}, {0}
shape: uint8_type, {1}, {0}
Is there any specific parsing option for tuple_type?
ONNX Runtime also parses these inputs as a sequence of tensors:
https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/contrib_ops/cpu/quantization/qlinear_concat.cc#L18-L19
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.
Given that the models use the operator that way, the way the code is currently makes sense to me. I also don't think there is a tuple type in ONNX. The spec is problematic however (Microsoft probably made a mistake). I would like the docstring with the spec on this operator deleted and something that reflects the code to be written instead.
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.
Removed the docstring and added comments about the actual input tensor layout in the input checking part.
d506f12
to
55db65c
Compare
QlinearConcat operator is required for the following int8 onnx zoo models:
Fixes: migraphx-benchmark#158