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

Emit SPIR-V for glsl shaders #6966

Closed

Conversation

Neo-Zhixing
Copy link
Contributor

Naga's SPIR-V conversion seems to be working fine now.

There are built-ins in glsl unsupported by wgsl (for example gl_PointSize) so it matters that we convert glsl directly to SPIR-V without using the wgsl writer.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Dec 16, 2022
@alice-i-cecile
Copy link
Member

@cwfitzgerald do you think this is ready?

@cwfitzgerald
Copy link

It's ready, I just don't get why you'd do it.

Our spirv frontend has to do considerable work to "wgsl-ify" the control flow of the shaders in order to translate them. The wgsl frontend has minimal work to do.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Dec 16, 2022
@mockersf
Copy link
Member

@Neo-Zhixing do you have an interest in moving to SPIRV or is it just to resolve the TODO?

@cart this TODO is from you, any reason to go forward with it?

@Neo-Zhixing
Copy link
Contributor Author

@Neo-Zhixing do you have an interest in moving to SPIRV or is it just to resolve the TODO?

@cart this TODO is from you, any reason to go forward with it?

There are certain things missing from the wgsl spec, for example gl_PointSize. I rewrote my shader code to glsl so that I can use those primitives, but the pipeline creation is still failing as the shader is getting converted into wgsl anyways.

@cart
Copy link
Member

cart commented Dec 21, 2022

Our spirv frontend has to do considerable work to "wgsl-ify" the control flow of the shaders in order to translate them. The wgsl frontend has minimal work to do.

@cwfitzgerald
Hmm does this mean that precompiling shaders to SPIRV is less advisable than precompiling to WGSL? In a world where we have asset preprocessing and we're trying to eliminate as much work as possible for deployed shaders, what should we do? I haven't been keeping track: does WGPU currently have support for direct "pass through" of pre-compiled shaders for a given backend (and does it plan to)?

@cart this TODO is from you, any reason to go forward with it?

@mockersf
In my head when I wrote this TODO, it was because it seemed like on platforms that supported spirv directly (ex: Vulkan), this would be cheaper than doing WGSL (which would need to be translated into something vulkan understands).

If that isn't the case, then I don't have other rationale. However @Neo-Zhixing's point about supporting features that WGSL does not seems relevant / important. It would come down to "speed vs features" and we'd want to enumerate the features we're losing and compare that to the speed cost.

@cwfitzgerald
Copy link

cwfitzgerald commented Dec 21, 2022

Hmm does this mean that precompiling shaders to SPIRV is less advisable than precompiling to WGSL? In a world where we have asset preprocessing and we're trying to eliminate as much work as possible for deployed shaders, what should we do?

Current state of affairs: translate to wgsl to give to wgpu/Naga. This gives the least amount of work for Naga to do.

I haven't been keeping track: does WGPU currently have support for direct "pass through" of pre-compiled shaders for a given backend (and does it plan to)?

No and yes, see gfx-rs/wgpu#3103.

on platforms that supported spirv directly (ex: Vulkan), this would be cheaper than doing WGSL (which would need to be translated into something vulkan understands).

To explain this point a bit more, in order to validate the shader, we need to parse, normalize, and analyze it, which all happens on a wgsl-ish Naga IR. We can offer passthrough shaders which bypass this (see issue) but we also reserve the right to (and do) modify the vulkan abi of shaders for various reasons. So any passthrough shaders need to conform to that new abi.

Also note that spirv-in is missing major features like atomics. Though neither does glsl-in, so your not losing much.

Additionally, vulkan as default on windows is going to go away, likely in 0.16.

@Neo-Zhixing
Copy link
Contributor Author

Neo-Zhixing commented Dec 21, 2022

@cart So are we blocked on gfx-rs/wgpu#3103, or can we merge this PR as-is for now and revisit later when 3103 gets resolved?

I would prefer that we use the pass-through option by default for glsl,hlsl and spv. If people are using non-wgsl, they're probably doing so for a reason.

@cwfitzgerald
Copy link

I think the correct course of action at this point is to close.

Allowing passthrough is fine, but it would need to be through an unsafe api as we provide no validation with passthrough.

@alice-i-cecile
Copy link
Member

Deferring to the experts here. I'm not opposed to this functionality, but it sounds like this needs a bit more careful thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants