Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement WGSLLanguageExtension #6814

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Dec 23, 2024

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 sagudev changed the title [naga] Expose ImplementedLanguageExtension as WGSLLanguageExtension Implement WGSLLanguageExtension Dec 23, 2024
@sagudev sagudev marked this pull request as ready for review December 23, 2024 08:36
@sagudev sagudev requested review from a team as code owners December 23, 2024 08:36
@sagudev
Copy link
Contributor Author

sagudev commented Dec 23, 2024

cc @ErichDonGubler, who wrote #6437.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Looking good, some comments

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 sagudev requested a review from cwfitzgerald December 24, 2024 06:56
wgpu/Cargo.toml Outdated Show resolved Hide resolved
@sagudev sagudev requested a review from cwfitzgerald December 24, 2024 07:27
@sagudev sagudev force-pushed the wgsl-language-features branch from 8787ee3 to e9ab502 Compare December 30, 2024 09:08
@sagudev
Copy link
Contributor Author

sagudev commented Dec 30, 2024

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

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

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 70 to 75
const ALL: [ImplementedLanguageExtension; 0] = [];

/// Returns slice of all variants of [`ImplementedLanguageExtension`]
pub const fn all() -> &'static [Self] {
&Self::ALL
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
naga/src/front/wgsl/mod.rs Outdated Show resolved Hide resolved
Comment on lines +7 to +23
bitflags::bitflags! {
/// WGSL language extensions.
///
/// WGSL spec.: <https://www.w3.org/TR/WGSL/#language-extensions-sec>
#[derive(Debug, Clone, PartialEq, PartialOrd, Ord, Eq, Hash)]
pub struct WGSLLanguageFeatures: u32 {
/// <https://www.w3.org/TR/WGSL/#language_extension-readonly_and_readwrite_storage_textures>
const ReadOnlyAndReadWriteStorageTextures = 1 << 0;
/// <https://www.w3.org/TR/WGSL/#language_extension-packed_4x8_integer_dot_product>
const Packed4x8IntegerDotProduct = 1 << 1;
/// <https://www.w3.org/TR/WGSL/#language_extension-unrestricted_pointer_parameters>
const UnrestrictedPointerParameters = 1 << 2;
/// <https://www.w3.org/TR/WGSL/#language_extension-pointer_composite_access>
const PointerCompositeAccess = 1 << 3;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

question: Is there a reason this shouldn't be in wgpu-types? This would also have the benefit of not needing to add the bitflags dep. to wgpu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, except that it is only used in wgpu.

wgpu/src/api/instance.rs Outdated Show resolved Hide resolved
wgpu/src/backend/webgpu.rs Outdated Show resolved Hide resolved
Comment on lines +1579 to +1590
"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)
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sagudev sagudev Jan 7, 2025

Choose a reason for hiding this comment

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

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.

@ErichDonGubler ErichDonGubler self-assigned this Jan 6, 2025
@ErichDonGubler 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
Cargo.lock Outdated Show resolved Hide resolved
@sagudev sagudev force-pushed the wgsl-language-features branch from b89fe77 to ce36775 Compare January 7, 2025 06:26
Signed-off-by: sagudev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: webgpu Issues with direct interface with WebGPU area: api Issues related to API surface area: naga front-end lang: WGSL WebGPU Shading Language naga Shader Translator
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants