Skip to content

[ET-VK] Do not partition constant_pad_nd with a symbolic pad - #22399

Open
msluszniak wants to merge 1 commit into
pytorch:mainfrom
msluszniak:ms/vulkan-partitioner-guards
Open

[ET-VK] Do not partition constant_pad_nd with a symbolic pad#22399
msluszniak wants to merge 1 commit into
pytorch:mainfrom
msluszniak:ms/vulkan-partitioner-guards

Conversation

@msluszniak

@msluszniak msluszniak commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

A pad amount derived from a dynamic dimension is serialized as a VALUELIST of Int/SymInt, and Pad.cpp reads the list with get_int_list(), which requires a literal IntList. The partitioner claims the node regardless, so the model lowers and then aborts at prepack:

Exception raised from toIntList at .../graph/containers/Value.h:272:
(isIntList()) is false! Expected value to have type IntList, got VALUELIST instead

This is reachable from ordinary code. Any model that pads a dynamic sequence up to the static length its LSTM wants hits it, which is how kokoro's synthesizer fails:

x = torch.nn.functional.pad(x, (0, 0, 0, target - seq_len))

Why not just read the list symbolically

Split.cpp, View.cpp and Expand.cpp all handle a symbolic list with extract_int_or_symint_list(), and Pad.cpp was simply missed. Swapping the extractor in is not sufficient on its own, though: add_constant_pad_nd_node() folds the amounts into a per-dim offset and bakes that into a params buffer at build time, so the dispatch would use stale offsets even if the list were read symbolically. Refreshing that buffer on resize is the real fix and is left for a follow-up. Until then this declines the node so it falls back instead of aborting.

Test plan

test_vulkan_backend_constant_pad_nd pads by a literal (1, 2, 3, 4, 5, 6), so no test covered a symbolic pad. test_vulkan_backend_constant_pad_nd_symbolic_pad aborts on main and falls back cleanly with this change.

Note on scope

This PR previously also guarded clamp/hardtanh and _native_batch_norm_legit_no_training.

  • The clamp guards are dropped: [ET-VK][ops] Extend arange, clamp, and index.Tensor support #22481 implements a real dynamic-bounds path for clamp and hardtanh via add_dynamic_clamp_node, which is strictly better than declining, and keeping the guard alongside it would decline nodes that now work. hardshrink and leaky_relu still read their bounds at build time through get_val_or_inf(), but a symbolic lambda or negative_slope is a hyperparameter no real model produces, so that is left alone rather than guarded.
  • The batch norm guard moved to its own PR, since it is an unrelated failure.

Merges cleanly with #22481.

@msluszniak
msluszniak requested a review from SS-JIA as a code owner September 1, 2026 08:25
@pytorch-bot pytorch-bot Bot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Sep 1, 2026
@pytorch-bot

pytorch-bot Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22399

Note: Links to docs will display an error until the docs builds have been completed.

⚠️ 16 Awaiting Approval

As of commit 77c1a8c with merge base 135a109 (image):

AWAITING APPROVAL - The following workflows need approval before CI can run:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

msluszniak added a commit to software-mansion-labs/executorch that referenced this pull request Sep 1, 2026
Backport of pytorch/executorch#22399.

Two op registrations advertise support the runtime does not have, so a graph
containing either lowers cleanly and then aborts at execute time.

constant_pad_nd: a symbolic pad list is serialized as a VALUELIST, and Pad.cpp
reads it with get_int_list(), raising "Expected value to have type IntList, got
VALUELIST instead". add_constant_pad_nd_node() also bakes the pad amounts into
a params buffer at build time, so a pad derived from a dynamic dim would be
stale even if the list were read symbolically; decline both cases rather than
trade an abort for a wrong result.

_native_batch_norm_legit_no_training: add_native_batch_norm_node() asserts
in_sizes.size() == 4, so any conv1d model (rank-3 activations) aborts with
"BatchNorm only support 4d tensor".

Both now fall back instead of aborting. Found with the Supertonic TTS model,
whose text encoder hits the first (VITS relative-attention pads are derived
from the sequence length) and whose vocoder hits the second.
@msluszniak msluszniak changed the title Vulkan: do not partition ops the runtime will reject Vulkan: do not partition ops whose symbolic arguments the runtime reads at build time Sep 1, 2026
msluszniak added a commit to software-mansion-labs/executorch that referenced this pull request Sep 1, 2026
Backport of pytorch/executorch#22399.

get_val_or_inf() in UnaryOp.cpp reads each clamp bound with
extract_scalar<float>() when the node is BUILT and bakes the result into the
dispatch, so a bound derived from a dynamic dim is never refreshed and the op
silently computes against a stale limit. Worst when clamp is applied to index
tensors, where a wrong limit reorders or drops data downstream.

Supertonic on S26 Ultra (Adreno 840), vs a CPU reference that XNNPACK
reproduces at cosine 1.000000:

  vocoder          0.016757 -> 0.999977
  vector_estimator 0.994432 -> 0.999994

hardtanh, hardshrink and leaky_relu share the same build-time path.
A pad amount derived from a dynamic dimension is serialized as a VALUELIST of
Int/SymInt, and Pad.cpp reads the list with get_int_list(), which requires a
literal IntList. The partitioner claims the node regardless, so the model
lowers and then aborts at prepack:

  Exception raised from toIntList at .../graph/containers/Value.h:272:
  (isIntList()) is false! Expected value to have type IntList, got VALUELIST

This is reachable from ordinary code. Any model that pads a dynamic sequence up
to the static length its LSTM wants hits it, which is how kokoro's synthesizer
fails: `F.pad(x, (0, 0, 0, target - seq_len))`.

Supporting it properly is more than swapping in extract_int_or_symint_list(),
the way Split.cpp, View.cpp and Expand.cpp read their symbolic lists.
add_constant_pad_nd_node() folds the amounts into a per-dim offset and bakes
that into a params buffer at build time, so the dispatch would use stale
offsets even if the list were read symbolically; the buffer has to be refreshed
on resize first. Decline the node until then, so it falls back rather than
aborting.

The existing test_vulkan_backend_constant_pad_nd pads by a literal
(1, 2, 3, 4, 5, 6), so no test covered a symbolic pad. The new case aborts on
main and falls back cleanly with this change.
@msluszniak
msluszniak force-pushed the ms/vulkan-partitioner-guards branch from 6da6af3 to 77c1a8c Compare September 3, 2026 07:19
@msluszniak msluszniak changed the title Vulkan: do not partition ops whose symbolic arguments the runtime reads at build time Vulkan: do not partition constant_pad_nd with a symbolic pad Sep 3, 2026
@msluszniak msluszniak changed the title Vulkan: do not partition constant_pad_nd with a symbolic pad [ET-VK] Do not partition constant_pad_nd with a symbolic pad Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants