NXP backend: added support for aten.conv_transpose1 and refactored convolution_converter#19004
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19004
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 2b79e1a with merge base a49171d ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "release notes: nxp" |
|
@pytorchbot label "module: nxp" |
There was a problem hiding this comment.
Pull request overview
Adds NXP backend support for aten.conv_transpose1d by moving 1D-to-2D convolution lowering out of the IR converter and into a dedicated ATen graph rewrite pass, plus adjusts quantization handling for grouped transposed convolutions.
Changes:
- Introduce
ConvertConv1dToConv2dPassto rewriteaten.conv1d/aten.conv_transpose1dinto 2D equivalents via unsqueeze/conv2d(or conv_transpose2d)/squeeze. - Remove 1D-convolution handling from the TFLite
convolution_converterand enable the new pass in the default Neutron ATen pass pipeline. - Update quantizer patterns/utilities to correctly derive bias qparams for grouped
conv_transpose2dand fix per-channel axis handling; add comprehensive tests for the new pass.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backends/nxp/aten_passes/convert_1d_conv_to_2d.py | New ATen pass converting 1D conv/transposed conv to 2D form with shape/meta propagation. |
| backends/nxp/aten_passes/neutron_aten_pass_manager.py | Registers the new pass in the default Neutron ATen pass sequence. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/convolution_converter.py | Removes 1D convolution conversion logic; now expects only 2D weights rank. |
| backends/nxp/quantizer/utils.py | Adds helper to “pad”/repeat weight scales when deriving bias qparams for grouped transposed conv. |
| backends/nxp/quantizer/patterns.py | Drops 1D conv patterns; updates ConvTranspose2d quantization (bias qparams + correct per-channel axis). |
| backends/nxp/quantizer/neutron_quantizer.py | Removes the Conv1dPattern registration (since 1D conv is rewritten earlier). |
| backends/nxp/tests/test_convert_1d_conv_to_2d.py | New test suite covering conv1d + conv_transpose1d rewrite and full pipeline delegation. |
| backends/nxp/tests/models.py | Updates Conv1d test module API and adds ConvTranspose1d + runtime-weight conv1d models for testing. |
| backends/nxp/tests/ir/converter/node_converter/test_conv_converter.py | Removes prior conv1d conversion tests (superseded by new pass tests). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8568135 to
73e97a6
Compare
|
Refactored the whole solution. Solved cases where there was |
|
Please review @MartinPavella and @StrycekSimon. Thank you 😊 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Great work!
I only have cosmetic suggestions and questions. Perhaps for future PRs you could split the implementation into multiple commits. Ultimately it would still get squashed to 1, but it would make the review easier as the commit messages would explain the motivation for some changes. Here I'm really not sure why some changes were made.
Also Copilot had some good comments, so please address them too if you see fit.
StrycekSimon
left a comment
There was a problem hiding this comment.
Please, handle the 2D inputs or at least mark them as unsupported and create an issue.
73e97a6 to
1578982
Compare
|
Enabled Fixed some of the issues and provided comments on some. Please review it @StrycekSimon @MartinPavella. Thank you! 😊 |
|
LGTM. Please resolve the conflicts so the tests can run. |
1578982 to
de6a90a
Compare
|
Rebased and resolved all comments. Please run the tests @MartinPavella |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
backends/nxp/backend/ir/converter/node_converters/ops_converters/convolution_converter.py:437
- After removing in-converter 1D convolution lowering, the
NotImplementedErrormessage will be hit if a model reaches IR conversion with rank-3 weights (i.e., theConvertConv1dToConv2dPasswasn’t run). Consider updating the error message to explicitly point users toNeutronAtenPassManager/ConvertConv1dToConv2dPassso the failure is actionable.
rank = t_op.tmp_inputs[1].shape.len()
if rank == 4: # Conv2D
ops_to_add = self._convert_2d_conv(t_op, conv_params)
else:
raise NotImplementedError(
f"{rank - 2}D convolution is not supported."
) # Should never get here.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
de6a90a to
4a73b63
Compare
|
Fixed some issues mentioned by Copilot. |
4a73b63 to
6e9b978
Compare
6e9b978 to
2b79e1a
Compare
|
@MartinPavella Hopefully everything will pass now, please run the tests. Thank you 😊 |
MartinPavella
left a comment
There was a problem hiding this comment.
LGTM. Once tests pass, we can merge.
|
Internal tests are passing. The Github failing tests don't seem to be related to NXP code. |
|
The failing checks seem unrelated and the internal CI is passing. Merging. |
Summary
Added support for
aten.conv_transpose1dby moving functionality fromconvolution_converterto brand newconvert_1d_conv_to2daten pass, and extending it.Test plan
tests can be manually run using
pytest -c /dev/null backends/nxp/tests/cc @robert-kalmar @JakeStevens @digantdesai @MartinPavella