Fix IndexError in RemovePermutesAroundElementwiseOps traversal - #21625
Open
cceroici wants to merge 1 commit into
Open
Fix IndexError in RemovePermutesAroundElementwiseOps traversal#21625cceroici wants to merge 1 commit into
cceroici wants to merge 1 commit into
Conversation
Summary: `_adapt_permute_across_view` indexed the permutation without checking it was expressed in the view's input-rank space, so `visit()` crashed with `IndexError: list index out of range` instead of rejecting the subgraph. - Traversal adapts the permutation when it crosses a rank-changing view. If it then reaches a *second* squeeze/unsqueeze view whose input is still at the original rank, `permute[index]` is out of range — e.g. squeezing a trailing size-1 dim gives `index == 3` for a rank-3 permutation. - Fix: validate the index / rank in all three branches of `_adapt_permute_across_view` and return `None`, the function's documented "not possible" signal. All three call sites already handle `None` by skipping the subgraph. - This mirrors the guard D105787161 added in `permute_subgraph()`, which only runs at apply time — traversal had no equivalent check. Surfaced by D113424191: its `_is_permutation_sink_view` branch lets traversal continue past a layout-invariant flatten rather than aborting, which grows regions enough to reach a rank-mismatched view. That optimization is intentional and is preserved here; only the crash is fixed. The latent bug predates it. Broke `test_auth_u85` (ECAPA -> Ethos-U85) — ARM's `RemovePermutesAroundElementwiseTosaOps` subclasses this shared pass, so it runs inside `ArmPassManager` on every Ethos-U lowering. That test is `CONTINUOUS_STATE`, so it is omitted on diffs and could not have blocked the landing. Authored with AI assistance (Claude Code). Differential Revision: D115062769
|
Contributor
|
@cceroici has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115062769. |
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
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:
_adapt_permute_across_viewindexed the permutation without checking it was expressed in the view's input-rank space, sovisit()crashed withIndexError: list index out of rangeinstead of rejecting the subgraph.permute[index]is out of range — e.g. squeezing a trailing size-1 dim givesindex == 3for a rank-3 permutation._adapt_permute_across_viewand returnNone, the function's documented "not possible" signal. All three call sites already handleNoneby skipping the subgraph.permute_subgraph(), which only runs at apply time — traversal had no equivalent check.Surfaced by D113424191: its
_is_permutation_sink_viewbranch lets traversal continue past a layout-invariant flatten rather than aborting, which grows regions enough to reach a rank-mismatched view. That optimization is intentional and is preserved here; only the crash is fixed. The latent bug predates it.Broke
test_auth_u85(ECAPA -> Ethos-U85) — ARM'sRemovePermutesAroundElementwiseTosaOpssubclasses this shared pass, so it runs insideArmPassManageron every Ethos-U lowering. That test isCONTINUOUS_STATE, so it is omitted on diffs and could not have blocked the landing.Authored with AI assistance (Claude Code).
Differential Revision: D115062769