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

Fix QNN type inference #7074

Merged
merged 3 commits into from
Dec 10, 2020
Merged

Fix QNN type inference #7074

merged 3 commits into from
Dec 10, 2020

Conversation

mbrookhart
Copy link
Contributor

@masahi
Copy link
Member

masahi commented Dec 10, 2020 •
edited
Loading

@anijain2305 So after #6704, it seems type inferencer can pass IncompleteType to QNN type rel functions, which by itself is not wrong. Previously, @mbrookhart applied the same fix to the dynamic op type relation functions to make type inference pass.

masahi
masahi approved these changes Dec 10, 2020
View reviewed changes
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.

Thank you very much!! @mbrookhart

jwfromm
jwfromm requested changes Dec 10, 2020
View reviewed changes
src/relay/qnn/op/op_common.h Outdated Show resolved Hide resolved
src/relay/qnn/op/requantize.cc Show resolved Hide resolved
tests/python/frontend/pytorch/qnn_test.py Show resolved Hide resolved
@jwfromm
Copy link
Contributor

jwfromm commented Dec 10, 2020

I'm a little confused what exactly is being type checked. The comments say its scale and zero points but the number of checked types in the loop don't match up. A little better documentation would go a long way.

@mbrookhart
Copy link
Contributor Author

@jwfromm These functions are a little odd, they often check types for some of the scales/zero points and then run assignments on others I would expect to be inputs. I assume there is a reason for this, perhaps @anijain2305 knows? Anyway, to make it work, I only added the return false on those input types we actually end up checking.

I just pushed a bunch of comments for what we expect in the types vector, I hope that helps.

jwfromm
jwfromm approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@jwfromm jwfromm 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 ffb6029 into apache:main Dec 10, 2020
@masahi
Copy link
Member

masahi commented Dec 10, 2020

Thanks @mbrookhart @jwfromm

TusharKanekiDey pushed a commit to TusharKanekiDey/tvm that referenced this pull request Jan 20, 2021
Fix QNN type inference (apache#7074)
faa95d3 
* check for incomplete types in QNN Relation functions

* add regression test from apache#7067

* respond to review comments
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jan 21, 2021
@trevor-m
Fix QNN type inference (apache#7074)
c3ca260 
* check for incomplete types in QNN Relation functions

* add regression test from apache#7067

* respond to review comments
electriclilies pushed a commit to electriclilies/tvm that referenced this pull request Feb 18, 2021
@electriclilies
Fix QNN type inference (apache#7074)
e56f484 
* check for incomplete types in QNN Relation functions

* add regression test from apache#7067

* respond to review comments
@junrushao junrushao mentioned this pull request Nov 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masahi masahi masahi approved these changes

@jwfromm jwfromm jwfromm approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@mbrookhart @masahi @jwfromm