Do not move a view's shape earlier than the nodes that compute it - #22403
Open
msluszniak wants to merge 1 commit into
Open
Do not move a view's shape earlier than the nodes that compute it#22403msluszniak wants to merge 1 commit into
msluszniak wants to merge 1 commit into
Conversation
`_merge_view_copy_chains` gives the first view in a chain the shape of the
last one:
final_shape = view_nodes_to_remove[-1].args[1]
node.args = (node.args[0], final_shape)
Under dynamic shapes a view's shape is not a list of ints, it is a list that
can contain nodes computing symbolic sizes. Those nodes may be defined
anywhere between the first and the last view, so assigning the later shape to
the earlier node can make that node read a value that does not exist yet.
The result is a graph that fails fx's own `lint()`, which the pass then hits
itself on the very next line via `eliminate_dead_code()`:
RuntimeError: Argument 'mul' of Node 'aten_view_copy_default_21' was used
before it has been defined! Please check that Nodes in the graph are
topologically ordered
seen while lowering a TTS model to Vulkan, where the chain ends on a
`view_copy(x, [2*s, 512])` whose `2*s` is produced one node after the first
view.
Fuse the longest prefix of the chain whose shape is available where the first
view sits, and leave the rest alone. Chains with static shapes, which is the
common case, are unaffected.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22403
Note: Links to docs will display an error until the docs builds have been completed.
|
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
msluszniak
added a commit
to software-mansion-labs/executorch
that referenced
this pull request
Sep 1, 2026
Backport of pytorch/executorch#22403 and #22406. - FuseViewCopyTransform no longer moves a view's shape earlier than the nodes that compute it, which produced a topologically invalid graph under dynamic shapes. - index.Tensor with the index on any dimension is dispatched to index_select instead of falling back to the CPU. TagMemoryMetaPass now assigns a representation to tensors that sit in a list alongside Nones. Supertonic vector_estimator goes from 29 delegate calls to 2, ~11% faster end to end on Adreno 840, outputs unchanged.
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
_merge_view_copy_chainsgives the first view in a chain the shape of the last one:Under dynamic shapes a view's shape is not a list of ints, it is a list that can contain nodes computing symbolic sizes. Those nodes may be defined anywhere between the first and the last view, so assigning the later shape to the earlier node can make that node read a value that does not exist yet.
The result is a graph that fails fx's own
lint(), which the pass then hits itself on the very next line viaeliminate_dead_code():I hit this lowering a TTS model to Vulkan. The chain ends on a
view_copy(x, [2*s, 512])whose2*sis produced exactly one node after the first view of the chain, so the rewrite moves the shape one position too far back. The submodule handed to the backend is correctly ordered; this pass is what breaks it.Fix
Fuse the longest prefix of the chain whose shape is available where the first view sits, and leave the rest alone. Chains with static shapes, which is the common case, keep fusing exactly as before.
Test plan
New
backends/transforms/test/test_fuse_view_copy.py:graph.lint())The second test fails on the current pass and passes with this change; the first passes either way, so the existing fusion is not narrowed for static shapes.