NXP backend: Add support for softmax with the new Neutron flow. - #19739
NXP backend: Add support for softmax with the new Neutron flow.#19739irtrukhina wants to merge 2 commits into
softmax with the new Neutron flow.#19739Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19739
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 9 PendingAs of commit 79b5ba5 with merge base d29706a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
154ffff to
56a1296
Compare
c8324ed to
c122421
Compare
| 1. Input and Output must be INT8/UINT8 | ||
| 2. Channels <= 2040 | ||
| 3. Total spatial size (H*W) <= 4096 | ||
| 3. Total spatial size <= 4096 |
There was a problem hiding this comment.
Nit: As far as I know total spatial size is not a universally known term. Before your change, it was supposed to represent H*W, whereas now it means N*H*W. Please keep the explicit definition here.
There was a problem hiding this comment.
Kept the explicit definition (NHW)
|
Please take a look at the canceled pull / unittest-nxp-neutron / linux-job (pull_request). Otherwise, LGTM |
502f045 to
f504223
Compare
f504223 to
8cc126d
Compare
8cc126d to
0d13bba
Compare
|
@MartinPavella, could you please take a final look at the PR? All tests have passed, and the documentation has been fixed to meet our checks. |
MartinPavella
left a comment
There was a problem hiding this comment.
Just a couple of observations to consider. No blockers.
| # Allow MSE up to the theoretical error introduced by 1-bit quantization (1/256). | ||
| comparator = NumericalStatsOutputComparator( | ||
| max_mse_error=1.0e-3, is_classification_task=True | ||
| max_mse_error=0.00390625, |
There was a problem hiding this comment.
I don't see how the dequantized value of a single-bit error directly dictates the MSE threshold. So the comment doesn't make sense to me. (same for the following test)
Also Nit: Using 1/256 would be cleaner.
There was a problem hiding this comment.
Sorry. This issue was caused by a combination of my brain overheating in the summer heat and a copy-paste error. The MSE should be the squared one-bit quantization error. I fixed the comparator settings and removed the comment.
| expected_delegated_ops={Softmax: 1}, | ||
| expected_non_delegated_ops={}, | ||
| ) | ||
| output_comparator = NumericalStatsOutputComparator( |
There was a problem hiding this comment.
Why are you using the NumericalStatsOutputComparator?
For other ops we use the AllCloseOutputComparator with a quantized dataset and atol=1. So it is clear only single-bit errors are tollerated.
There was a problem hiding this comment.
It is a very old PR and was created before we add remove_quant_io_ops option into lower_run_compare.
Replaced NumericalStatsOutputComparator with AllCloseOutputComparator(atol=1).
roman-janik-nxp
left a comment
There was a problem hiding this comment.
I recommend to fix the issues mentioned by Martin.
| ], | ||
| ) | ||
| def test_softmax_delegation__channel_first(input_shape, dim: int, mocker): | ||
| model = ConvSoftmaxModule(dim, input_shape[1]) |
There was a problem hiding this comment.
Is there are reason why no longer test channel first variant?
There was a problem hiding this comment.
It doesn't make sense to me. Neutron only supports channels-last format. Any errors in the channels-first version are more likely caused by incorrect dimension ordering than by the softmax operation.
These tests are specifically focused on validating softmax.
Do you agree?
There was a problem hiding this comment.
Well spotted @roman-janik-nxp, we should definitely have a test with a softmax after a convolution/maxpool/... node to make sure our SoftmaxConverter handles that edge case correctly.
@irtrukhina please add such a test.
Summary
Add support for the
softmaxto the Neutron backend using the new Neutron MLIR flow. Add new tests and update old tests for thesoftmaxas well.Test plan
New tests are included in the PR.
cc @robert-kalmar @JakeStevens @digantdesai @rascani