Skip to content

Initial precompiled shaders implementation #7834

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

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

Conversation

SupaMaggie70Incorporated
Copy link
Contributor

@SupaMaggie70Incorporated SupaMaggie70Incorporated commented Jun 20, 2025

Connections
Works towards #3103
Depends on #7831

Also, as this is the start of a big change, I'd like to ping @cwfitzgerald to at least make sure I haven't gone off in the wrong direction.

Description
This adds a CreateShaderModuleDescriptorPassthrough::Generic enum variant which contains code for multiple types of shader source,allows creating passthrough shaders without writing backend specific code (if on metal pass MSL, etc). This variant also includes an optional reflection thing. For now, I don't know exactly what reflection info is needed, but if possible, we should make sure this can live in wgpu-types or wgpu-core to avoid dependency on naga. It seems the best model for this is the wgpu_core::validation::Interface, we might just have to replace some of the naga types.

Using this requires enabling the EXPERIMENTAL_PRECOMPILED_SHADERS feature. This feature is only supported on DX12, Vulkan, and Metal. Logic on these backends is otherwise identical to the respective specific passthrough methods.

Nothing is currently done with the reflection info.

An overview of my approach can be found in this comment. If this process takes longer than expected we can make a tracking issue. For now I will be posting updates by editing that comment and referencing it in PRs.

Testing
No testing yet. However, the code seems somewhat small and robust.

Squash pls

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@SupaMaggie70Incorporated SupaMaggie70Incorporated marked this pull request as ready for review June 21, 2025 02:45
@SupaMaggie70Incorporated SupaMaggie70Incorporated requested a review from a team as a code owner June 21, 2025 02:45
@cwfitzgerald cwfitzgerald self-assigned this Jun 25, 2025
@cwfitzgerald
Copy link
Member

Hmm, I think in general I like the ideas put forward in your plan you wrote up, that makes sense to me. As for the code changes themselves, I don't think that we need another variant internally - this feels redundant when all the other variants exist. Maybe what would need to be added is an optional reflection info to each of the variants?

@SupaMaggie70Incorporated
Copy link
Contributor Author

SupaMaggie70Incorporated commented Jun 25, 2025

@cwfitzgerald The purpose of this variant is so that the user can pass a single shader without considering backend-specific code. This way we can also later on add a macro that creates this struct, filling fields for all the backends that are supported, and it doesn't need to be passed a Backend parameter. I can change the approach if you want, but it might make everything a little less clean from the user's point of view.

@cwfitzgerald
Copy link
Member

Yeah, I agree that it makes sense in wgpu's api but wgpu-core and wgpu-hal should be able to use the original ones?

@SupaMaggie70Incorporated
Copy link
Contributor Author

Yeah, I agree that it makes sense in wgpu's api but wgpu-core and wgpu-hal should be able to use the original ones?

True, didn't think about that to be honest. I'll rewrite that part of this PR

@jimblandy
Copy link
Member

@cwfitzgerald When you're back - this has been waiting for a while.

Copy link
Collaborator

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I'm really looking forward to this! Just some things which popped up!

@DJMcNab DJMcNab removed their assignment Aug 6, 2025
@SupaMaggie70Incorporated
Copy link
Contributor Author

@DJMcNab Thanks for the review! Upon further inspection it seems my PR was not super well thought through or checked, so I'll have another look over everything myself in addition to addressing your comments.

@SupaMaggie70Incorporated
Copy link
Contributor Author

@DJMcNab I've just given this PR a final look and it looks good. Some of your comments I left open as I wasn't sure how to handle them. Let me know what you think, you get the final say of course!

Also pinging @cwfitzgerald because this hopefully can be merged soon

Another note: the only backend not currently supported is OpenGL. Once we get that done, maybe we should consider removing this as a feature? I'm not sure how that would interact with e.g. custom backends so I'll wait until later to inquire about that.

@DJMcNab DJMcNab removed their request for review August 11, 2025 08:05
@DJMcNab
Copy link
Collaborator

DJMcNab commented Aug 11, 2025

I'm not planning to review this again - it needs to be reviewed by someone with write access, and I was providing a "first pass" review to help speed this along, but I can't give a fully "in-context" review.

Thanks for actioning the review feedback so well! I've found it easy to submit PRs which don't hold up when given a second reading myself as well - that's what review is for, after all!

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.

A few small comments, but this is exactly what I was looking for!

CHANGELOG.md Outdated
Comment on lines 56 to 57
### General
- Added `Features::EXPERIMENTAL_PRECOMPILED_SHADERS`, replacing existing passthrough types with a unified `CreateShaderModuleDescriptorPassthrough` which allows passing multiple shader codes for different backends. By @SupaMaggie70Incorporated in [#7834](https://github.com/gfx-rs/wgpu/pull/7834)
Copy link
Member

Choose a reason for hiding this comment

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

This should be a major changes with a small things showing off the diff and the new API

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 made a new changelog entry, let me know if you want more changes to it

@SupaMaggie70Incorporated
Copy link
Contributor Author

Alright @cwfitzgerald, I think I've addressed most of the recent changes (and also updated the PR to latest trunk). If there's anything more let me know.

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.

4 participants