NXP backend - added support for aten.conv2d using the new Neutron flow#19717
NXP backend - added support for aten.conv2d using the new Neutron flow#19717novak-vaclav wants to merge 2 commits into
aten.conv2d using the new Neutron flow#19717Conversation
🔗 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: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
@pytorchbot label "module: nxp" |
|
@pytorchbot label "release notes: nxp" |
pytorch-bot
Bot
added
module: 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.
novak-vaclav
force-pushed
the
feature/EIEX-861-add-conv2d-support-new-neutron-flow
branch
from
9cedbc4 to
bc6ecdf
Compare
|
Fixed the issues found by Copilot. |
MartinPavella
left a comment
MartinPavella
left a comment
There was a problem hiding this comment.
Will review the rest of test_conv_converter tomorrow.
|
|
||
| # The arguments of the conv are: | ||
| # [x, w, b, stride, padding, dilation, transposed, output padding, groups] | ||
| # https://github.com/pytorch/pytorch/blob/v2.6.0/aten/src/ATen/native/Convolution.cpp#L286-L291 |
There was a problem hiding this comment.
Nit:
This link only describes the arguments after x, w and b, so for example it doesn't mention that the bias is optional (as your code suggests).
Perhaps this could be a better source for this information: https://docs.pytorch.org/docs/main/user_guide/torch_compiler/torch.compiler_ir.html (mostly for future PRs)
convolution(Tensor input, Tensor weight, Tensor? bias, SymInt[] stride, SymInt[] padding, SymInt[] dilation, bool transposed, SymInt[] output_padding, SymInt groups) -> Tensor
| # strideH <= 4096, strideW <= 4096 | ||
| # dilationH <= 4096, dilationW <= 4096 | ||
| w_node_shape = ( | ||
| w_node.meta["val"].shape if hasattr(w_node, "meta") else w_node.shape |
There was a problem hiding this comment.
Have you encountered a case where the weights don't have the meta attribute?
I have not seen such a case yet and I believe we rely meta being available in our codebase.
| conv_utils.conv_op_factory, | ||
| ) | ||
| ): | ||
| raise RuntimeError("Group convolution was not decomposed.") |
There was a problem hiding this comment.
Nit:
Consider mentioning the NXP backend in the message. E.g. raise RuntimeError("NXP backend: Group convolution was not decomposed.").
| nodes = list(edge_program.graph.nodes) | ||
| assert len(nodes) == 15 | ||
| assert ( | ||
| nodes[11].target.__name__ == "aten.convolution.default" |
There was a problem hiding this comment.
Nit: Please also replace these assertions with ones that use the Convoultion alias.
| assert all_close | ||
| assert ( | ||
| all_close | ||
| ), f"NPU output doesn't match reference. Maximum absolute difference: {max_diff}" |
There was a problem hiding this comment.
Why duplicate the message?
If you think the error message should contain the max_diff, please remove the print statement and keep only the assert message.
| expected_non_delegated_ops={}, | ||
| ) | ||
| dataset = RandomDatasetCreator(low=-1, high=1) | ||
| comparator = AllCloseOutputComparator(atol=1) |
There was a problem hiding this comment.
Since you are using a float dataset in the range of [-1, 1), an absolute tolerance of 1 seems awfully high. Is it really intended?
|
|
||
| except AssertionError as e: | ||
| if "NPU output doesn't match reference" in str(e): | ||
| pytest.xfail( |
There was a problem hiding this comment.
This seems dangerous.
If I understand correctly, all tests (which don't fail with NeutronConverter) will pass. Either their error is low (so it passes), or the error is high and the test is marked with xfail (so it passes).
Is that really the behavior?
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