Fix unsqueeze permutation adaptation in RemovePermutesAroundElementwiseOps (#21551) - #21551
Fix unsqueeze permutation adaptation in RemovePermutesAroundElementwiseOps (#21551)#21551mcremon-meta wants to merge 2 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21551
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 Cancelled Job, 239 PendingAs of commit 7ff3c1e with merge base f67adf5 ( CANCELLED JOB - The following job was cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@mcremon-meta has exported this pull request. If you are a Meta employee, you can view the originating Diff in D114508242. |
This PR needs a
|
| # The scalar constants are numel-1 (1,1,1,1): layout-invariant, so they are | ||
| # left wired directly with no compensating permute, and every permute in the | ||
| # region cancels. | ||
| assert _count_nodes(result.graph_module, PERMUTE_TARGET) == 0 |
There was a problem hiding this comment.
perhaps a naive question, the permute is done on the input x, and not on the scalar constants, how was the pass able to remove it?
There was a problem hiding this comment.
we used to add compensating permutes to the [1, 1, 1, 1] constants, which is now layout invariant and treated as a nop. So those 3 are not added anymore, bringing the total count to 0
…seOps (#21551) Summary: `_adapt_permute_across_view` and `permute_subgraph` disagreed about where an unsqueeze's new dim lands in the un-permuted layout. When the region walker crosses an unsqueeze, it has to (a) adapt the running permutation to the new rank and (b) later rewrite the unsqueeze's `dim` arg into un-permuted space. `permute_subgraph` does (b) as `permute[index]`, but `_adapt_permute_across_view` did (a) by inserting `index` -- both as the shift threshold and as the inserted value. The two only coincide when `permute[index] == index`. For example with `P = [0, 2, 1]` and an unsqueeze at index 1, the rewrite emits `dim=2` while the adaptation produces `[0, 1, 3, 2]`; the permutation consistent with `dim=2` is `[0, 2, 3, 1]`. Downstream nodes then get remapped against a wrong permutation and the end-permute comparison no longer matches. In practice that shows up as a missed optimisation -- the subgraph is rejected -- but nothing guarantees the wrong permutation cannot coincidentally match an end permute, so this is a correctness fix, not just a missed-match fix. The squeeze branch immediately below already did this correctly (`squeezed_value = permute[index]`, with a comment spelling out that the position and the value are different things); only unsqueeze was inconsistent. Note this pass is shared: the Cadence (opt_level=2) and Arm/TOSA subclasses inherit the fixed behaviour. Their suites are covered below. Differential Revision: D114508242
7e3ff8b to
90a45ea
Compare
…seOps (#21551) Summary: `_adapt_permute_across_view` and `permute_subgraph` disagreed about where an unsqueeze's new dim lands in the un-permuted layout. When the region walker crosses an unsqueeze, it has to (a) adapt the running permutation to the new rank and (b) later rewrite the unsqueeze's `dim` arg into un-permuted space. `permute_subgraph` does (b) as `permute[index]`, but `_adapt_permute_across_view` did (a) by inserting `index` -- both as the shift threshold and as the inserted value. The two only coincide when `permute[index] == index`. For example with `P = [0, 2, 1]` and an unsqueeze at index 1, the rewrite emits `dim=2` while the adaptation produces `[0, 1, 3, 2]`; the permutation consistent with `dim=2` is `[0, 2, 3, 1]`. Downstream nodes then get remapped against a wrong permutation and the end-permute comparison no longer matches. In practice that shows up as a missed optimisation -- the subgraph is rejected -- but nothing guarantees the wrong permutation cannot coincidentally match an end permute, so this is a correctness fix, not just a missed-match fix. The squeeze branch immediately below already did this correctly (`squeezed_value = permute[index]`, with a comment spelling out that the position and the value are different things); only unsqueeze was inconsistent. Note this pass is shared: the Cadence (opt_level=2) and Arm/TOSA subclasses inherit the fixed behaviour. Their suites are covered below. Reviewed By: digantdesai Differential Revision: D114508242
90a45ea to
65908a6
Compare
…seOps (#21551) Summary: `_adapt_permute_across_view` and `permute_subgraph` disagreed about where an unsqueeze's new dim lands in the un-permuted layout. When the region walker crosses an unsqueeze, it has to (a) adapt the running permutation to the new rank and (b) later rewrite the unsqueeze's `dim` arg into un-permuted space. `permute_subgraph` does (b) as `permute[index]`, but `_adapt_permute_across_view` did (a) by inserting `index` -- both as the shift threshold and as the inserted value. The two only coincide when `permute[index] == index`. For example with `P = [0, 2, 1]` and an unsqueeze at index 1, the rewrite emits `dim=2` while the adaptation produces `[0, 1, 3, 2]`; the permutation consistent with `dim=2` is `[0, 2, 3, 1]`. Downstream nodes then get remapped against a wrong permutation and the end-permute comparison no longer matches. In practice that shows up as a missed optimisation -- the subgraph is rejected -- but nothing guarantees the wrong permutation cannot coincidentally match an end permute, so this is a correctness fix, not just a missed-match fix. The squeeze branch immediately below already did this correctly (`squeezed_value = permute[index]`, with a comment spelling out that the position and the value are different things); only unsqueeze was inconsistent. Note this pass is shared: the Cadence (opt_level=2) and Arm/TOSA subclasses inherit the fixed behaviour. Their suites are covered below. Reviewed By: digantdesai Differential Revision: D114508242
65908a6 to
958a9b7
Compare
Summary: `ConvToChannelsLast` wraps every conv in `permute(NCHW->NHWC) -> conv -> permute(NHWC->NCHW)`. When convs are joined by elementwise fused_quant ops (residual add/mul, activations), permutes end up threaded through the surrounding region and were never removed: the existing Cadence `RemovePermutesAroundElementwiseOps` only recognizes aten/cadence elementwise ops, not the SAS fused_quant ops -- and it would also wrongly treat their lifted scale/zero_point operands as constants to be permuted. This adds a fused_quant-aware permute-removal pass and wires it into the edge optimization group: - Extend the shared ExecuTorch `RemovePermutesAroundElementwiseOps` with a small overridable seam (`_permute_relevant_inputs`) so a subclass can hide operands from layout propagation. Behavior-preserving for existing users. - New `RemovePermutesAroundFusedQuantElementwiseOps` (SAS) subclasses it, adding `fused_quant.add`/`mul` and the activation ops as permutable and exposing only their tensor operands, so the lifted scale/zero_point placeholders are never permuted/compensated (which would break lowering). Ops with per-channel qparams are skipped (not permutation-invariant). - Replace the two Cadence permute passes (which no-op pre-Lower) in the optimization group with this single pass. - Teach the shared subgraph engine about "permutation-sink" flattens: a `view_copy` whose input has <=1 non-unit dim (e.g. the `[1, C, 1, 1] -> [1, C]` after a global pool) is layout-invariant, so a permutation flowing into it simply dies. The region can terminate cleanly there with no compensating permute -- which lets the residual-block permutes collapse across the avgpool -> flatten -> classifier head instead of being stranded by it. Note: fused_quant is currently SAS-specific, NOT yet a generic cross-backend dialect, so the fused_quant knowledge deliberately stays in the SAS subclass rather than the shared ExecuTorch pass. When fused_quant graduates to a shared dialect, this can fold into the base pass via `extra_permutable_ops` + the seam. (The permutation-sink flatten handling is generic and correctly lives in the shared pass.) On resnet18 the optimized graph goes from 83 permutes down to a single one (the model-input boundary); every permute around the residual add/relu blocks and across the global-pool flatten is removed. Reviewed By: DrJessop Differential Revision: D113424191
…seOps (#21551) Summary: `_adapt_permute_across_view` and `permute_subgraph` disagreed about where an unsqueeze's new dim lands in the un-permuted layout. When the region walker crosses an unsqueeze, it has to (a) adapt the running permutation to the new rank and (b) later rewrite the unsqueeze's `dim` arg into un-permuted space. `permute_subgraph` does (b) as `permute[index]`, but `_adapt_permute_across_view` did (a) by inserting `index` -- both as the shift threshold and as the inserted value. The two only coincide when `permute[index] == index`. For example with `P = [0, 2, 1]` and an unsqueeze at index 1, the rewrite emits `dim=2` while the adaptation produces `[0, 1, 3, 2]`; the permutation consistent with `dim=2` is `[0, 2, 3, 1]`. Downstream nodes then get remapped against a wrong permutation and the end-permute comparison no longer matches. In practice that shows up as a missed optimisation -- the subgraph is rejected -- but nothing guarantees the wrong permutation cannot coincidentally match an end permute, so this is a correctness fix, not just a missed-match fix. The squeeze branch immediately below already did this correctly (`squeezed_value = permute[index]`, with a comment spelling out that the position and the value are different things); only unsqueeze was inconsistent. Note this pass is shared: the Cadence (opt_level=2) and Arm/TOSA subclasses inherit the fixed behaviour. Their suites are covered below. Reviewed By: digantdesai Differential Revision: D114508242
958a9b7 to
7ff3c1e
Compare
Summary:
_adapt_permute_across_viewandpermute_subgraphdisagreed about where anunsqueeze's new dim lands in the un-permuted layout.
When the region walker crosses an unsqueeze, it has to (a) adapt the running
permutation to the new rank and (b) later rewrite the unsqueeze's
dimarginto un-permuted space.
permute_subgraphdoes (b) aspermute[index], but_adapt_permute_across_viewdid (a) by insertingindex-- both as the shiftthreshold and as the inserted value. The two only coincide when
permute[index] == index.For example with
P = [0, 2, 1]and an unsqueeze at index 1, the rewrite emitsdim=2while the adaptation produces[0, 1, 3, 2]; the permutationconsistent with
dim=2is[0, 2, 3, 1]. Downstream nodes then get remappedagainst a wrong permutation and the end-permute comparison no longer matches.
In practice that shows up as a missed optimisation -- the subgraph is rejected
-- but nothing guarantees the wrong permutation cannot coincidentally match an
end permute, so this is a correctness fix, not just a missed-match fix.
The squeeze branch immediately below already did this correctly
(
squeezed_value = permute[index], with a comment spelling out that theposition and the value are different things); only unsqueeze was inconsistent.
Note this pass is shared: the Cadence (opt_level=2) and Arm/TOSA subclasses
inherit the fixed behaviour. Their suites are covered below.
Reviewed By: digantdesai
Differential Revision: D114508242