-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[ONNX][#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear #9028
Conversation
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 think the bugfix with DequantizeLinear isn't quite correct, but the QLinearSigmoid stuff looks great! Thank you.
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.
LGTM
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.
lgtm!
python/tvm/relay/frontend/onnx.py
Outdated
## and then requantize after: | ||
## https://github.com/microsoft/onnxruntime/blob/master/onnxruntime/core/ | ||
## providers/dml/DmlExecutionProvider/src/GraphTransformer.cpp#L245 | ||
x = _qnn.op.dequantize(inputs[0], x_scale, x_zero_point) |
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.
nit
x = _qnn.op.dequantize(inputs[0], x_scale, x_zero_point) | |
x = _qnn.op.dequantize(x, x_scale, x_zero_point) |
The de-quantize change did cause those tests to fail in CI. I think we need a better solution there, but otherwise, it looks good. |
Thank you, @mbrookhart , @anwang2009 , @masahi , @AndrewZhaoLuo for reviewing, catching the issue with this change, and for suggesting a solution. Updated the PR to address your comments. |
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.
cc @mbrookhart
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.
LGTM. Thanks!
…izeLinear (apache#9028) * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear
…izeLinear (apache#9028) * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear * [ONNX][apache#8838] QLinearSigmoid contrib op and Bug Fix for DequantizeLinear
This PR implements com.microsoft.QLinearSigmoid (tracked here). This is implemented as dequantize->sigmoid->quantize for now.
This also fixes an issue with DequantizeLinear for tensors with rank=1. The current default axis of 1 for _impl_v13 leads to failures with rank=1 tensors. A new test is added for an existing op (QLinearMul) for this case.