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

[QNN] Support CallNode inputs in qnn.concatenate #5360

Merged
merged 9 commits into from
May 5, 2020

Conversation

mbaret
Copy link
Contributor

@mbaret mbaret commented Apr 17, 2020

Currently, qnn.concatenate assumes that its 1st arg (data) is a TupleNode. This may not necessarily be true if the input is a CallNode which returns a value of tuple-type. This patch handles the CallNode case by inserting TupleGetItemNodes.

@anijain2305
Copy link
Contributor

@zhiics

@zhiics
Copy link
Member

zhiics commented Apr 17, 2020

I would suggest we add a unit test for this case.

@mbaret
Copy link
Contributor Author

mbaret commented Apr 20, 2020

Is there somewhere that QnnCanonicalize is tested where I can add a test?

@anijain2305
Copy link
Contributor

You can add it here - tests/python/relay/test_op_qnn_concatenate.py
It calls QNNCanonicalize internally

@mbaret
Copy link
Contributor Author

mbaret commented Apr 22, 2020

I'm finding it difficult to test this case because in the Python API for qnn.concatenate the data value is expected to be a tuple (not a tuple-type call). Do you think it makes sense to relax this requirement in the Python API and let it fail in C++ instead?

@mbaret mbaret force-pushed the concat-fix branch 2 times, most recently from 467346f to 9567256 Compare April 29, 2020 14:42
@mbaret
Copy link
Contributor Author

mbaret commented Apr 29, 2020

@anijain2305 @zhiics could you take a look at the changes I've made and see if this is now OK?

Copy link
Contributor

@anijain2305 anijain2305 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 for adding the test.

python/tvm/relay/qnn/op/qnn.py Outdated Show resolved Hide resolved
src/relay/qnn/op/concatenate.cc Outdated Show resolved Hide resolved
Currently, qnn.concatenate assumes that its 1st arg
(data) is a TupleNode. This may not necessarily be true
if the input is a CallNode which returns a value of
tuple-type. This patch handles the CallNode case by
inserting TupleGetItemNodes.
Change-Id: I40b55517b8b1dabbeca89337f80c0c8e62e34981
Change-Id: I731a231113c5214528373ef52b603a9f05ec502a
Change-Id: Ib3495532f6e4feb5aae3d3096cedd4dc4676cdb4
Change-Id: Id8123ea2dd9ce3d8267609de7b5602bb84b084fb
Change-Id: Ib6899bb22260575aa3f5d8b51b5d2a0277ee2b10
Change-Id: I56cf1930315344e42d956818a6c68e80836ae786
@mbaret
Copy link
Contributor Author

mbaret commented May 1, 2020

@zhiics @anijain2305 could you take another look at this please?

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

Only a small nitpick

Change-Id: I3edab192e32bafa9ffdc915315791c63279d85dc
@anijain2305 anijain2305 merged commit 32a094c into apache:master May 5, 2020
@anijain2305
Copy link
Contributor

Thanks @mbaret @zhiics This is merged

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 9, 2020
* [QNN] Support CallNode inputs in qnn.concatenate

Currently, qnn.concatenate assumes that its 1st arg
(data) is a TupleNode. This may not necessarily be true
if the input is a CallNode which returns a value of
tuple-type. This patch handles the CallNode case by
inserting TupleGetItemNodes.

* Fix lint

* Add test

Change-Id: I40b55517b8b1dabbeca89337f80c0c8e62e34981

* Use isinstance

Change-Id: I731a231113c5214528373ef52b603a9f05ec502a

* isinstance fix

Change-Id: Ib3495532f6e4feb5aae3d3096cedd4dc4676cdb4

* Use elif/else if

Change-Id: Id8123ea2dd9ce3d8267609de7b5602bb84b084fb

* Fix lint

Change-Id: Ib6899bb22260575aa3f5d8b51b5d2a0277ee2b10

* Lint fix

Change-Id: I56cf1930315344e42d956818a6c68e80836ae786

* Spaces

Change-Id: I3edab192e32bafa9ffdc915315791c63279d85dc
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 18, 2020
* [QNN] Support CallNode inputs in qnn.concatenate

Currently, qnn.concatenate assumes that its 1st arg
(data) is a TupleNode. This may not necessarily be true
if the input is a CallNode which returns a value of
tuple-type. This patch handles the CallNode case by
inserting TupleGetItemNodes.

* Fix lint

* Add test

Change-Id: I40b55517b8b1dabbeca89337f80c0c8e62e34981

* Use isinstance

Change-Id: I731a231113c5214528373ef52b603a9f05ec502a

* isinstance fix

Change-Id: Ib3495532f6e4feb5aae3d3096cedd4dc4676cdb4

* Use elif/else if

Change-Id: Id8123ea2dd9ce3d8267609de7b5602bb84b084fb

* Fix lint

Change-Id: Ib6899bb22260575aa3f5d8b51b5d2a0277ee2b10

* Lint fix

Change-Id: I56cf1930315344e42d956818a6c68e80836ae786

* Spaces

Change-Id: I3edab192e32bafa9ffdc915315791c63279d85dc
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 18, 2020
* [QNN] Support CallNode inputs in qnn.concatenate

Currently, qnn.concatenate assumes that its 1st arg
(data) is a TupleNode. This may not necessarily be true
if the input is a CallNode which returns a value of
tuple-type. This patch handles the CallNode case by
inserting TupleGetItemNodes.

* Fix lint

* Add test

Change-Id: I40b55517b8b1dabbeca89337f80c0c8e62e34981

* Use isinstance

Change-Id: I731a231113c5214528373ef52b603a9f05ec502a

* isinstance fix

Change-Id: Ib3495532f6e4feb5aae3d3096cedd4dc4676cdb4

* Use elif/else if

Change-Id: Id8123ea2dd9ce3d8267609de7b5602bb84b084fb

* Fix lint

Change-Id: Ib6899bb22260575aa3f5d8b51b5d2a0277ee2b10

* Lint fix

Change-Id: I56cf1930315344e42d956818a6c68e80836ae786

* Spaces

Change-Id: I3edab192e32bafa9ffdc915315791c63279d85dc
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.

3 participants