Repository navigation
[WebGPU] Add scoped code review skill - #33933
Open
Yang Gu (gyagp) wants to merge 9 commits into
Open
Yang Gu (gyagp) wants to merge 9 commits into
Yang Gu (gyagp) wants to merge 9 commits into
Conversation
Preserve the WebGPU guidance from 4453ec9 and organize it behind code-review-webgpu with cumulative reading routes and 13 focused references. Add source and historical-review coverage evidence, explicit non-WebGPU exclusion, and repository-contained guidance for local and Copilot reviews. This keeps unrelated review instructions unchanged and avoids loading the full WebGPU checklist or historical corpus for every review. Clarify the current shader validation job and verify actual cache identity for overrides. Validated skill metadata, all original requirement mappings, 61 local links, 13 routes, positive/negative scope probes, and git diff --check. Changes are documentation-only; no ORT build, GPU execution, or hosted Copilot run claimed.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Make the native review scope gate follow the existing JSEP deprecation policy. A JSEP kernel fix must inspect the native counterpart and include the native fix and regression when the same defect applies; a blanket JSEP-only exclusion could hide that requirement. Keep unaffected JSEP maintenance outside native checks, direct feature and performance work to native WebGPU, and distinguish JSEP operator listings from native registration. Add deprecation-aware selection walkthroughs. Validated skill metadata, 64 repository links, all 13 reference routes, original requirement preservation, scope probes, and git diff --check. Documentation-only; no ORT build, GPU run, or hosted Copilot run claimed.
The existing review check required shader variable accessors only for tensors larger than a storage binding, leaving direct indexing of smaller tensors implicitly allowed. Make helper-only access explicit at kernel/template call sites so segmentation, buffer views and storage conversions stay centralized. Require extending the helper API for unsupported access, while allowing helper internals and workgroup/private arrays. Guidance and skill validation pass; this documentation-only change does not require an ORT build or GPU test.
Make the existing build, runtime and kernel references explicitly cover Dawn rolls, supported concurrency contracts and end-to-end native kernel authoring. Keep their detail behind selective reading routes and add source-backed review walkthroughs without activating WebGPU guidance for unrelated changes. Distinguish independent plugin sessions from same-session Run serialization and caller-serialized compatibility mode. Clarify callback result lifetime and correct the current Release validation default against source. Validated skill metadata, 84 repository links, all 13 routes, original guidance preservation, scope probes and whitespace checks. Documentation only; no Dawn roll, GPU run or hosted Copilot review was performed.
Historical PR lists and regression tables repeat the review rules and add context and maintenance cost as the skills evolve. Move their provenance to the maintenance audit and remove generic build snippets and snapshot scenarios. Keep the substantive checks, helper API names and WebGPU validation tools. Direct reviewers to current source/workflows for changing defaults and CI coverage, and retain examples only when they clarify an ambiguous invariant. Validated 83 repository links, all 13 routes, original check/provenance retention, skill metadata, scope probes and whitespace. Documentation only; no GPU or hosted Copilot execution was performed.
Consolidate co-used checks into eight references and shorten their wording while retaining the original invariants, corrective actions and regression checks. Keep historical evidence outside ordinary review context and document pre-merge Copilot trials on heads containing real code changes. Exercise the guidance through bounded source review of 22 pinned WebGPU PRs and three unrelated controls. Refine barrier uniformity, native alias ownership, segmented test limits and profiling proc ownership from verified source and feedback. Make review-time CI-only execution explicit and avoid requiring CI wiring for intentionally manual tests without a contract. Validated skill metadata, 67 local links, eight routes, original requirement retention, scope isolation and whitespace. No builds, source tests, GPU workloads or CI dispatch during the expanded pass; hosted Copilot selection and generated comment quality still require a live trial.
Keep the detailed source and historical review audit in local task evidence instead of shipping it as an upstream maintenance document. Remove both incoming links and retain a short attribution to Devang's original contribution in the skill and PR description. All eight review references and their checks are unchanged. Validated skill metadata, 50 repository links/anchors, eight routes, scope isolation and whitespace. No builds, source tests, GPU workloads or CI dispatch performed.
Replace the eight mixed topic references with three responsibility groups, using short filenames that correspond to their contents: - computation.md: operators and computation, including host code and WGSL. - execution.md: execution and memory, including threads and profiling code. - platform.md: platform support and release, including devices and packaging. Keep validation.md for shared test coverage and performance evidence rather than a fourth subsystem category. Separate cache-key correctness from cache lifetime and operator support from host ABI compatibility. Preserve existing checks and read relevant sections within each selected category. Verified 18 migration blocks, original requirement retention, skill metadata, 47 local links, three category routes plus shared evidence, scope probes and whitespace. Reassessed the existing 22 pinned WebGPU PR scopes and three unrelated controls; this is manual routing evidence, not hosted selection telemetry or statistical independence. No builds or source tests performed.
Yang Gu (gyagp)
force-pushed
the
pr-webgpu-pr-review-1a80e471-5e97-4951-a55c-44276856d102
branch
from
November 6, 2026 07:26
62b7974 to
26721c0
Compare
Yang Gu (gyagp)
marked this pull request as ready for review
November 6, 2026 07:32
Contributor
Author
|
devang-ml Based on your guidance for WebGPU EP code review, I worked out this PR. Please review it, thanks! |
Contributor
Author
|
Jiajia Qin (@qjia7) Please review |
Contributor
Author
|
/run bots |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The blanket binding prohibition incorrectly rejects supported segmented outputs and should apply only to overlapping byte ranges when either binding is writable.
1 open issue
What changed in this PR
Adds a scoped native WebGPU review skill to ONNX Runtime’s agent guidance, without changing runtime behavior.
Changes:
- Adds conditional entry points that select WebGPU guidance only for direct or traceable WebGPU effects.
- Organizes checks into computation, execution, and platform references, with shared test and performance evidence guidance.
- Documents guidance maintenance and pre-merge hosted-review trials, keeping build and test execution outside the review workflow.
| File | Description |
|---|---|
docs/Agent_Coding_Guidance.md |
Documents selective routing, maintenance, and hosted-review trials. |
.github/skills/code-review-webgpu/SKILL.md |
Defines applicability, reference selection, and review completion criteria. |
.github/skills/code-review-webgpu/references/validation.md |
Covers test coverage, existing CI results, and performance evidence. |
.github/skills/code-review-webgpu/references/platform.md |
Covers devices, plugin contracts, dependencies, and packaging. |
.github/skills/code-review-webgpu/references/execution.md |
Covers concurrency, resource lifetime, capture, and profiling. |
.github/skills/code-review-webgpu/references/computation.md |
Covers operator support, shader correctness, numerics, and graph changes. |
.github/instructions/webgpu-ep.instructions.md |
Routes matching native WebGPU paths to the skill. |
.github/copilot-instructions.md |
Adds a repository-wide conditional WebGPU entry point. |
Review effort: Balanced
| indexing. Helpers preserve segmentation above `maxStorageBufferBindingSize`, | ||
| view offsets, and storage/value conversion; extend the API when necessary. | ||
| Helper-internal indexing and workgroup/private arrays are exempt. | ||
| - Never bind one buffer twice when either binding is writable. |
Contributor
Author
There was a problem hiding this comment.
Fixed: the rule now rejects overlapping writable ranges and allows disjoint segmented bindings.
Contributor
|
The request was received. Approve workflow runs. Thanks. |
The buffer-identity-only rule rejects supported segmented outputs, which bind disjoint ranges of one storage buffer. Restrict the prohibition to overlapping byte ranges when either binding is writable, including dynamic offsets. Verified against ProgramManager, WebGpuContext::CreateBindGroup and the WebGPU specification. Skill metadata, guidance links/scope checks and whitespace checks pass. Documentation only; no builds or runtime tests triggered.
This branch has not been deployed
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.
Description
Derived from Devang Patel's original WebGPU review guidance, this adds
code-review-webgpufor native WebGPU implementation and review. Conditional Copilot/path entry points preserve the relevance gate: unrelated reviews keep their existing guidance, and shared changes qualify only through an affected WebGPU consumer.Motivation and Context
Organize checks into three broad responsibilities with plain names:
computation.md: operator support, host/kernel/WGSL implementation, cache keys, numerics, layouts, registration and graph changes.execution.md: scheduling, threads, callbacks, buffers/transfers, capture/replay, cache storage and profiling code.platform.md: options/devices/features, host/plugin API/ABI, Dawn/DXC, builds, packaging and release tools.Kernel host code and shaders stay together. Each check has one owner; a PR can affect several categories. Cache-key meaning differs from cache-entry lifetime, and operator acceptance differs from host ABI compatibility. These are responsibility boundaries, not a claim of mathematically independent changes. Read relevant sections within selected categories.
Test coverage, existing CI evidence and performance measurements are shared review steps, not peer subsystem categories. Reviewers do not configure/build, install dependencies, run workloads or dispatch CI. CI bots own execution; implementation tasks retain their separately authorized workflows.
The checks include Dawn rolls, thread safety, new kernels, helper-only tensor storage access and deprecated JSEP's native-counterpart policy. Hosted guidance is committed on the PR head branch; the documentation explains pre-merge trials. Detailed audit/evaluation evidence stays local. Existing generic, JSEP and local-testing instructions remain unchanged.
Validation