Skip to content

[ExecuTorch][WebGPU] Use requiredFeatures instance API on native + emscripten Dawn#20652

Open
JulianCloudNTH wants to merge 6 commits into
gh/JulianCloudNTH/84/basefrom
gh/JulianCloudNTH/84/head
Open

[ExecuTorch][WebGPU] Use requiredFeatures instance API on native + emscripten Dawn#20652
JulianCloudNTH wants to merge 6 commits into
gh/JulianCloudNTH/84/basefrom
gh/JulianCloudNTH/84/head

Conversation

@JulianCloudNTH

@JulianCloudNTH JulianCloudNTH commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Stack from ghstack (oldest at bottom):

Key the timedWaitAny instance setup to the actual Dawn API instead of __EMSCRIPTEN__, so native-rig Dawn and emscripten/emdawnwebgpu use the modern requiredFeatures path and only the vendored Dawn uses the legacy capabilities.* path.

The instance-descriptor setup was guarded by #if defined(__EMSCRIPTEN__), which routed emscripten (emdawnwebgpu, emcc 4.0.19+) through the legacy capabilities.* API that no longer exists there. The guard now keys off the API actually present.

Key changes:

  • WebGPUDevice.cpp#if defined(__EMSCRIPTEN__)#if defined(WEBGPU_DAWN_INSTANCE_CAPABILITIES). The legacy instance_desc.capabilities.* path is taken only by the buck-vendored Dawn (which defines the macro); native cmake Dawn and emscripten leave it undefined and take the requiredFeatures / WGPUInstanceFeatureName_TimedWaitAny path.

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

Differential Revision: D110149678

Differential Revision: D110149678

[ghstack-poisoned]
@pytorch-bot

pytorch-bot Bot commented Jun 30, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit c8dad50 with merge base 124b677 (image):

NEW FAILURE - The following job has failed:

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 Jun 30, 2026
@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.

@SS-JIA SS-JIA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review automatically exported from Phabricator review in Meta.

[ghstack-poisoned]
JulianCloudNTH added a commit that referenced this pull request Jul 2, 2026
…scripten Dawn

Pull Request resolved: #20652

**Key the `timedWaitAny` instance setup to the actual Dawn API instead of `__EMSCRIPTEN__`, so native-rig Dawn and emscripten/emdawnwebgpu use the modern `requiredFeatures` path and only the vendored Dawn uses the legacy `capabilities.*` path.**

The instance-descriptor setup was guarded by `#if defined(__EMSCRIPTEN__)`, which routed emscripten (emdawnwebgpu, emcc 4.0.19+) through the legacy `capabilities.*` API that no longer exists there. The guard now keys off the API actually present.

Key changes:
- `WebGPUDevice.cpp` — `#if defined(__EMSCRIPTEN__)` → `#if defined(WEBGPU_DAWN_INSTANCE_CAPABILITIES)`. The legacy `instance_desc.capabilities.*` path is taken only by the buck-vendored Dawn (which defines the macro); native cmake Dawn and emscripten leave it undefined and take the `requiredFeatures` / `WGPUInstanceFeatureName_TimedWaitAny` path.

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

Differential Revision: [D110149678](https://our.internmc.facebook.com/intern/diff/D110149678/)
[ghstack-poisoned]
JulianCloudNTH added a commit that referenced this pull request Jul 2, 2026
…scripten Dawn

Pull Request resolved: #20652

**Key the `timedWaitAny` instance setup to the actual Dawn API instead of `__EMSCRIPTEN__`, so native-rig Dawn and emscripten/emdawnwebgpu use the modern `requiredFeatures` path and only the vendored Dawn uses the legacy `capabilities.*` path.**

The instance-descriptor setup was guarded by `#if defined(__EMSCRIPTEN__)`, which routed emscripten (emdawnwebgpu, emcc 4.0.19+) through the legacy `capabilities.*` API that no longer exists there. The guard now keys off the API actually present.

Key changes:
- `WebGPUDevice.cpp` — `#if defined(__EMSCRIPTEN__)` → `#if defined(WEBGPU_DAWN_INSTANCE_CAPABILITIES)`. The legacy `instance_desc.capabilities.*` path is taken only by the buck-vendored Dawn (which defines the macro); native cmake Dawn and emscripten leave it undefined and take the `requiredFeatures` / `WGPUInstanceFeatureName_TimedWaitAny` path.

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

Differential Revision: [D110149678](https://our.internmc.facebook.com/intern/diff/D110149678/)
@JulianCloudNTH

Copy link
Copy Markdown
Contributor Author

@claude Review thoroughly. Check for stale code / bugs.

@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown

Claude finished @JulianCloudNTH's task in 1m 54s —— View job


Review: WebGPU requiredFeatures instance API

  • Read PR diff
  • Analyze WebGPUDevice.cpp for stale code / bugs
  • Check where WEBGPU_DAWN_INSTANCE_CAPABILITIES is defined
  • Post review

Verdict: the change is correct and does exactly what the description claims. No bugs or stale code in the diff. A couple of notes below.


The change is sound

The old guard #if defined(__EMSCRIPTEN__) conflated "is this emscripten?" with "does this Dawn expose the legacy capabilities.* API?". Since emcc 4.0.19+ / emdawnwebgpu switched to requiredFeatures, emscripten was being routed through an API that no longer exists there. Keying off WEBGPU_DAWN_INSTANCE_CAPABILITIES (defined only by the buck-vendored Dawn) is the right fix — native cmake Dawn and emscripten both fall through to the requiredFeatures / WGPUInstanceFeatureName_TimedWaitAny path.

No lifetime bug in the #else branch

WGPUInstanceFeatureName features[1] = {WGPUInstanceFeatureName_TimedWaitAny};
instance_desc.requiredFeatureCount = 1;
instance_desc.requiredFeatures = features;
...
ctx.instance = wgpuCreateInstance(&instance_desc);   // features[] still in scope

instance_desc.requiredFeatures is a borrowed pointer, but wgpuCreateInstance consumes it synchronously on the very next line while features is still alive. No dangling pointer. ✅

Notes / things to confirm (not blocking)

  1. WEBGPU_DAWN_INSTANCE_CAPABILITIES is not defined anywhere in the OSS tree. I grepped the whole repo — the macro only appears at the #if site. That is expected for OSS (native cmake + emscripten want the requiredFeatures path, so the macro should stay undefined). The definition lives in the internal buck build. Worth double-checking that the buck-vendored Dawn target still -D's this macro; if that define is ever dropped, buck-vendored Dawn would silently take the requiredFeatures branch and fail to compile against its older capabilities.* header. Since the OSS side can't verify it, a one-line comment near the macro's buck definition pointing back here would help future readers.

  2. The other __EMSCRIPTEN__ guard (line 188) is correctly left alone. get_default_webgpu_context()'s #if !defined(__EMSCRIPTEN__) gates the native-only lazy process-wide context — that's genuinely about platform (browser has no eager device bring-up), not about the Dawn API version, so it should not be converted. Good that it wasn't touched.

  3. Comment accuracy — the inline comment ("Vendored (buck) Dawn uses the older capabilities.* API…") is accurate and matches the guard. Nice to have the rationale in-tree.

Nothing to change. LGTM. 👍
· branch gh/JulianCloudNTH/84/head

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

3 participants