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

[ETHOSN] Transpose fully connected weights #12970

Merged
merged 5 commits into from
Oct 6, 2022

Conversation

lhutton1
Copy link
Contributor

@lhutton1 lhutton1 commented Oct 3, 2022

The NPU driver stack expects weights in IO (HWIO) format, however, Relay uses an OI representation. Although the shape of the weight tensor was correctly changed during codegen, the values in the weights tensor were not being transposed. This lead to an output mismatch when the output "units" was > 1. The tests didn't catch this due to using a weights tensor of all 1's.

cc @leandron @ashutosh-arm

@github-actions github-actions bot requested a review from leandron October 3, 2022 15:07
Copy link
Contributor

@ashutosh-arm ashutosh-arm left a comment

Choose a reason for hiding this comment

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

Thanks Luke for the fix. Surprised that none of the common networks picked it up. Just a few nits and questions.

The NPU driver stack expects weights in IO (HWIO) format, however, Relay
uses an OI representation. Although the shape of the weight tensor was
correctly changed during codegen, the values in the weights tensor were
not being transposed. This lead to an output mismatch when the output
"units" was > 1. The tests didn't catch this due to using a weights
tensor of all 1's.

Change-Id: I51b2bcd14b677280ef3b6a6845d56b7dfacc7d6a
@lhutton1 lhutton1 force-pushed the fully-connected-fix branch from e14bfcf to 66972bf Compare October 5, 2022 10:21
* Refactor use of weight transpose to common file between contrib
codegens.
* Make function areguments more explicit.
* Update network hashes.

Change-Id: Ib53bc7d2837b62908b92fd09062cbe9a8bb4ab30
@lhutton1 lhutton1 force-pushed the fully-connected-fix branch from 66972bf to 53b3383 Compare October 5, 2022 10:23
Fix lint
Change-Id: I6a1d9ffa8e3a747b7c77c9b27aa1b1c0d4c5cbff

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Change-Id: Ie89e429da222ffe17bc8faf831bf59217008a68a
@lhutton1
Copy link
Contributor Author

lhutton1 commented Oct 5, 2022

Thanks for the review @ashutosh-arm, this is ready for another look :)

Copy link
Contributor

@ashutosh-arm ashutosh-arm left a comment

Choose a reason for hiding this comment

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

Thanks Luke for making the changes. Looks much cleaner with a separate API. I've left few nits to follow up in the future.

Change-Id: Ie5ded2db3024b9e2c5095f01adea65798fc1da55
Copy link
Contributor

@ashutosh-arm ashutosh-arm 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 Luke 👍

Copy link
Contributor

@leandron leandron 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 @lhutton1!

@leandron leandron merged commit 1b9e20a into apache:main Oct 6, 2022
@lhutton1 lhutton1 deleted the fully-connected-fix branch November 10, 2022 08:49
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
* [ETHOSN] Transpose fully connected weights

The NPU driver stack expects weights in IO (HWIO) format, however, Relay
uses an OI representation. Although the shape of the weight tensor was
correctly changed during codegen, the values in the weights tensor were
not being transposed. This lead to an output mismatch when the output
"units" was > 1. The tests didn't catch this due to using a weights
tensor of all 1's.

Change-Id: I51b2bcd14b677280ef3b6a6845d56b7dfacc7d6a

* Address comments

* Refactor use of weight transpose to common file between contrib
codegens.
* Make function areguments more explicit.
* Update network hashes.

Change-Id: Ib53bc7d2837b62908b92fd09062cbe9a8bb4ab30

* Fix lint

Change-Id: I6a1d9ffa8e3a747b7c77c9b27aa1b1c0d4c5cbff

* Fix cmsis-nn weights transpose

Change-Id: Ie89e429da222ffe17bc8faf831bf59217008a68a

* Address comments

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

None yet

3 participants