Add `Instance::wgsl_language_features` #6814

sagudev · 2024-12-23T07:34:07Z

Connections
#6350

Description
This is to allow browsers to implement https://www.w3.org/TR/webgpu/#gpuwgsllanguagefeatures, currently we do not implement any such extension (but I am working on pointer_composite_access, hence this PR).

Testing
There is none.

Checklist

Run cargo fmt.
Run taplo format.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- --target wasm32-unknown-emscripten
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

sagudev · 2024-12-23T08:39:22Z

cc @ErichDonGubler, who wrote #6437.

cwfitzgerald

Looking good, some comments

wgpu/src/api/instance.rs

wgpu/src/backend/webgpu.rs

wgpu/src/backend/wgpu_core.rs

wgpu/Cargo.toml

naga/src/front/wgsl/parse/directive/language_extension.rs

sagudev · 2024-12-30T09:10:19Z

rebased and squashed into two commits (one for naga changes, one for wgpu changes).

ErichDonGubler

Mostly LGTM, mostly nit-level things and some maintainability concerns.0

ErichDonGubler · 2025-01-06T20:20:18Z

naga/src/front/wgsl/parse/directive/language_extension.rs

+    const ALL: [ImplementedLanguageExtension; 0] = [];
+
+    /// Returns slice of all variants of [`ImplementedLanguageExtension`]
+    pub const fn all() -> &'static [Self] {
+        &Self::ALL
+    }


nitpick: It would be more future-proof to implement this in terms of strum::{EnumIter,IntoEnumIterator}, and return an impl Iterator<Item = ImplementedLanguageExtension instead.more maintenance-proof.

Oh didn't know we already have strum as dep, will fix this.

Actually it's only dev-dep and it's not present in FF, are we still okay to add it?

I did it anyway in 0bc74e2 using VariantArray. I will revert it if we do not want strum as dep.

naga/src/front/wgsl/parse/directive/language_extension.rs

naga/src/front/wgsl/mod.rs

wgpu/src/api/instance.rs

wgpu/src/backend/webgpu.rs

ErichDonGubler · 2025-01-06T20:43:32Z

wgpu/src/backend/webgpu.rs

+                    "readonly_and_readwrite_storage_textures" => {
+                        Some(crate::WGSLLanguageFeatures::ReadOnlyAndReadWriteStorageTextures)
+                    }
+                    "packed_4x8_integer_dot_product" => {
+                        Some(crate::WGSLLanguageFeatures::Packed4x8IntegerDotProduct)
+                    }
+                    "unrestricted_pointer_parameters" => {
+                        Some(crate::WGSLLanguageFeatures::UnrestrictedPointerParameters)
+                    }
+                    "pointer_composite_access" => {
+                        Some(crate::WGSLLanguageFeatures::PointerCompositeAccess)
+                    }


issue: Matching on identifiers here duplicates logic that we already have LanguageExtension::from_ident.

suggestion: We're also going to need to map from ImplementedLanguageExtension to WGSLLanguageFeatures anyway when using the wgpu_core backend, so let's match LanguageExtension::from_ident(wlf.as_str()) (probably using a separate method on WGSLLanguageFeatures or an implementation of Into) instead.

The problem is that here is webgpu impl, that does not necessary have naga available.

But we do use LanguageExtension::from_ident for parsing in core backend.

Mm, I guess I don't see this as a blocking issue, then. Let's file a follow-up issue to deal with "properly" sharing this, if we can come up with a decent solution.

Cargo.lock

…ension` Signed-off-by: sagudev <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

Signed-off-by: sagudev <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

…antArray` Signed-off-by: sagudev <[email protected]>

sagudev · 2025-01-09T05:28:30Z

naga/src/front/wgsl/parse/directive/language_extension.rs

 /// A variant of [`LanguageExtension::Implemented`].
-#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
-pub(crate) enum ImplementedLanguageExtension {}


65 | /// A variant of [`LanguageExtension::Implemented`]. | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this item is private

the problem is that we still only expose ImplementedLanguageExtension outside of this module, hence rustdoc errors. We will either need to expose them all or change it's doc (I did the second option initially).

I think exposing all *LanguageExtension APIs is a fine course of action. However, I think I'd prefer we not rename them, if that's the case.

I assume you will push PR to finish line, let me know if that's not the case.

sagudev changed the title ~~[naga] Expose ImplementedLanguageExtension as WGSLLanguageExtension~~ Implement WGSLLanguageExtension Dec 23, 2024

sagudev marked this pull request as ready for review December 23, 2024 08:36

sagudev requested review from a team as code owners December 23, 2024 08:36

sagudev mentioned this pull request Dec 23, 2024

WGSL: Support the pointer_composite_access language extension #6192

Open

cwfitzgerald requested changes Dec 23, 2024

View reviewed changes

wgpu/src/api/instance.rs Outdated Show resolved Hide resolved

wgpu/src/backend/webgpu.rs Show resolved Hide resolved

wgpu/src/backend/wgpu_core.rs Outdated Show resolved Hide resolved

sagudev requested a review from cwfitzgerald December 24, 2024 06:56

cwfitzgerald requested changes Dec 24, 2024

View reviewed changes

wgpu/Cargo.toml Outdated Show resolved Hide resolved

naga/src/front/wgsl/parse/directive/language_extension.rs Show resolved Hide resolved

sagudev requested a review from cwfitzgerald December 24, 2024 07:27

cwfitzgerald approved these changes Dec 24, 2024

View reviewed changes

sagudev force-pushed the wgsl-language-features branch from 8787ee3 to e9ab502 Compare December 30, 2024 09:08

sagudev force-pushed the wgsl-language-features branch from e9ab502 to b89fe77 Compare January 3, 2025 08:00

ErichDonGubler requested changes Jan 6, 2025

View reviewed changes

ErichDonGubler self-assigned this Jan 6, 2025

ErichDonGubler added area: api Issues related to API surface api: webgpu Issues with direct interface with WebGPU naga Shader Translator area: naga front-end lang: WGSL WebGPU Shading Language labels Jan 6, 2025

ErichDonGubler assigned sagudev Jan 6, 2025

ErichDonGubler reviewed Jan 6, 2025

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

sagudev force-pushed the wgsl-language-features branch from b89fe77 to ce36775 Compare January 7, 2025 06:26

cwfitzgerald requested a review from ErichDonGubler January 8, 2025 16:19

ErichDonGubler force-pushed the wgsl-language-features branch from c566f93 to 2900b24 Compare January 9, 2025 00:06

ErichDonGubler enabled auto-merge January 9, 2025 00:07

ErichDonGubler approved these changes Jan 9, 2025

View reviewed changes

ErichDonGubler added 2 commits January 8, 2025 23:22

docs(naga): clarify desc. for LanguageExtension

2777e0f

feat(naga): expose {,Implemented,Unimplemented}LanguageExtension

df2658a

feat(naga): expose ImplementedLanguageExtension as `WgslLanguageExt…

af6a65f

…ension` Signed-off-by: sagudev <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

ErichDonGubler changed the title ~~Implement WGSLLanguageExtension~~ Implement WgslLanguageExtension Jan 9, 2025

ErichDonGubler changed the title ~~Implement WgslLanguageExtension~~ Implement WgslLanguageFeatures Jan 9, 2025

ErichDonGubler changed the title ~~Implement WgslLanguageFeatures~~ Add Instance.wgsl_language_features Jan 9, 2025

ErichDonGubler changed the title ~~Add Instance.wgsl_language_features~~ Add Instance::wgsl_language_features Jan 9, 2025

sagudev and others added 2 commits January 8, 2025 23:52

feat: add Instance::wgsl_language_features

d860ea9

Signed-off-by: sagudev <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

refactor: reimpl. ImplementedLanguageExtension::all w/ `strum::Vari…

9e58b3d

…antArray` Signed-off-by: sagudev <[email protected]>

ErichDonGubler force-pushed the wgsl-language-features branch from 2900b24 to 9e58b3d Compare January 9, 2025 04:54

sagudev commented Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Instance::wgsl_language_features` #6814

Add `Instance::wgsl_language_features` #6814

sagudev commented Dec 23, 2024 •

edited by ErichDonGubler

Loading

sagudev commented Dec 23, 2024

cwfitzgerald left a comment

sagudev commented Dec 30, 2024

ErichDonGubler left a comment

ErichDonGubler Jan 6, 2025

sagudev Jan 7, 2025

sagudev Jan 7, 2025

sagudev Jan 7, 2025

ErichDonGubler Jan 6, 2025

sagudev Jan 7, 2025 •

edited

Loading

ErichDonGubler Jan 8, 2025

sagudev Jan 9, 2025 •

edited by ErichDonGubler

Loading

ErichDonGubler Jan 9, 2025

sagudev Jan 9, 2025

Add Instance::wgsl_language_features #6814

Are you sure you want to change the base?

Add Instance::wgsl_language_features #6814

Conversation

sagudev commented Dec 23, 2024 • edited by ErichDonGubler Loading

sagudev commented Dec 23, 2024

cwfitzgerald left a comment

Choose a reason for hiding this comment

sagudev commented Dec 30, 2024

ErichDonGubler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagudev Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagudev Jan 9, 2025 • edited by ErichDonGubler Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Instance::wgsl_language_features` #6814

Add `Instance::wgsl_language_features` #6814

sagudev commented Dec 23, 2024 •

edited by ErichDonGubler

Loading

sagudev Jan 7, 2025 •

edited

Loading

sagudev Jan 9, 2025 •

edited by ErichDonGubler

Loading