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

[ONNX] Add MatMulInteger16 contrib op #9186

Merged
merged 12 commits into from
Nov 24, 2021
Merged

[ONNX] Add MatMulInteger16 contrib op #9186

merged 12 commits into from
Nov 24, 2021

Conversation

abhikran-quic
Copy link
Contributor

@abhikran-quic abhikran-quic commented Oct 4, 2021 •
edited
Loading

This PR adds support for contrib op: com.microsoft.MatMulInteger16.

Converge MatMul and MatmulInteger16 ops to use a single function with output dtype.

@abhikran-quic abhikran-quic mentioned this pull request Oct 4, 2021
Closed
@tmoreau89
Copy link
Contributor

Thanks for helping with the review @cconvey ! CC-ing @mbrookhart @anwang2009 @AndrewZhaoLuo

AndrewZhaoLuo
AndrewZhaoLuo reviewed Oct 7, 2021
View reviewed changes
python/tvm/relay/frontend/onnx.py 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
@AndrewZhaoLuo
Copy link
Contributor

@abhikran-quic do you still plan on working on this?

@abhikran-quic
Copy link
Contributor Author

abhikran-quic commented Oct 21, 2021 •
edited
Loading

@abhikran-quic do you still plan on working on this?

Hi @AndrewZhaoLuo : I've been out of office since last two weeks and hence I've not been able to reply here. Sorry about this!
I will fix the changes that you've suggested once I'm back.

@AndrewZhaoLuo
Copy link
Contributor

No problem, have a good vacation!

@tmoreau89
Copy link
Contributor

@abhikran-quic friendly ping to see if updating this PR is currently blocked, thanks!

@abhikran-quic
Copy link
Contributor Author

abhikran-quic commented Nov 21, 2021 •
edited
Loading

@abhikran-quic https://github.com/apache/tvm/pull/9540/files should fix this. You can take these changes in this PR and I can close mine.

Thank you @AndrewZhaoLuo for your help on this. I've incorporated the changes mentioned by you in the latest patch.

However, there's one more pending issue. After removing test_matmulinteger from unsupported_onnx_tests, I saw an error from onnx.py . Here is the link to CI where failure is seen.

IMHO, when we remove test_matmulinteger the error is expected because there is no op named matmulinteger in onnx.py. Onnxruntime supports MatMulInteger16 and MatMulIntegerToFloat and hence we can add ops with the names mentioned in Onnx runtime documentation. Hence, removing test_matmulinteger is leading to the error.

In my latest patch, I've retained test_matmulinteger in unsupported_onnx_tests. Please share your thoughts on this.

@AndrewZhaoLuo
Copy link
Contributor

Ah yes, MatMulInteger is an onnx op but it is seperate from MatMulInteger16

AndrewZhaoLuo
AndrewZhaoLuo approved these changes Nov 22, 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

@abhikran-quic
Copy link
Contributor Author

LGTM

Thank you @AndrewZhaoLuo for your review.

I would request reviewers to please review this PR.

@abhikran-quic
Copy link
Contributor Author

Hello Reviewers,

Could you please review this PR for any more changes needed ?

tmoreau89
tmoreau89 approved these changes Nov 24, 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.

LGTM

@tmoreau89 tmoreau89 merged commit 9c4e9ff into apache:main Nov 24, 2021
@tmoreau89
Copy link
Contributor

Thank you @abhikran-quic, @AndrewZhaoLuo, @cconvey the PR has been merged

@abhikran-quic abhikran-quic deleted the onnx-matmulinteger16 branch November 24, 2021 17:35
dchauhan-arm pushed a commit to dchauhan-arm/tvm that referenced this pull request Nov 29, 2021
@abhikran-quic @dchauhan-arm
[ONNX] Add MatMulInteger16 contrib op (apache#9186)
a94d51c 
* [ONNX] Add MatMulInteger16 contrib op

* Fix formatting errors

* Remove a code comment and do not set default value of nd

* Move flatten_to_nd function outside matmul to be used across multiple functions

* Add function docstring and describe the tests

* Use max/min value of int16 as high/low while generating input vectors

* Converge MatMul and MatMulInteger16 ops into a single op using output dtype

* Fix indentation issues

* Formatting changes

* Fix CUDA batchmatmul strategy to allow mixed precision

* Add test_matmulinteger to unsupported_onnx_tests
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
@abhikran-quic @mehrdadh
[ONNX] Add MatMulInteger16 contrib op (apache#9186)
a87f4c8 
* [ONNX] Add MatMulInteger16 contrib op

* Fix formatting errors

* Remove a code comment and do not set default value of nd

* Move flatten_to_nd function outside matmul to be used across multiple functions

* Add function docstring and describe the tests

* Use max/min value of int16 as high/low while generating input vectors

* Converge MatMul and MatMulInteger16 ops into a single op using output dtype

* Fix indentation issues

* Formatting changes

* Fix CUDA batchmatmul strategy to allow mixed precision

* Add test_matmulinteger to unsupported_onnx_tests
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
@abhikran-quic @mehrdadh
[ONNX] Add MatMulInteger16 contrib op (apache#9186)
f831cc5 
* [ONNX] Add MatMulInteger16 contrib op

* Fix formatting errors

* Remove a code comment and do not set default value of nd

* Move flatten_to_nd function outside matmul to be used across multiple functions

* Add function docstring and describe the tests

* Use max/min value of int16 as high/low while generating input vectors

* Converge MatMul and MatMulInteger16 ops into a single op using output dtype

* Fix indentation issues

* Formatting changes

* Fix CUDA batchmatmul strategy to allow mixed precision

* Add test_matmulinteger to unsupported_onnx_tests
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
@abhikran-quic @ylc
[ONNX] Add MatMulInteger16 contrib op (apache#9186)
0258671 
* [ONNX] Add MatMulInteger16 contrib op

* Fix formatting errors

* Remove a code comment and do not set default value of nd

* Move flatten_to_nd function outside matmul to be used across multiple functions

* Add function docstring and describe the tests

* Use max/min value of int16 as high/low while generating input vectors

* Converge MatMul and MatMulInteger16 ops into a single op using output dtype

* Fix indentation issues

* Formatting changes

* Fix CUDA batchmatmul strategy to allow mixed precision

* Add test_matmulinteger to unsupported_onnx_tests
@Icemist
Copy link
Contributor

Icemist commented Jan 10, 2022

@abhikran-quic
Could I please ask why did you delete the code branch under the ONNX_DEFAULT_CONFIGS["use_nt_batch_matmul"] parameter?
I found that after this change, some of the code samples stopped working with an error like “Check failed: (reporter->AssertEQ(xk, yk)) is false: BatchDot: shapes of x and y is inconsistent, x shape=[16, 384, 384], y shape=[16, 384, 64]“
So now I see ONNX_DEFAULT_CONFIGS is not used by anyone. But The same logic with use_nt_batch_matmul is there for tensorflow.
Can we restore this logic? Will it affect the MatMulInteger16 operation you added?

yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
@abhikran-quic @yangulei
[ONNX] Add MatMulInteger16 contrib op (apache#9186)
4260b8c 
* [ONNX] Add MatMulInteger16 contrib op

* Fix formatting errors

* Remove a code comment and do not set default value of nd

* Move flatten_to_nd function outside matmul to be used across multiple functions

* Add function docstring and describe the tests

* Use max/min value of int16 as high/low while generating input vectors

* Converge MatMul and MatMulInteger16 ops into a single op using output dtype

* Fix indentation issues

* Formatting changes

* Fix CUDA batchmatmul strategy to allow mixed precision

* Add test_matmulinteger to unsupported_onnx_tests
@abhikran-quic
Copy link
Contributor Author

Hi @Icemist , This change was done to optimize the code path and since all the tests in automation passed, we went ahead and committed the change.
If this is affecting the logic you've mentioned above, we can restore it definitely.

yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
@abhikran-quic @yangulei
[ONNX] Add MatMulInteger16 contrib op (apache#9186)
c456d93 
* [ONNX] Add MatMulInteger16 contrib op

* Fix formatting errors

* Remove a code comment and do not set default value of nd

* Move flatten_to_nd function outside matmul to be used across multiple functions

* Add function docstring and describe the tests

* Use max/min value of int16 as high/low while generating input vectors

* Converge MatMul and MatMulInteger16 ops into a single op using output dtype

* Fix indentation issues

* Formatting changes

* Fix CUDA batchmatmul strategy to allow mixed precision

* Add test_matmulinteger to unsupported_onnx_tests
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@abhikran-quic @ylc
[ONNX] Add MatMulInteger16 contrib op (apache#9186)
4134b45 
* [ONNX] Add MatMulInteger16 contrib op

* Fix formatting errors

* Remove a code comment and do not set default value of nd

* Move flatten_to_nd function outside matmul to be used across multiple functions

* Add function docstring and describe the tests

* Use max/min value of int16 as high/low while generating input vectors

* Converge MatMul and MatMulInteger16 ops into a single op using output dtype

* Fix indentation issues

* Formatting changes

* Fix CUDA batchmatmul strategy to allow mixed precision

* Add test_matmulinteger to unsupported_onnx_tests
@Icemist Icemist mentioned this pull request Jan 13, 2022
Merged
@driazati driazati mentioned this pull request Jul 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cconvey cconvey cconvey left review comments

@AndrewZhaoLuo AndrewZhaoLuo AndrewZhaoLuo approved these changes

@tmoreau89 tmoreau89 tmoreau89 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

@mbrookhart mbrookhart Awaiting requested review from mbrookhart

@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

@icemelon icemelon Awaiting requested review from icemelon

@anijain2305 anijain2305 Awaiting requested review from anijain2305

@MarisaKirisame MarisaKirisame Awaiting requested review from MarisaKirisame

@slyubomirsky slyubomirsky Awaiting requested review from slyubomirsky

@vinx13 vinx13 Awaiting requested review from vinx13

@wweic wweic Awaiting requested review from wweic

@zhiics zhiics Awaiting requested review from zhiics

@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
@abhikran-quic @tmoreau89 @AndrewZhaoLuo @Icemist @cconvey