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.

Sign up for GitHub

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

Jump to bottom

[TVM] Add importer for ONNX QLinearMatMul op #8952

Merged
merged 1 commit into from
Oct 11, 2021
Merged

[TVM] Add importer for ONNX QLinearMatMul op #8952

merged 1 commit into from
Oct 11, 2021

Conversation

cconvey
Copy link
Contributor

@cconvey cconvey commented Sep 7, 2021

  • adds importer code

  • enables test_qlinearmatmul_2D unit test

@cconvey cconvey mentioned this pull request Sep 7, 2021
Closed
AndrewZhaoLuo
AndrewZhaoLuo reviewed Sep 7, 2021
View reviewed changes
python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
tmoreau89
tmoreau89 reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@tmoreau89 tmoreau89 left a comment

Choose a reason for hiding this comment

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

Thank you @cconvey for this first contribution!

@tmoreau89
Copy link
Contributor

@cconvey may need to rebase and resolve conflicts given that test_forward has changed. @anwang2009 and @mbrookhart if you get cycles on Friday could you help review? thanks!

anwang2009
anwang2009 reviewed Sep 24, 2021
View reviewed changes
python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
@cconvey cconvey changed the title [TVM] Add importer for ONNX QLinearMatMul op **WIP** [TVM] Add importer for ONNX QLinearMatMul op Oct 6, 2021
@cconvey
Copy link
Contributor Author

cconvey commented Oct 6, 2021

I just pushed a significantly revised version of the code, but it's not ready for review yet.

@cconvey cconvey force-pushed the cconvey/qlmatmul branch 2 times, most recently from 1092db4 to d15bc48 Compare October 6, 2021 20:29
@cconvey cconvey changed the title **WIP** [TVM] Add importer for ONNX QLinearMatMul op [TVM] Add importer for ONNX QLinearMatMul op Oct 6, 2021
AndrewZhaoLuo
AndrewZhaoLuo reviewed Oct 6, 2021
View reviewed changes
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.

Some comments

tests/python/frontend/onnx/test_forward.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/onnx.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/onnx.py Show resolved Hide resolved
tests/python/frontend/onnx/test_forward.py Show resolved Hide resolved
@cconvey cconvey force-pushed the cconvey/qlmatmul branch 3 times, most recently from 5f8b0d4 to 23748c1 Compare October 7, 2021 15:01
@cconvey
Copy link
Contributor Author

cconvey commented Oct 7, 2021

@mbrookhart @AndrewZhaoLuo ready for (re)review.

AndrewZhaoLuo
AndrewZhaoLuo reviewed Oct 7, 2021
View reviewed changes
python/tvm/relay/frontend/onnx.py
# express these constraints.
matmul_result_dtype = "int32"

matmul_result = _qnn.op.dense(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a TODO looking into refactoring this with regular MatMul code which handles broadcasting and the 3D cases already?

Feel free to assign it to you or me.

E.g.
TODO (AndrewZhaoLuo): ... or TODO(cconvey): ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking that this should probably be part of a larger discussion about what collection of matmul-related ops are provided by Relay / Relax. If you don't mind I'll bring the topic up in a different forum.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the Matmul code in onnx, which handles numpy style broadcasting. That one is just a frontend change on how we generate relay from frontend code.

AndrewZhaoLuo
AndrewZhaoLuo approved these changes Oct 7, 2021
View reviewed changes
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

@tmoreau89
Copy link
Contributor

Also CC- @anwang2009 for the re-review

@cconvey cconvey force-pushed the cconvey/qlmatmul branch 2 times, most recently from e20764d to ec16e22 Compare October 8, 2021 18:39
mbrookhart
mbrookhart approved these changes Oct 8, 2021
View reviewed changes
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

@mbrookhart
Copy link
Contributor

Looks like you hit #9231

@cconvey
[TVM] Add importer for ONNX QLinearMatMul op
484fd9d 
 * adds importer code

 * enables `test_qlinearmatmul_2D` unit test
@tmoreau89 tmoreau89 merged commit 0fa860c into apache:main Oct 11, 2021
@tmoreau89
Copy link
Contributor

Thank you @cconvey @anwang2009 @AndrewZhaoLuo @mbrookhart the PR has been merged!

masahi pushed a commit to Laurawly/tvm-1 that referenced this pull request Oct 14, 2021
@cconvey @masahi
[TVM] Add importer for ONNX QLinearMatMul op (apache#8952)
2876402 
* adds importer code

 * enables `test_qlinearmatmul_2D` unit test
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
@cconvey @ylc
[TVM] Add importer for ONNX QLinearMatMul op (apache#8952)
b2e357b 
* adds importer code

 * enables `test_qlinearmatmul_2D` unit test
@yuanfz98 yuanfz98 mentioned this pull request Jan 12, 2022
Closed
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@cconvey @ylc
[TVM] Add importer for ONNX QLinearMatMul op (apache#8952)
6040fcf 
* adds importer code

 * enables `test_qlinearmatmul_2D` unit test
@cconvey cconvey deleted the cconvey/qlmatmul branch May 5, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmoreau89 tmoreau89 tmoreau89 left review comments

@anwang2009 anwang2009 anwang2009 left review comments

@AndrewZhaoLuo AndrewZhaoLuo AndrewZhaoLuo approved these changes

@mbrookhart mbrookhart mbrookhart approved these changes

@areusch areusch Awaiting requested review from areusch

@comaniac comaniac Awaiting requested review from comaniac

@Huyuwei Huyuwei Awaiting requested review from Huyuwei

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@jwfromm jwfromm Awaiting requested review from jwfromm

@kazum kazum Awaiting requested review from kazum

@merrymercy merrymercy Awaiting requested review from merrymercy

@siju-samuel siju-samuel Awaiting requested review from siju-samuel

@srkreddy1238 srkreddy1238 Awaiting requested review from srkreddy1238

@tqchen tqchen Awaiting requested review from tqchen

@yzhliu yzhliu Awaiting requested review from yzhliu

@kparzysz-quic kparzysz-quic Awaiting requested review from kparzysz-quic

@masahi masahi Awaiting requested review from masahi

@vinx13 vinx13 Awaiting requested review from vinx13

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cconvey @tmoreau89 @mbrookhart @anwang2009 @AndrewZhaoLuo