-
Notifications
You must be signed in to change notification settings - Fork 837
Reverts the addition of RemoveNoopPass to the Cortex-M pass manager…
#17407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… (introduced in #17300) which broke 8 quantization tests. After #17300 merged, the following tests started failing: - `test_shared_qspec_quantizer[input_fork_x_shared]` - `test_shared_qspec_quantizer[input_fork_y_shared]` - `test_shared_qspec_quantizer[surrounded_quantized_op]` - `test_shared_qspec_quantizer[output_fork_shared]` - `test_shared_qspec_quantizer[many_forks]` - `test_dialect_mv2[mobilenet_v2]` (+ 2 more) `RemoveNoopPass` removes `clone_dim_order` operators that are functionally necessary for shared quantization specs. It only checks dtype equality, not dim_order/layout changes, causing it to incorrectly remove clones needed for: - Tensor forking in quantized graphs - Shared quantization parameter propagation - Correct quantization fusion Remove `RemoveNoopPass` from the Cortex-M pass pipeline. The MobileNetV2 `clone_dim_order` issue it was meant to address needs a more targeted solution. ```bash pytest -c backends/arm/test/pytest.ini backends/cortex_m/test/misc/test_quantization.py::test_shared_qspec_quantizer -v pytest -c backends/arm/test/pytest.ini backends/cortex_m/test/models/test_mobilenet_v2.py::test_dialect_mv2 -v
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/17407
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 84 PendingAs of commit abadaa6 with merge base 429925d ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Reverts the recently-added RemoveNoopPass from the Cortex-M pass pipeline to prevent incorrect removal of _clone_dim_order ops that are required for shared quantization spec behavior (fixing the quantization test regressions introduced in #17300).
Changes:
- Remove
RemoveNoopPassfromCortexMPassManagerto preserve necessaryclone_dim_orderops. - Update the MobileNetV2 Cortex-M dialect test expectations to include one remaining
clone_dim_orderop after transforms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backends/cortex_m/passes/cortex_m_pass_manager.py | Drops RemoveNoopPass from the Cortex-M pass list to avoid stripping required dim-order clones. |
| backends/cortex_m/test/models/test_mobilenet_v2.py | Updates expected op counts to reflect that clone_dim_order is no longer removed by the pass pipeline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Failing mcu test will be fixed by this #17405 |
|
Hi, have you considered adding cortex-m unittests to your CI? |
|
Now the mv2 test is failing instead... |
Should be fixed by #17405. Yes adding unittests in the next PR, moreover enabling test-mcu-cortex-m-backend / linux-job to run on diff times |
… (introduced in #17300) which broke 8 quantization tests.
After #17300 merged, the following tests started failing:
test_shared_qspec_quantizer[input_fork_x_shared]test_shared_qspec_quantizer[input_fork_y_shared]test_shared_qspec_quantizer[surrounded_quantized_op]test_shared_qspec_quantizer[output_fork_shared]test_shared_qspec_quantizer[many_forks]test_dialect_mv2[mobilenet_v2](+ 2 more)RemoveNoopPassremovesclone_dim_orderoperators that are functionally necessary for shared quantization specs. It only checks dtype equality, not dim_order/layout changes, causing it to incorrectly remove clones needed for:Remove
RemoveNoopPassfrom the Cortex-M pass pipeline. The MobileNetV2clone_dim_orderissue it was meant to address needs a more targeted solution.