-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Precompiled shaders macro #8217
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
base: trunk
Are you sure you want to change the base?
Precompiled shaders macro #8217
Conversation
… (unsure why), testing if its only on macos/msl
@cwfitzgerald This doesn't require any immediate action from your part, but I think it probably should be brought up in the next maintainers meeting, in terms of where stuff should live (new crate) and how the dependency stuff should work. |
(I'm doing this to see how it does, I'll do a full human review tomorrow, no action is needed based on its comments). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds precompiled shader support to wgpu by introducing a new proc macro crate and corresponding macros to compile shaders with naga at compile time. The key changes enable developers to precompile WGSL, GLSL, and SPIR-V shaders into various target formats (MSL, HLSL, SPIRV, GLSL, DXIL, WGSL) at build time rather than runtime.
- New
wgpu-precompile-macro
crate with proc macros for compile-time shader compilation - Updated
ShaderModuleDescriptorPassthrough
API to use structured descriptors instead of raw strings/bytes - Added feature-gated precompilation support with conditional compilation for different backends
Reviewed Changes
Copilot reviewed 24 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
wgpu/src/macros.rs | Adds new precompilation macros for WGSL, GLSL, and SPIR-V |
wgpu/src/lib.rs | Exports new passthrough descriptor types |
wgpu-types/src/lib.rs | Defines new structured descriptor types for shader passthroughs |
wgpu-precompile-macro/src/lib.rs | Core proc macro implementation for compile-time shader compilation |
wgpu/Cargo.toml | Adds precompile feature and dependencies |
tests/ and examples/ | Updates to use new precompilation API and test the functionality |
Comments suppressed due to low confidence (1)
wgpu-precompile-macro/src/lib.rs:1
- There's a logic error here.
args.shader_stage
is anOption<naga::ShaderStage>
, but you're callingunwrap()
directly in an if condition, which will panic ifNone
. This should beargs.shader_stage.is_some()
or pattern matching.
use hashbrown::HashSet;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Lmao, hashbrown is to blame! |
I'd just like to say about this PR: I'm very satisfied with how it works. I haven't really done much macro programming before but writing it was surprisingly simple and pleasant, I'm shocked it works as well as it does already! Even though its surely a little rough around the edges, not easily configurable, etc. Also, I'd really really like if we could get this in before the release next week, since it'd probably be useful to several wgpu consumers, so if this could be fast tracked I'd appreciate it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool stuff! I have some high level concerns about this and some initial nits when I'm looking it over. Very happy to see progress here though! Doesn't seem to be as awful a transform as I feared.
I do have some conserns about how resource mappings are handled, as the naga backend does take information about where to put various resources (flattening bindings on metal/dx12, dealing with bindless samplers on dx12, etc)
I think we should split this into two separate bits of code. One which is a normal function which does the shader compilation and stuff, then a macro which is a frontend to that function. This will help separation of concerns and will also support doing precompilation in a build script or an external tool, which I would expect to be a more common desire (and I personally would want, as opposed to a proc macro). It should also vastly simplify the cfg situation.
Happy to discuss more about this plan. We can have two crates (wgpu-precompile and wgpu-precompile-macro).
std::fs::write(&input_file, args.hlsl_code.as_bytes()) | ||
.expect("Failed to write to HLSL input file"); | ||
let temporary_file_location = temporary_folder_location.join("file.dxil"); | ||
let output = std::process::Command::new("dxc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely need to make sure the errors when external tools aren't found are very good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the error messages slightly. You probably have a way you'd prefer to do it though
// Related features: dx12 | ||
// Note: this differs slightly from platform-deps which enables dx12 even on linux/android, but just doesn't | ||
// let you use it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels weird? what's up with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the windows-linux-android
platform deps crate:
[features]
gles = ["wgpu-hal/gles"]
vulkan = ["wgpu-hal/vulkan"]
dx12 = ["wgpu-hal/dx12"]
renderdoc = ["wgpu-hal/renderdoc"]
# Depend on wgpu-hal conditionally, so that the above features only apply to wgpu-hal on this set of platforms.
[target.'cfg(any(windows, target_os = "linux", target_os = "android", target_os = "freebsd"))'.dependencies]
wgpu-hal.workspace = true
so the wgpu-hal feature for dx12 is enabled even on linux. So I'm saying here that in the precompile macro, we diverge from that to avoid shelling into DXC on other platforms. Otherwise I tried to copy pretty closely the platform-deps crate
CompileTarget::Msl => { | ||
// Related features: metal | ||
#[cfg(feature = "metal")] | ||
quote! { | ||
target_vendor = "apple" | ||
} | ||
// Always false | ||
#[cfg(not(feature = "metal"))] | ||
always_false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concerns with how far from the syntax of the normal cfgs in wgpu-core etc are. Would there be some way of duplicating the cfg (in the build script) so that this code used the same cfg as wgpu-core does? Keeping this updated is going to be difficult. Additionally this is running at build time, so we can use runtime cfg constructs rather than compile time, which are easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macro crate isn't necessarily compiled specifically for a target, and there's not a way currently to know what target you're compiling for. So all of the platform specific cfgs have to be in the generated code, and the features have to be in the non-generated code. So I'm not sure there's a clean way to match wgpu-core unless I'm misunderstanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do generally agree that what we have right now isn't ideal and wouldn't be very maintainable.
@cwfitzgerald Yeah I kind of just sent it with this PR lol. Do you want to discuss this on a call at some point? Also, do you think that the eventual solution will actually involve a macro, even if it isn't the most popular option? |
Also feel free to bring this to matrix |
Sure if that would make it easier to sync up, that's fine! I'll message you on matrix to set this up.
I think you could implement the macro in terms of the regular function, so there's no code duplication. |
Co-authored-by: Connor Fitzgerald <[email protected]>
…g check against non-uploaded image)
…macro crate, and the whole gang
Connections
Works towards #3103
Description
This adds a new proc macro crate and corresponding proc macro to compile shaders with naga at compile time.
The crate is within the workspace root and not the wgpu crate directory because eventually it will probably be used by
wgpu-core
to precompile internal compute shaders, and because proc-macro crates must be separate crates.Example ripped from test
Limitations
wgpu-core
, so things like timestamp normalization still require runtime shader compilingTesting
I have ensured that the hello_triangle example works using precompiled shaders, on the backends Metal, Vulkan, DX12 with HLSL or DXIL, and GLES. WGSL should in theory also be perfectly fine. Also added a test to ensure that precompiled shaders can be compiled and can be constructed into shader modules.
Squash or Rebase?
Squash
Checklist
cargo fmt
.taplo format
.cargo clippy --tests
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
entry.