NXP backend - added support for aten.conv2d using the new Neutron flow#19717
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19717
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 3 New Failures, 2 Unrelated FailuresAs of commit dc7cd68 with merge base 252f57c ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "module: nxp" |
|
@pytorchbot label "release notes: nxp" |
|
Please also review @MartinPavella @roman-janik-nxp @StrycekSimon. Thank you 😄 |
There was a problem hiding this comment.
Pull request overview
Adds aten.conv2d delegation coverage for the NXP Neutron “new flow” and expands the NXP backend test suite to validate delegation behavior and (where possible) numerical correctness.
Changes:
- Add a new-flow support-check path in the convolution converter and tighten failure behavior when group conv decomposition is missing.
- Add extensive new pytest coverage for conv2d delegation/non-delegation scenarios under the new Neutron flow (including depthwise and edge/bounds cases, plus targeted
xfails for known issues). - Improve NPU-vs-CPU mismatch reporting in the output comparator by including the max diff in the assertion message.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backends/nxp/tests/model_output_comparator.py | Improves assertion failure message for output mismatches. |
| backends/nxp/tests/ir/converter/node_converter/test_conv_converter.py | Adds a large new test suite for conv2d under the new Neutron flow, including delegation checks and known-issue xfails. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/convolution_converter.py | Introduces new-flow target support checks and adjusts convolution argument handling / group-conv behavior. |
| backends/nxp/aten_passes/split_group_convolution.py | Updates group-conv splitting pass to handle optional bias and propagate FakeTensor meta safely. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9cedbc4 to
bc6ecdf
Compare
|
Fixed the issues found by Copilot. |
MartinPavella
left a comment
There was a problem hiding this comment.
Will review the rest of test_conv_converter tomorrow.
|
@novak-vaclav please take a look at this issue: https://jira.sw.nxp.com/browse/EIEX-809 |
bc6ecdf to
e6db65f
Compare
|
|
Fixed issues mentioned by @MartinPavella and updated the tests so they are based on Neutron SW 3.1.2. The support for padding mode |
e6db65f to
1f46333
Compare
|
Fixed issues mentioned by Copilot. Note: I am not using the |
|
Now I see I will have to rebase onto the Neutron SW 3.1.2 branch, will do |
MartinPavella
left a comment
There was a problem hiding this comment.
LGTM 👍🏻
I will approve after you rebase to the 3.1.2 PR
1f46333 to
ba6aee9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 50 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
backends/nxp/tests/ir/converter/node_converter/test_conv_converter.py:1
AllCloseOutputComparatoris imported fromexecutorch.backends.nxp.tests.nsys_testing, but in this PR other test modules import output comparators fromexecutorch.backends.nxp.tests.model_output_comparator. Ifnsys_testingdoesn’t define/re-exportAllCloseOutputComparator, this will fail at import time. Import the comparator from its actual module (or re-export it explicitly innsys_testing/a shared test utilities module) to keep imports consistent and avoid test collection errors.
examples/nxp/setup.sh:1- The
--index-urlvalue is unquoted; ifEIQ_PYPI_URLever contains special characters (or is empty/oddly formatted), word-splitting can break the command. Quote the variable (\"${EIQ_PYPI_URL}\") to make the script more robust.
ba6aee9 to
469260c
Compare
270b744 to
5a99b69
Compare
5a99b69 to
735d329
Compare
|
@novak-vaclav I noticed that the job was killed after 90 minutes. Tested 2 times, the timeout triggered during completely different tests and timestamps suggest that nothing froze. Seems like the added I increased the limit to 120 minutes in the last commit. |
3a8765d to
24f5700
Compare
9b5c5df to
69a80ef
Compare
9f9ccde to
2692e90
Compare
2692e90 to
7a1fc36
Compare
|
Rebased onto current main, (hopefully) resolved all test issues. Let's see |
7a1fc36 to
a917d53
Compare
|
Hopefully now it will all be resolved. |
a917d53 to
dc7cd68
Compare
|
Internal tests are passing. The opensource CI test failures do not seem to be related. |
Summary
Added support for
aten.conv2dusing new Neutron flow.Test plan
tests can be manually run using
pytest -c /dev/null backends/nxp/tests/cc @robert-kalmar @JakeStevens @digantdesai @rascani @MartinPavella