Add Instance::wgsl_language_features#6814
Conversation
ImplementedLanguageExtension as WGSLLanguageExtensionWGSLLanguageExtension
|
cc @ErichDonGubler, who wrote #6437. |
cwfitzgerald
left a comment
There was a problem hiding this comment.
Looking good, some comments
8787ee3 to
e9ab502
Compare
|
rebased and squashed into two commits (one for naga changes, one for wgpu changes). |
e9ab502 to
b89fe77
Compare
ErichDonGubler
left a comment
There was a problem hiding this comment.
Mostly LGTM, mostly nit-level things and some maintainability concerns.0
| const ALL: [ImplementedLanguageExtension; 0] = []; | ||
|
|
||
| /// Returns slice of all variants of [`ImplementedLanguageExtension`] | ||
| pub const fn all() -> &'static [Self] { | ||
| &Self::ALL | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh didn't know we already have strum as dep, will fix this.
There was a problem hiding this comment.
Actually it's only dev-dep and it's not present in FF, are we still okay to add it?
There was a problem hiding this comment.
I did it anyway in 0bc74e2 using VariantArray. I will revert it if we do not want strum as dep.
| "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) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
b89fe77 to
ce36775
Compare
c566f93 to
2900b24
Compare
WGSLLanguageExtensionWgslLanguageExtension
WgslLanguageExtensionWgslLanguageFeatures
WgslLanguageFeaturesInstance.wgsl_language_features
Instance.wgsl_language_featuresInstance::wgsl_language_features
2900b24 to
9e58b3d
Compare
…ension` Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com> Co-Authored-By: Erich Gubler <erichdongubler@gmail.com>
9e58b3d to
b74ab18
Compare
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com> Co-Authored-By: Erich Gubler <erichdongubler@gmail.com>
…antArray` Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
b74ab18 to
2e3b652
Compare
|
PR in servo: servo/servo#34928 |
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).
PR in servo: servo/servo#34928
Testing
There is none.
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.