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

[microNPU] Add support for TFLite PAD #13732

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

Aleksei-grovety
Copy link
Contributor

A separate nn.pad relay operator is legalized to an Ethos-U depthwise_conv2d operator. For ethosu_depthwise_conv2d the hardware only supports padding up to 31, 31, 32, 32, 32, so the pad size for legalization on the NPU is within these limits.

cc @leandron, @ekalda, @lhutton1

A separate nn.pad relay operator is legalized to an Ethos-U depthwise_conv2d operator.
For ethosu_depthwise_conv2d the hardware only supports padding up to 31, 31, 32, 32, 32,
so the pad size for legalization on the NPU is within these limits.
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 9, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@github-actions github-actions bot requested a review from leandron January 9, 2023 09:52
Copy link
Contributor

@ekalda ekalda left a comment

Choose a reason for hiding this comment

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

Thanks @alexey-yazev , I think it's good to go, but I'll leave it open for little longer in case anybody else wants to have a look.

Comment on lines +1373 to +1375
channels_map = {
"NHWC": 3,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC this one entry channels_map is a historic relic that can go and so simplify the code little bit, but as it is present in other operators, it's probably a clean up task for some other time.

Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks @alexey-yazev, LGTM! After looking at how padding is lowered in Vela I think there might be a couple more opportunities for optimization, although it seems out of scope for this PR. Just a couple of things to consider in the future:

@lhutton1 lhutton1 merged commit 6b65a59 into apache:main Jan 9, 2023
@lhutton1
Copy link
Contributor

lhutton1 commented Jan 9, 2023

Thanks @alexey-yazev, @ekalda!

@lhutton1
Copy link
Contributor

lhutton1 commented Jan 9, 2023

Oops I forgot to ask, would it be possible to add a legalization test under: tests/python/contrib/test_ethosu/test_legalize.py in a follow-up? Apologies for missing this before!

@arina-grovety
Copy link
Contributor

Hello @lhutton1, thanks for the review!

Yes, this was the first option that we tried to implement. But in the Vela implementation, this is done by "copies IFM to the right place inside the OFM" using write_offset attribute of the created AvgPool operation.

In the TVM, VelaAPI operations are derived from the NpuOperation class, which does not have a write_offset attribute, so we cannot replicate Vela convert_pad() function.

We tried to implement PAD legalization using the Concatenate operation but encountered an error. Seems like Cascader must be turned off for Concatenate to work. For example, Cascader is disabled in test_tflite_concat() (if Cascader is enabled, there is the same error as we have with the Concatenate)

So far, the most feasible option seems to use several depthwise_conv2d operators, if padding exceeds [31, 31, 32, 32].

But of course, I do not have all the knowledge about this, maybe there are other options?

@Aleksei-grovety Aleksei-grovety deleted the ethosu-add-separate-pad branch January 10, 2023 13:07
@Aleksei-grovety
Copy link
Contributor Author

Oops I forgot to ask, would it be possible to add a legalization test under: tests/python/contrib/test_ethosu/test_legalize.py in a follow-up? Apologies for missing this before!

Test was added in PR

@lhutton1
Copy link
Contributor

Thanks @arina-grovety for the explanation, just following up on some of the questions...

I suspect this is a case of needing to expose this functionality from within Vela, I'll see if we can make this happen for a future Vela release.

The concatenate error does indeed sound like a separate issue in itself. It might be worth investigating the reason for that at some point.

fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
A separate nn.pad relay operator is legalized to an Ethos-U depthwise_conv2d operator.
For ethosu_depthwise_conv2d the hardware only supports padding up to 31, 31, 32, 32, 32,
so the pad size for legalization on the NPU is within these limits.
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.

5 participants