Skip to content

NXP backend: Enable amin with new Neutron flow#20549

Open
roman-janik-nxp wants to merge 1 commit into
pytorch:mainfrom
nxp-upstream:feature/nxg11066/EIEX-890-Add-amin-support-using-new-Neutron-flow
Open

NXP backend: Enable amin with new Neutron flow#20549
roman-janik-nxp wants to merge 1 commit into
pytorch:mainfrom
nxp-upstream:feature/nxg11066/EIEX-890-Add-amin-support-using-new-Neutron-flow

Conversation

@roman-janik-nxp

@roman-janik-nxp roman-janik-nxp commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add tests verifying correct support for amin by the Neutron backend using the new Neutron MLIR flow.

Test plan

Unit tests provided.

cc @robert-kalmar

@roman-janik-nxp roman-janik-nxp 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 Jun 26, 2026
@pytorch-bot

pytorch-bot Bot commented Jun 26, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 8 New Failures, 123 Unrelated Failures

As of commit 368a3f2 with merge base e095c57 (image):

NEW FAILURES - The following jobs have failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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.

@meta-cla meta-cla 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 Jun 26, 2026
@linux-foundation-easycla

linux-foundation-easycla Bot commented Jun 26, 2026

Copy link
Copy Markdown

CLA Not Signed

Comment on lines +46 to +49
if is_alone_in_partition and keepdim and all(input_shape[d] == 1 for d in dim):
# The operator is a no-op, so the Neutron Converter will skip it. If it's the only node in the
# partition, the graph would end up empty.
return False

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Well done for considering this case 👍🏻

Just FYI, NeutronPartitioner automatically tries to identify all no-ops by running every operator selected for delegation with random data. If it produces outputs that are exactly equal to the inputs (data, shape, type), it considers the node to be a no-op, and it will not delegate it as long as the operator is in a list of allowed no-ops. In the case of amin, I believe this system would have always correctly identified the no-ops, so simply adding the aten.amin into the list of allowed no-ops would have done the job.

But your solution is valid as well. No change needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I learn from the best (you 😄 ). However I don't see any logic for said identifying in NeutronPartitioner and when I omit these checks in supports_partitioning_result() the tests fail. So I'm keeping it.

Comment on lines +76 to +77
if not NodeConverter._has_shared_q_params_if_quantized(node):
return False

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where did you find this requirement?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In your mean dim converter, I kept it, but I don't think it's necessary.

@MartinPavella MartinPavella Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It depends on the requirements of Neutron/Neutron IR. Often we can see requirements for equal input and output quantization here: https://developers.google.com/edge/litert/conversion/tensorflow/quantization/quantization_spec
The spec is, however, incomplete. It doesn't mention the ReduceMin operator. The Neutron documentation doesn't mention any such restrictions, so I think we should remove it. Also note that you used the shared quantization spec in our quantizer for the amin operator, so please update that too.

P.S.: We can handle mean later.

Comment thread backends/nxp/backend/ir/converter/node_converters/shared/reduce_utils.py Outdated
Comment thread backends/nxp/backend/node_format_inference.py Outdated
@roman-janik-nxp roman-janik-nxp changed the title NXP backend: Enable min amin with new Neutron flow NXP backend: Enable amin with new Neutron flow Jun 29, 2026
@roman-janik-nxp roman-janik-nxp force-pushed the feature/nxg11066/EIEX-890-Add-amin-support-using-new-Neutron-flow branch from c780806 to b7e0a71 Compare June 29, 2026 18:11
@roman-janik-nxp roman-janik-nxp force-pushed the feature/nxg11066/EIEX-890-Add-amin-support-using-new-Neutron-flow branch from b7e0a71 to dd1f281 Compare June 30, 2026 16:15
@roman-janik-nxp roman-janik-nxp force-pushed the feature/nxg11066/EIEX-890-Add-amin-support-using-new-Neutron-flow branch from dd1f281 to 368a3f2 Compare June 30, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.

2 participants