[ET-VK] Fix squeeze_copy of the outermost dim under dynamic shapes - #22409
Open
msluszniak wants to merge 1 commit into
Open
[ET-VK] Fix squeeze_copy of the outermost dim under dynamic shapes#22409msluszniak wants to merge 1 commit into
msluszniak wants to merge 1 commit into
Conversation
add_squeeze_copy_dims_node() skips dim 0 and falls back to add_clone_node(). resize_clone_node() only propagates sizes when input and output have the same dim count, which a squeeze never does, so the output keeps the extents it was built with. With static shapes that is invisible. With dynamic shapes the output holds its upper-bound extents while consumers read it at the real size, so the copy lands in the wrong places and roughly half the output comes back zeroed -- silently, with no error. Route dim 0 through the permute path like every other squeeze dim; resize_permute_node() already has an explicit branch for the rank-reducing case. Repro: any model that ends up with torch.cat(list(x), -1) over a rank-4 tensor with a dynamic dim. The unbind lowers to slice_copy plus squeeze_copy.dims, and the second slice comes back zeroed for every extent below the bound. Reduced to a 15-line case: y[1:2] is correct while y[1:2].squeeze(0) returns exactly half zeros (cosine 0.704 = sqrt of 0.5 against the reference), correct only at the bound. Verified on a Galaxy S26 Ultra (Adreno 840): the reduced case goes from 0.704 to 1.000000 at extents 200, 500 and 1000, and a TTS model whose classifier-free-guidance batch is built this way goes from cosine 0.36 to 0.99993 against its CPU reference.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22409
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
add_squeeze_copy_dims_node()deliberately skips dim 0 and falls back toadd_clone_node(). Butresize_clone_node()only propagates sizes when input and output have the same dim count -- which a squeeze never does, and the code says so:So the output is never resized. With static shapes that is invisible. With dynamic shapes the output keeps its upper-bound extents while consumers read it at the real size, the copy lands in the wrong places, and roughly half the output comes back zeroed. There is no error; the values are just wrong.
op_registry.pydeclaressupports_resize=Trueforsqueeze_copy.dims, so the partitioner accepts the op and the result is a silent wrong answer rather than a rejected partition.Fix
Route dim 0 through the permute path like every other squeeze dim.
resize_permute_node()already has an explicit branch for the rank-reducing case, so no new resize logic is needed.Reproduction
Any model that ends up with
torch.cat(list(x), -1)over a rank-4 tensor with a dynamic dim: the unbind lowers toslice_copy+squeeze_copy.dims, and the second slice comes back zeroed for every extent below the bound. Reduced:At L=200,
y[1:2]is correct whiley[1:2].squeeze(0)returns exactly half zeros -- cosine 0.704 against the reference, which is sqrt(0.5). It is correct only at L=1000, the bound.Verification
Galaxy S26 Ultra, Adreno 840, fp16:
y[1:2].squeeze(0), L=200 / 500 / 1000cat(list(x), -1)The second row is a supertonic TTS vector-estimator whose classifier-free-guidance uncond branch was the zeroed slice; it is now within fp16 noise of its CPU reference at every sequence length tested (64, 200, 500, 1000).
Note that this class of bug is invisible to
executor_runner, which can only run a model at its dynamic upper bound -- there is no flag to request a smaller shape, and--inputsfiles must matchnbytes()of the bound. I found it with a local patch adding an--input_shapesflag; happy to send that separately if it would be useful.