Cortex-M: share one to_edge config instead of five hand-copied ones - #21826
Open
rascani wants to merge 3 commits into
Open
Cortex-M: share one to_edge config instead of five hand-copied ones#21826rascani wants to merge 3 commits into
rascani wants to merge 3 commits into
Conversation
The to_edge configuration the Cortex-M backend needs was written out by hand in
five places, and four of them were wrong in different ways:
location linear hardsigmoid hardswish silu maxpool exc
AOT compiler yes yes yes NO NO
test harness yes yes yes yes yes
docs overview yes yes yes NO yes
examples/arduino yes NO NO NO yes
examples/raspberry_pi/pico2 yes NO NO NO NO
The missing silu is not a lost optimisation. Left to decompose, silu becomes
sigmoid plus an elementwise mul, and because that happens in to_edge, after
convert_pt2e, the resulting mul carries no input_qparams. AtenToCortexMPass then
raises KeyError: 1 in _get_mul_replacement, so every model containing SiLU failed
to compile through the compiler, through the documented recipe, and through both
examples. The same model lowers to a single cortex_m.quantized_activation through
the test harness, which is why this was invisible: the tests were exercising a
different configuration from the one users are told to write.
All five now come from cortex_m_edge_compile_config(), so they cannot drift again,
and the documented recipe is a call rather than a list to copy incorrectly. The
harness config was already the superset, so its behaviour is unchanged and the
suite is unaffected: 606 passed. The two examples only ever exercised linear, so
they gain the other entries without changing what they currently do.
Nothing in CI drives the compiler's Cortex-M path, which is how the divergence
survived. Sharing the constant removes this class of drift by construction, but
that coverage gap is worth closing separately.
Authored with assistance from Claude Code.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21826
Note: Links to docs will display an error until the docs builds have been completed. ⏳ 42 Pending, 1 Unrelated FailureAs of commit 61a5f59 with merge base 33f68d1 ( BROKEN TRUNK - The following job 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
Bot
added
the
CLA Signed
This PR needs a
|
psiddh
reviewed
Aug 14, 2026
| """ | ||
| return EdgeCompileConfig( | ||
| preserve_ops=list(_PRESERVE_OPS), | ||
| _check_ir_validity=False, |
Contributor
There was a problem hiding this comment.
Is _check_ir_validity=False necessary for the op set ?Asking because preserve_ops and _core_aten_ops_exception_list look like they'd already cover the non-core ops , alsi if validity is off, does the exception list still do anything?
Contributor
Author
There was a problem hiding this comment.
Yes, it is necessary. I removed the exception list.
psiddh
reviewed
Aug 14, 2026
| ) | ||
| # Use the backend's own configuration rather than hand-writing one. Ops such as | ||
| # silu and hardswish must survive to_edge for the Cortex-M passes to lower them; | ||
| # omitting one does not degrade gracefully, it fails the AtenToCortexMPass. |
Contributor
There was a problem hiding this comment.
So is it true that omitting an op fails the AtenToCortexMPass? wouldn't you just get a slower graph rather than an error?
Contributor
Author
There was a problem hiding this comment.
Depends on the op. The activations do result in a lowering failure, KeyErrors on quant params.
psiddh
reviewed
Aug 14, 2026
| """The to_edge configuration the Cortex-M backend requires. | ||
|
|
||
| Shared by the AOT compiler and the test harness so the two cannot drift: an | ||
| entry present in only one of them means the tests exercise a lowering users |
Contributor
There was a problem hiding this comment.
Are there other hand-written EdgeCompileConfigs left that should use this factory?
Contributor
Author
There was a problem hiding this comment.
Yeah, I missed one in the cortex-m skill. Added.
psiddh
approved these changes
Aug 14, 2026
…l recipe Review follow-up on the shared to_edge config. _core_aten_ops_exception_list is removed rather than shared. It feeds the edge-dialect verifier, and _check_ir_validity=False disables that verifier, so on this path it has never done anything. It arrived that way: it was added in a00bad2 alongside s8 max_pool2d support, onto a config that already had validation switched off. Enabling validation is not a flag flip either, it fails 50 of the backend's tests with mismatched-dtype SpecViolationErrors, so the reason validation stays off is now recorded in the factory where the next reader will look, along with the note that max_pool2d would need an entry if that ever changes. The claim that omitting a preserve_ops entry fails the lowering was too broad. It depends on the op, because the decompositions happen in to_edge, after convert_pt2e, so the nodes they create were never annotated and carry no qparams. hardsigmoid and hardswish expand into clamp and div, and ActivationFusionPass raises KeyError: 0 reading output_qparams off the clamp. silu expands into sigmoid and mul, and AtenToCortexMPass raises KeyError: 1 reading the second operand's input_qparams. linear is the quiet case: it expands into addmm, no pass tries to claim it, and the graph silently stays on portable float kernels. The docs now say both outcomes. The Cortex-M skill doc was a sixth copy of the recipe, and the worst one: it passed no preserve_ops at all, so anyone following it got the float fallback for every op in the list. It now calls the factory. Two Cortex-M test files keep their own plain config on purpose. They exercise the portable int8 ops and the quant-node replacement pass in isolation, where preserving the backend's op set would change what they test. Authored with assistance from Claude Code.
pytorch#21819 landed the same silu preserve_ops entry this branch was adding, in the two copies it touched: the AOT compiler and the docs recipe. Both conflicts resolve to the shared factory, whose op set is byte-identical to the one pytorch#21819 settled on, so the merge keeps their fix and removes the remaining copies of it. The docs snippet also loses the _core_aten_ops_exception_list that main still carries, which is the deliberate change in the previous commit: it feeds a verifier that _check_ir_validity=False disables. Still outstanding after the merge, and the reason this branch is worth landing on top of pytorch#21819: the cortex-m skill doc and the two examples remain copies, and the skill doc passes no preserve_ops at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The to_edge configuration the Cortex-M backend needs was written out by hand in five places, and four of them were wrong in different ways:
The missing silu is not a lost optimisation. Left to decompose, silu becomes sigmoid plus an elementwise mul, and because that happens in to_edge, after convert_pt2e, the resulting mul carries no input_qparams. AtenToCortexMPass then raises KeyError: 1 in _get_mul_replacement, so every model containing SiLU failed to compile through the compiler, through the documented recipe, and through both examples. The same model lowers to a single cortex_m.quantized_activation through the test harness, which is why this was invisible: the tests were exercising a different configuration from the one users are told to write.
All five now come from cortex_m_edge_compile_config(), so they cannot drift again, and the documented recipe is a call rather than a list to copy incorrectly. The harness config was already the superset, so its behaviour is unchanged and the suite is unaffected: 606 passed. The two examples only ever exercised linear, so they gain the other entries without changing what they currently do.
Nothing in CI drives the compiler's Cortex-M path, which is how the divergence survived. Sharing the constant removes this class of drift by construction, but that coverage gap is worth closing separately.
Authored with assistance from Claude Code.
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218 @mansnils @Sebastian-Larsson @robell