select: accept a constant SymInt dim, reject driven ones - #22568
Conversation
Summary: `read_scalar` in the WebGPU `select` op treated every `ValueType::SymInt` as dynamic and threw `select: dynamic/unsupported dim`. That is stricter than it needs to be: the Vulkan serializer sometimes parks a plain integer constant in a SymInt slot, much as it can reuse an earlier `Double` value for an equal integer scalar. Such a graph is fully static, but failed to build. A SymInt is now rejected only when some op actually drives it at execute time. The check is deny-by-default -- the value counts as a build-time constant only if no producer registry claims it -- and consults all three SymInt producers: `et_vk.select_as_symint` (`symint_sources`), `sym_size.int` (`symint_dim_sources`) and SymInt arithmetic (`symint_computed`). Rejection has to stay loud for those, because `select` drops an axis: a dim that varies at runtime changes the output rank, which the graph cannot express, so silently accepting it would return a wrong-shaped result instead of an error. The arithmetic case needed a new registry. `register_sym_binary` keys its resize hook on the trigger operand rather than on the result, so neither existing registry can answer "does some op write this id?" for a value produced by `add`/`sub`/`mul`/`floordiv`. `WebGPUGraph::add_symint_computed` records the result instead, and `register_sym_binary` calls it whenever it registers a hook. That call is gated on operand liveness so `add(Int, Int) -> SymInt`, which registers no hook and genuinely cannot change, is not falsely rejected. Any future SymInt producer must be consulted in `is_runtime_driven_symint`; the comment there says so. Missing one puts `select` back to reading a build-time seed for a value that varies at execute. Authored with Claude Code. Differential Revision: D118248620
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22568
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New Failures, 1 Unrelated FailureAs of commit ec13706 with merge base fcc3eb5 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
@psiddh has exported this pull request. If you are a Meta employee, you can view the originating Diff in D118248620. |
This PR needs a
|
There was a problem hiding this comment.
🟢 Approval recommended
The core behavior change is narrowly scoped, includes targeted regression coverage, and the remaining feedback is limited to minor comment/wording fixes.
Pull request overview
This PR relaxes WebGPU select scalar parsing to accept SymInt-backed constants while continuing to fail loudly for SymInts that are actually driven at execution time (to avoid silently producing wrong-rank outputs when dim is dynamic). It adds a new registry for SymInt arithmetic results so select can reliably detect resize-hook-driven SymInts as runtime-driven.
Changes: (1) select now treats a ValueType::SymInt as acceptable only if it is not claimed by any known SymInt producer registry (deny-by-default); (2) WebGPUGraph adds symint_computed_ tracking plus an accessor for symint_dim_sources_ to support runtime-driven detection; (3) tests extend the serialized-graph coverage to include constant SymInt dims (accepted) and runtime-driven SymInt dims via sym_size.int and arithmetic (rejected).
File summaries
| File | Description |
|---|---|
| backends/webgpu/test/test_webgpu_native.cpp | Extends regression tests to cover constant SymInt dims (builds + matches Int behavior) and dynamic SymInt producers (still rejected). |
| backends/webgpu/runtime/WebGPUGraph.h | Adds symint_dim_sources() accessor and introduces symint_computed_ to track SymInts recomputed by resize hooks (arithmetic). |
| backends/webgpu/runtime/ops/select/Select.cpp | Implements deny-by-default runtime-driven SymInt detection and allows SymInt constants to be read as scalars. |
| backends/webgpu/runtime/ops/select_as_symint/SelectAsSymint.cpp | Marks arithmetic-produced SymInts as runtime-driven when recompute hooks are registered, enabling select to reject them. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // dim/index are static integer scalars; Vulkan serialization can reuse an | ||
| // earlier Double value for an equal integer scalar, or park one in a SymInt | ||
| // slot. Only a SymInt an op actually drives at runtime throws: select drops an | ||
| // axis, so a varying dim/index would change the output rank. |
| // How the select `dim` argument is serialized. The two dynamic kinds prepend | ||
| // the ops that make the SymInt runtime-driven: SymIntFromDim a sym_size.int, | ||
| // SymIntFromArithmetic a sym_size.int plus an `add` whose result feeds select. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The constant-vs-runtime distinction is correct, and tracking arithmetic results closes the existing producer gap. The remaining risk is architectural: select now has to know every SymInt producer, so adding a fourth producer without updating this op silently turns a dynamic value into a build-time constant and can produce a wrong-rank result. I would centralise is_runtime_driven_symint(id) on WebGPUGraph (or maintain one producer-owned driven-ID registry) so producer registration, not each consumer op, owns that invariant.
SS-JIA
left a comment
SS-JIA
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
Summary:
read_scalarin the WebGPUselectop treated everyValueType::SymIntasdynamic and threw
select: dynamic/unsupported dim. That is stricter than itneeds to be: the Vulkan serializer sometimes parks a plain integer constant in
a SymInt slot, much as it can reuse an earlier
Doublevalue for an equalinteger scalar. Such a graph is fully static, but failed to build.
A SymInt is now rejected only when some op actually drives it at execute time.
The check is deny-by-default -- the value counts as a build-time constant only
if no producer registry claims it -- and consults all three SymInt producers:
et_vk.select_as_symint(symint_sources),sym_size.int(
symint_dim_sources) and SymInt arithmetic (symint_computed). Rejection hasto stay loud for those, because
selectdrops an axis: a dim that varies atruntime changes the output rank, which the graph cannot express, so silently
accepting it would return a wrong-shaped result instead of an error.
The arithmetic case needed a new registry.
register_sym_binarykeys itsresize hook on the trigger operand rather than on the result, so neither
existing registry can answer "does some op write this id?" for a value produced
by
add/sub/mul/floordiv.WebGPUGraph::add_symint_computedrecords theresult instead, and
register_sym_binarycalls it whenever it registers ahook. That call is gated on operand liveness so
add(Int, Int) -> SymInt,which registers no hook and genuinely cannot change, is not falsely rejected.
Any future SymInt producer must be consulted in
is_runtime_driven_symint;the comment there says so. Missing one puts
selectback to reading abuild-time seed for a value that varies at execute.
Authored with Claude Code.
Differential Revision: D118248620