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

Support 16-bit unorm/snorm formats #2210

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Conversation

fintelia
Copy link
Contributor

This PR adds handling for 16-bit unorm and snorm formats.

After making a few tweaks to wgpu, I was able to use this patch to load and run several GLSL compute shaders operating on r16 unorm textures. I haven't tested the other frontends/backends but the code I added mirrors the existing cases, so there's a good chance they should behave properly

@jimblandy
Copy link
Member

This looks interesting, but before I review in detail:

We can't unconditionally extend WGSL, since that affects Firefox's implementation of the WebGPU standard, and we'd introduce incompatibilities between browsers. This probably needs to be conditional on some new flag in naga::valid::Capabilities.

@fintelia
Copy link
Contributor Author

Looks like #2162 lays out what the capability should be. I'll see about adding suitable validation checks when I have a chance

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

LGTM, but could you also add them to the SPV front-end?

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

The WGSL changes need to be conditional, as noted in the conversation.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

@teoxoy points out that #2161 covers making the non-WebGPU formats we already support in the WGSL front end conditional, and that's in milestone 4. So I think this is fine to merge as it is.

@teoxoy
Copy link
Member

teoxoy commented Jan 17, 2023

Sorry, I approved it without reading the comments properly.

I thought gfx-rs/wgpu#4412 would cover this and could be done in a separate PR since there are a bunch of other formats. But ideally this PR should close #2162 (we don't yet have the framework for naga specific extensions but we can move that point from the issue into gfx-rs/wgpu#4410).

@fintelia adding the Capability would be appreciated since we have to wire it to wgpu as well.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks for introducing the new capability!
Could you also add the new formats to the spv front-end?

@teoxoy teoxoy merged commit 1be8024 into gfx-rs:master Jan 18, 2023
@fintelia fintelia deleted the unorm16bit branch January 18, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants