Skip to content
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

Merged
merged 5 commits into from
Sep 17, 2021

Conversation

arangasa
Copy link
Contributor

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.

@tmoreau89
Copy link
Contributor

@tmoreau89 tmoreau89 self-assigned this Sep 16, 2021
Copy link
Contributor

@mbrookhart mbrookhart left a 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.

python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@anwang2009 anwang2009 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

## 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
x = _qnn.op.dequantize(inputs[0], x_scale, x_zero_point)
x = _qnn.op.dequantize(x, x_scale, x_zero_point)

@mbrookhart
Copy link
Contributor

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.

@arangasa
Copy link
Contributor Author

arangasa commented Sep 17, 2021

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.

Copy link
Member

@masahi masahi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@masahi masahi merged commit a65bf34 into apache:main Sep 17, 2021
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
…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
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants