Skip to content

[ExecuTorch][WebGPU] Test coverage for the f16-multiply steel q4gsw GEMM#20753

Open
JCNTH wants to merge 4 commits into
gh/JCNTH/7/basefrom
gh/JCNTH/7/head
Open

[ExecuTorch][WebGPU] Test coverage for the f16-multiply steel q4gsw GEMM#20753
JCNTH wants to merge 4 commits into
gh/JCNTH/7/basefrom
gh/JCNTH/7/head

Conversation

@JCNTH

@JCNTH JCNTH commented Jul 6, 2026

Copy link
Copy Markdown

Stack from ghstack (oldest at bottom):

Adds numeric coverage for the f16-multiply steel q4gsw kernel from the parent diff, which runs only in a -DWGPU_BACKEND_STEEL_F16 build and had none. Its f16 multiply has a rounding floor (~2.3e-4) above the strict f32 golden (1e-4), so the new configs use a looser abs tolerance without touching the f32 golden.

Key changes:

  • test_quantized_linear.py CONFIGS — add steel_f16 (same shape as the steel config, exact-N tile) and steel_f16_edge (M=70, K=1024, N=136, partial M and N tiles). The exported .pte is dtype-independent, so each fixture drives whichever steel kernel the runtime build selected.
  • test_webgpu_native.cpp kQ4gswConfigs — matching steel_f16/steel_f16_edge entries at abs 2.3e-4 / rel 1e-3, both under #ifdef WGPU_BACKEND_STEEL_F16. The native sweep self-discovers each .pte by name and goldens against the same fp64 truth the f32 configs use.

The looser abs gate reflects the f16 rounding floor (uniform in K, not an accumulate artifact); the accumulator stays f32. The default f32 build's config table and its strict 1e-4 golden are #ifdef-excluded and byte-unchanged.

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

Differential Revision: D110802559

Differential Revision: D110802559

[ghstack-poisoned]
@pytorch-bot

pytorch-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure, 4 Pending

As of commit d68e5b8 with merge base f8c8334 (image):

NEW FAILURE - The following job has failed:

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

@github-actions

github-actions Bot commented Jul 6, 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.

@JCNTH

JCNTH commented Jul 6, 2026

Copy link
Copy Markdown
Author

@claude throoughly review, check for overengineering, correctness, and areaas for modularization

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown

Claude finished @JCNTH's task in 3m 26s —— View job


Review: f16-multiply steel q4gsw test coverage

  • Read the PR diff against origin/gh/JCNTH/7/base
  • Trace how kQ4gswConfigs / CONFIGS are consumed (sweep, tol check, kernel routing)
  • Assess correctness, overengineering, and modularization

The mechanics are clean and consistent with the existing sweep: the native table self-discovers q4gsw_<name>.pte, the Python CONFIGS drives export, and the fp64 golden is dtype-independent so a shared fixture legitimately feeds whichever kernel the build selected. The #ifdef WGPU_BACKEND_STEEL_F16 guarding keeps the default f32 table byte-unchanged. Shapes check out (steel_f16_edge: N=136 → N%8==0, K=1024 → K%32==0, M=70/N=136 partial tiles). Nice tight change.

One substantive concern and a couple of smaller ones.

1. Correctness — does the pre-existing steel config still pass in an f16 build? (main question)

The routing in QuantizedLinear.cpp:270-278 swaps the steel kernel for the f16-multiply variant for any use_steel path when shader_f16_supported:

if (use_steel) {
  ...
  if (ctx != nullptr && ctx->shader_f16_supported) {
    shader_src = kQ4gswLinearGemmSteelHalfWGSL;
  }
}

The existing steel entry (test_webgpu_native.cpp:279, tol_abs=1e-4) is not #ifdef-guarded. So in a -DWGPU_BACKEND_STEEL_F16 build it runs the same f16 kernel on the same shape as the new steel_f16 entry — just with the strict 1e-4 abs gate you're saying the f16 floor (~2.3e-4) exceeds.

The tol gate is abs-AND-rel (quant_within_tol line 314: if (ae > atol && re > rtol) ok = false), so this only bites elements with both abs > 1e-4 and rel > 1e-3. That forces one of two conclusions:

  • If such elements exist (f16 error genuinely breaks 1e-4/1e-3), then steel itself fails in the f16 build — the parallel looser config doesn't save it, because steel still runs the same kernel. The PR would be incomplete.
  • If no such element exists (the rel gate keeps steel passing at 1e-4), then steel already validates the f16 kernel on this shape at a stricter bar, and steel_f16's looser abs is never actually exercised.

Either way, steel_f16 (identical shape/golden to steel) asserts strictly less than steel does on a bit-identical computation. Can you confirm which case holds — does steel pass under the f16 kernel? If it doesn't, the fix belongs on steel (guard/loosen it in the f16 build), not alongside it.

2. Overengineering — steel_f16 is largely redundant

Given #1, the genuinely new coverage here is steel_f16_edge (partial M and N tiles + f16 boundary masking — a shape the sweep didn't previously exercise under the f16 kernel). steel_f16, being the same shape as steel, adds no new tile/masking path; its only distinguishing feature is a looser tolerance on an output steel already checks. Consider dropping steel_f16 and keeping just steel_f16_edge, unless the intent is specifically to document "this shape needs 2.3e-4 under f16" — in which case that's really an argument that steel's gate is wrong in the f16 build (see #1).

3. Minor — orphaned fixtures in f32 builds

Python CONFIGS isn't #ifdef-gated (correctly — it can't know the runtime build), so export_all_quantized_linear_models always writes q4gsw_steel_f16.pte / q4gsw_steel_f16_edge.pte. In a default f32 build the C++ table never references them, so they're exported-but-unused. Harmless, worth a one-line note in the Python comment that these fixtures are only consumed by an f16 build.

Nits

  • The two new C++ entries duplicate 2.3e-4f, 1e-3f. Fine as-is, but a named constant (kSteelF16Abs) would document the "f16 rounding floor" rationale in one place rather than in two comments.
  • steel_f16_edge uses the same 2.3e-4 abs as steel_f16 despite K=1024 vs 2048 — consistent with your "uniform in K, not accumulate" claim, so correct; just confirming that was intentional and not copy-paste.

Overall: small, well-documented, and the edge config is a good addition. The thing to resolve before landing is #1 — whether the unguarded steel config survives the f16 kernel swap. Fix this →
• branch gh/JCNTH/7/head

[ghstack-poisoned]
JCNTH added a commit that referenced this pull request Jul 6, 2026
Pull Request resolved: #20753

**Adds numeric coverage for the f16-multiply steel q4gsw kernel** from the parent diff, which runs only in a `-DWGPU_BACKEND_STEEL_F16` build and had none. Its f16 multiply has a rounding floor (~2.3e-4) above the strict f32 golden (1e-4), so the new configs use a looser abs tolerance without touching the f32 golden.

**Key changes:**
- `test_quantized_linear.py` CONFIGS — add `steel_f16` (same shape as the `steel` config, exact-N tile) and `steel_f16_edge` (M=70, K=1024, N=136, partial M and N tiles). The exported `.pte` is dtype-independent, so each fixture drives whichever steel kernel the runtime build selected.
- `test_webgpu_native.cpp` `kQ4gswConfigs` — matching `steel_f16`/`steel_f16_edge` entries at abs 2.3e-4 / rel 1e-3, both under `#ifdef WGPU_BACKEND_STEEL_F16`. The native sweep self-discovers each `.pte` by name and goldens against the same fp64 truth the f32 configs use.

The looser abs gate reflects the f16 rounding floor (uniform in K, not an accumulate artifact); the accumulator stays f32. The default f32 build's config table and its strict 1e-4 golden are `#ifdef`-excluded and byte-unchanged.

Co-authored-with: Claude Code.
ghstack-source-id: 400359070
@exported-using-ghexport

Differential Revision: [D110802559](https://our.internmc.facebook.com/intern/diff/D110802559/)
[ghstack-poisoned]
JCNTH added a commit that referenced this pull request Jul 7, 2026
Pull Request resolved: #20753

**Adds numeric coverage for the f16-multiply steel q4gsw kernel** from the parent diff, which runs only in a `-DWGPU_BACKEND_STEEL_F16` build and had none. Its f16 multiply has a rounding floor (~2.3e-4) above the strict f32 golden (1e-4), so the new configs use a looser abs tolerance without touching the f32 golden.

**Key changes:**
- `test_quantized_linear.py` CONFIGS — add `steel_f16` (same shape as the `steel` config, exact-N tile) and `steel_f16_edge` (M=70, K=1024, N=136, partial M and N tiles). The exported `.pte` is dtype-independent, so each fixture drives whichever steel kernel the runtime build selected.
- `test_webgpu_native.cpp` `kQ4gswConfigs` — matching `steel_f16`/`steel_f16_edge` entries at abs 2.3e-4 / rel 1e-3, both under `#ifdef WGPU_BACKEND_STEEL_F16`. The native sweep self-discovers each `.pte` by name and goldens against the same fp64 truth the f32 configs use.

The looser abs gate reflects the f16 rounding floor (uniform in K, not an accumulate artifact); the accumulator stays f32. The default f32 build's config table and its strict 1e-4 golden are `#ifdef`-excluded and byte-unchanged.

Co-authored-with: Claude Code.
ghstack-source-id: 400471240
@exported-using-ghexport

Differential Revision: [D110802559](https://our.internmc.facebook.com/intern/diff/D110802559/)
[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