NXP backend: Enable Amax with new Neutron flow#20628
Conversation
roman-janik-nxp
requested review from
mergennachin and
robert-kalmar
as code owners
roman-janik-nxp
added
the
release notes: nxp
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20628
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 1 PendingAs of commit 910e187 with merge base 3bd4748 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
roman-janik-nxp
added
the
module: nxp
roman-janik-nxp
changed the title
roman-janik-nxp
force-pushed
the
feature/nxg11066/EIEX-890-Add-amax-support-using-new-Neutron-flow
branch
from
3fa4f11 to
dab563d
Compare
roman-janik-nxp
force-pushed
the
feature/nxg11066/EIEX-890-Add-amax-support-using-new-Neutron-flow
branch
from
dab563d to
910e187
Compare
| # Just 1 test case to verify correct handling of the `dim`. | ||
| # Most cases fall into the single bit error case, and since this test uses 2 operators, the error accumulates | ||
| # and the final error is larger. We cannot with 100% certainty say that the error is only caused by the single | ||
| # bit errors and not related to the format. That's why only this 1 case with no errors is used. |
There was a problem hiding this comment.
It may happen that there is a two-bit error, however it is not a good idea to selectively only include test cases that pass.
I'd recommend allowing two bit error or not using remove_quant_io_ops=True in this test.
|
|
||
| class AmaxModule(torch.nn.Module): | ||
| def __init__( | ||
| self, dim: int | torch.Size | list[int] | tuple[int, ...], keepdim: bool |
There was a problem hiding this comment.
Nit: According to the documentation, dim can only be int, tuple of ints, and can be None. keepdim can be None. This is also true for MaxPoolAmaxModule.
I think it would be better to add two test cases that use the implicit value of dim and keepdim.
| self._assert_neutron_ir_model_has_ops( | ||
| model_builder_finish_spy, | ||
| expected_ops=[ | ||
| Transpose, |
There was a problem hiding this comment.
nit: in the comment above, you say no Transpose ops are added. Please clarify the comment if you meant to say "no Transpose ops are added after MaxPool2D".
2 participants
Summary
Add tests verifying correct support for amax by the Neutron backend using the new Neutron MLIR flow.
Test plan
Unit tests provided.
cc @robert-kalmar