Vulkan: support index.Tensor with the index on any dimension - #22406
Open
msluszniak wants to merge 1 commit into
Open
Vulkan: support index.Tensor with the index on any dimension#22406msluszniak wants to merge 1 commit into
msluszniak wants to merge 1 commit into
Conversation
`x[:, :, idx]` reaches the edge dialect as `index.Tensor(x, [None, None, idx])`. The partitioner only accepted the index on dim 0, so every other gather fell back to the CPU. That is not just slower than it needs to be: an unsupported node in the middle of a graph splits the delegate in two, and a model that gathers once per block ends up with one delegate call per block. A TTS vector estimator here lowered to 29 delegate calls, 28 of them created by this single op. Gathering along a dimension is exactly what `index_select` does, and the Vulkan implementation of it already handles every dimension, so dispatch there when the index does not sit on dim 0 and leave the index_tensor shader, which gathers along the leading dim, alone. Two things were in the way of simply accepting the node: - The dimension is recoverable at runtime, because the indices list keeps a null for every dimension that is not indexed, but the list pointer has to be released before `add_scalar()` may reallocate the graph's value store. - Nothing assigned a representation to the index tensor. It lives inside a list whose other entries are None, and both `is_tensor_arg_node()` and `is_non_constant_tensor_node()` only recognise lists that are entirely tensors, so the whole argument was treated as a non-tensor and skipped. `index_select` reads channels-packed textures and asserts on anything else. Add `tensor_nodes_in_arg()`, which returns the tensor nodes of an argument and skips the Nones, and use it everywhere the tagging pass walks a list argument. Lists that are entirely tensors, which is every other operator today, behave exactly as before. With this the same model lowers to 2 delegate calls, and its four sub-models keep their outputs: cosine 1.000000, 0.999414, 0.999994 and 0.999977 against their CPU references on a Galaxy S26 Ultra.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22406
Note: Links to docs will display an error until the docs builds have been completed.
|
This PR needs a
|
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
x[:, :, idx]reaches the edge dialect asindex.Tensor(x, [None, None, idx]). The partitioner only accepted the index on dim 0, so every other gather fell back to the CPU.That is not just slower than it needs to be. An unsupported node in the middle of a graph splits the delegate in two, and a model that gathers once per block ends up with one delegate call per block. A TTS vector estimator I was lowering came out as 29 delegate calls, 28 of them created by this single op. On a Galaxy S26 Ultra each of those boundaries costs about 3 ms in staging and submission, measured with a synthetic 4-block A/B.
Gathering along a dimension is exactly what
index_selectdoes, and the Vulkan implementation of it already handles every dimension. So dispatch there when the index does not sit on dim 0, and leave theindex_tensorshader, which gathers along the leading dim, alone.Two things were in the way of simply accepting the node
The dimension. It is recoverable at runtime: the serialized indices list keeps a null for every dimension that is not indexed, so the position of the tensor entry is the dimension. The list pointer has to be released before
add_scalar(), which may reallocate the graph's value store (check_no_active_value_ptrscatches this).The index tensor never got a representation. It lives inside a list whose other entries are None, and both
is_tensor_arg_node()andis_non_constant_tensor_node()only recognise lists that are entirely tensors, so the whole argument was treated as a non-tensor and skipped byTagMemoryMetaPass.index_selectreads channels-packed textures and asserts on anything else.This adds
tensor_nodes_in_arg(), which returns the tensor nodes of an argument and skips the Nones, and uses it everywhere the tagging pass walks a list argument. Lists that are entirely tensors, which is every other operator today, behave exactly as before.Test plan
New
test_vulkan_backend_index_tensor_non_leading_diminbackends/vulkan/test/test_vulkan_delegate.py, alongside the existing dim-0 test.Verified on a Galaxy S26 Ultra (Adreno 840). A minimal
x[:, :, idx] * 2model aborts oncheck_index_select_argsbefore this change and matches its CPU reference after (max abs diff 6e-4, fp16). The TTS model that motivated it goes from 29 delegate calls to 2 with unchanged outputs:End to end that is 11% faster on device, measured with the arms alternated forward and reverse inside each round to cancel thermal drift.
Depends on nothing, but note that lowering this model also needs #22403; without it
FuseViewCopyTransformtrips over the larger partition.cc @SS-JIA @manuelcandales @digantdesai @cbilgin