Skip to content

NXP backend: Add support for depthwise and separable convolution. #11215

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
Merged
merged 2 commits into from
Jul 10, 2025
Merged

NXP backend: Add support for depthwise and separable convolution. #11215

merged 2 commits into from
Jul 10, 2025
+924 −58

Conversation

robert-kalmar
Copy link
Collaborator

@robert-kalmar robert-kalmar commented May 29, 2025 •
edited by pytorch-bot bot
Loading

Summary

Adding depthwise convolution support for the NXP backend

Test plan

Functionality tested by python unit tests:

pytest -c /dev/null/ backend/nxp/tests

cc @digantdesai @JakeStevens , @Pop-korn

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 29, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/11215

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures, 2 Cancelled Jobs

As of commit be19da6 with merge base 75e4044 (image):

NEW FAILURES - The following jobs have failed:

CANCELLED JOBS - The following jobs were cancelled. Please retry:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 29, 2025
@robert-kalmar
Copy link
Collaborator Author

@pytorchbot label "module: nxp" "release notes: nxp"

@pytorch-bot pytorch-bot bot added module: nxp Issues related to NXP Neutron NPU delegation and code under backends/nxp/ release notes: nxp Changes to the NXP Neutron backend delegate labels May 29, 2025
digantdesai
digantdesai reviewed Jun 3, 2025
View reviewed changes
backends/nxp/tests/ir/converter/node_converter/test_conv_converter.py
assert len(nodes) == 11
assert (
nodes[7].target.__name__ == "aten.convolution.default"
) # Convolution not delegated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Love these tests.

@digantdesai
Copy link
Contributor

Just skimmed, let me take a close look in a day or so, esp _convert_2d_conv. Thanks

@robert-kalmar robert-kalmar force-pushed the upstream/main-nxp/depthwise-convolution branch from 8e64288 to cb91593 Compare June 10, 2025 11:09
@robert-kalmar
Copy link
Collaborator Author

Ready for review. Most of the tests passing. The failing seems unrelated to the changes:

@JacobSzwejbka
Copy link
Contributor

digant is out for a while I will try to find another person to review these

@robert-kalmar robert-kalmar marked this pull request as draft June 18, 2025 12:11
@robert-kalmar
Copy link
Collaborator Author

Converting to draft unless the NXP Backend CI is back (#11756)

@JakeStevens
Copy link
Contributor

digant is out for a while I will try to find another person to review these

I am back, I can review this coming week.

@robert-kalmar
Copy link
Collaborator Author

digant is out for a while I will try to find another person to review these

I am back, I can review this coming week.

This is yet a draft, as we had problems with the eIQ PyPI repository. For some reason the eIQ PyPI and the executorch self-hosted runners does not talk to each other [https://github.com//pull/11612].

Let have initially resolve that one here #11756. Then we will mark the PR as ready for review and add you (and Digant for refefence) as reviewers.

@StrycekSimon StrycekSimon force-pushed the upstream/main-nxp/depthwise-convolution branch from 07f79c2 to 774b8ef Compare July 1, 2025 07:10
@JakeStevens
Copy link
Contributor

@robert-kalmar is this ready now?

@robert-kalmar robert-kalmar marked this pull request as ready for review July 8, 2025 10:14
@robert-kalmar
Copy link
Collaborator Author

@robert-kalmar is this ready now?

It is ready for review and merging now.

@robert-kalmar robert-kalmar requested a review from JakeStevens July 8, 2025 13:13
JakeStevens
JakeStevens reviewed Jul 8, 2025
View reviewed changes
backends/nxp/backend/ir/converter/node_converters/shared/conv_utils.py
return False # Unable to split group Conv into separated convolutions because out_channels % group != 0.

# 10 is an empirical value. The `group` directly dictates how many branches will be created.
return 2 <= group <= 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Why limit the number of groups, is this a HW limit?

Is there any logging in this node converters? This will be relatively opaque if it fails since we return False for multiple different reasons

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently the Neutron supports only regular convolution (groups ==1) and the Depthwise convolution (groups == # channels).

The compute of the general group convolution can be performed by splitting it into multiple regular convolutions, based on groups. However, in the current code base we decided to not support the generic group convolution at all - https://github.com/pytorch/executorch/pull/11215/files#diff-a5d5ec3b84b15dbe487d027b8a993cc6bb5795aef06e3589150bb2195c35b1b1R73 , only regular and depthwise cases are allowed for delegation.
AI: I will add more descriptive comment, instead of just "not supported"

The limitation here is rather an empiric value to not allow more that 10 convolutions, as it is harder to debug. And it is more as a code for future enablement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

✅
https://github.com/pytorch/executorch/pull/11215/files#diff-a5d5ec3b84b15dbe487d027b8a993cc6bb5795aef06e3589150bb2195c35b1b1R74

@robert-kalmar robert-kalmar force-pushed the upstream/main-nxp/depthwise-convolution branch from 774b8ef to 9812a45 Compare July 9, 2025 13:34
@pytorch-bot pytorch-bot bot removed the ciflow/trunk label Jul 9, 2025
Pop-korn added 2 commits July 9, 2025 15:42
@Pop-korn @robert-kalmar
NXP backend: Add support for depthwise and separable convolution.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
5dcf600 
The `group` attribute of the `aten.convolution` has an effect on how the weights are used in the computation of the convolution. To properly utilize Neutron, the convolution is sometimes converted to `DepthwiseConv2D`.
@Pop-korn @robert-kalmar
NXP backend: Prohibit delegation of convolution with batch size != 1.
Loading
Loading status checks…
be19da6 
Neutron only supports batch == 1.
@robert-kalmar robert-kalmar force-pushed the upstream/main-nxp/depthwise-convolution branch from 9812a45 to be19da6 Compare July 9, 2025 13:44
digantdesai
digantdesai approved these changes Jul 10, 2025
View reviewed changes
backends/nxp/backend/ir/converter/node_converters/ops_converters/convolution_converter.py
# Not supported natively by the eIQ Neutron so Group Separable Convolution.
# In practice it can be computed by splitting the Group Separable Convolution into multiple Pointwise
# Convo it will use the Split and Concat operation. The Concat operation in Neutron Converter
# SDK 25.03 requires the # of channels to be multipy of # of MAC units in the eIQ Neutron.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# SDK 25.03 requires the # of channels to be multipy of # of MAC units in the eIQ Neutron.
# SDK 25.03 requires the # of channels to be multiple of # of MAC units in the eIQ Neutron.

@digantdesai digantdesai merged commit a8d7298 into pytorch:main Jul 10, 2025
93 of 97 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeStevens JakeStevens

@digantdesai digantdesai

Assignees
No one assigned
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: nxp Issues related to NXP Neutron NPU delegation and code under backends/nxp/ release notes: nxp Changes to the NXP Neutron backend delegate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@robert-kalmar @digantdesai @JacobSzwejbka @JakeStevens @facebook-github-bot @Pop-korn