Skip to content

[ExecuTorch][WebGPU] Generate typed to_copy shader variants - #21449

Open
JCNTH wants to merge 2 commits into
gh/JCNTH/199/basefrom
gh/JCNTH/199/head
Open

[ExecuTorch][WebGPU] Generate typed to_copy shader variants#21449
JCNTH wants to merge 2 commits into
gh/JCNTH/199/basefrom
gh/JCNTH/199/head

Conversation

@JCNTH

@JCNTH JCNTH commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Generate typed to_copy variants from one byte-identical template

The two conversion directions duplicated the same WGSL structure and could drift independently. This moves fp32-to-int32 and int32-to-fp32 into one typed family while preserving both expanded payloads. Mirrors Vulkan backends/vulkan/runtime/graph/ops/glsl/view_convert_buffer.{glsl,yaml}.

Key changes:

  • to_copy_convert.wgsl and YAML — declare the two typed variants.
  • Generated headers and codegen locks — preserve symbols, workgroups, registry entries, and payload hashes.
  • Structural/native tests — lock both directions and the round trip.

Host routing and dispatch remain unchanged; tests expand fixture coverage.

Co-authored-with: Claude Code.
@exported-using-ghexport

Differential Revision: D113979712

Differential Revision: D113979712

[ghstack-poisoned]
@pytorch-bot

pytorch-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit f9f2cdc with merge base 8200b9e (image):

NEW FAILURE - The following job has failed:

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

@github-actions

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.

@JCNTH

JCNTH commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@claude Double check for need of changes, modulariziation opportunity, etc

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 1m 52s —— View job


Review: WebGPU typed to_copy shader variants

  • Read the full diff
  • Review WGSL template + YAML
  • Review generated headers / codegen locks
  • Review tests
  • Assess modularization opportunities

Verdict: solid, ship-ready. The refactor collapses two byte-identical shaders into one typed template and mirrors the established binary_op/boolean_op multi-variant convention almost exactly. The generated headers keep their distinct symbols, workgroup constants, and payload hashes unchanged, so host routing in ToCopy.cpp needs no edits. Nothing blocking; findings below are minor / optional.

Correctness — looks right

  • The template collapse hinges on output[idx] = ${OUT_TYPE}(input[idx]). Both directions are pure scalar casts (i32(f32), f32(i32)), so a single expression covers both. ✅
  • The wgsl-sha256 in each header is unchanged from the pre-refactor files — the expansion is byte-identical, which is exactly what the new test_to_copy_convert_template_roundtrip_byte_identical locks. Good regression guard.
  • The float fixture (-8.75, -3.0, -1.5, -0.25, ...) deliberately includes fractional magnitudes, so the round-trip exercises truncation-toward-zero through int32 rather than trivially-invertible integers. Nice — this is the kind of value that would catch a wrong cast op. ✅

Minor: YAML license-header inconsistency

to_copy_convert.yaml:1-6 adds a BSD license header, but every sibling template YAML (binary_op.yaml, boolean_op.yaml, rms_norm.yaml, softmax.yaml) has none — they start directly with the template name. Adding headers is arguably better, but as-is it's the odd one out. Either drop it to match convention or (better) treat it as the start of a convention — just be consistent within the PR's scope.

Observation: float→int is only numerically exercised via the round-trip

In cases.py, _to_copy_factory maps only int_to_float and float_roundtrip (the suite's golden_dtype="float32" can't hold a pure int output). So the standalone float→int shader is only validated numerically through the round-trip module, not in isolation. Delegation of the float→int direction is covered directly in test_to_copy.py::test_float_to_int_delegates, so this is coverage-shaping, not a gap — just worth being aware that a float→int-only numeric regression would surface via the round-trip case rather than a dedicated one.

Modularization: _delegated_cast_dtypes is the one intricate spot

test_to_copy.py:88-102 does two-phase logic — return [] if an undelegated _to_dim_order_copy sits at the top level, otherwise harvest dtype kwargs from inside lowered submodules — using "_to_dim_order_copy" in str(getattr(node, "target", "")) string matching in both places. It works and is pragmatic, but:

  • The substring match on str(target) is a bit fragile if the op's qualified name ever shifts. A structural check (comparing against exir_ops.edge.dim_order_ops._to_dim_order_copy.default, or the torch.ops.aten.to.dtype target already used in _prepartition_cast_dtypes) would be sturdier.
  • The two branches repeat the same node-scan predicate; a small _has_dim_order_copy(nodes) helper would remove the duplication and make the "delegated vs not" intent explicit.

Both are readability nits, not correctness issues — reasonable to leave given the surrounding test style.

Nice touches

  • Splitting _export into _lower (returns ep, edge) + _export (calls to_executorch) is clean and lets test_roundtrip_keeps_both_casts_in_delegate assert on the pre-partition graph, delegated graph, and executorch program independently.
  • Workgroup-boundary sizes (63, 64, 65, 257) around wg_size=64 are the right stress points.

I did not run the test suite (write ops are disabled on this ghstack branch and pytest needs approval here), but the roundtrip/registry lock tests are self-checking against the committed hashes, so CI on the PR will confirm.
· branch gh/JCNTH/199/head

[ghstack-poisoned]
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. meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant