Compute Shaders + ExecutionMode's #195
Conversation
…ed changes do not break rendering example.
…cutionModel. Replaced SpirvAttribute::Entry ExecutionModel with an Entry struct, which includes a Vec of ExecutionMode, ExecutionModeExtra to be submitted in entry.rs. Compute example runs. Passes all tests.
|
wow thanks for the contribution! linking to #180 |
There was a problem hiding this comment.
Oh my goodness, I was gearing up to implement this, and then you just come along and, do it??? Thank you so so so very much, this is incredible! I'm very happy taking this as-is right now (with the -> bool rustc nightly version fixed) and addressing the other feedback in a follow-up (either me or you, either is fine by me).
| todo!() | ||
| } | ||
|
|
||
| fn add_unreachable_region(&mut self, _instance: Instance<'tcx>, _region: CodeRegion) -> bool { | ||
| fn add_unreachable_region(&mut self, _instance: Instance<'tcx>, _region: CodeRegion) { |
There was a problem hiding this comment.
ah, these should actually return bool - make sure you're up to date with the nightly specified in rust-toolchain! (these were changed to return bool recently)
| }); | ||
| } | ||
|
|
||
| fn compute_shader_entry_stub( |
There was a problem hiding this comment.
There's a decent amount of duplication between this and shader_entry_stub (and the existing duplication with kernel too), it might be nice to refactor things into something a little better structured - but probably in a later PR (I can do so), this is fine as-is as a temp solution.
rustc_codegen_spirv/src/symbols.rs
Outdated
| Fragment => { | ||
| //FIXME: Is this the correct default? | ||
| let origin_mode = origin_mode.unwrap_or(OriginUpperLeft); | ||
| e.execution_modes.push((origin_mode, ExecutionModeExtra::new([]))); |
There was a problem hiding this comment.
This is a little convoluted code so I might be missing something, but it might be nice to only specify this if the user didn't do so themselves. (Considering we didn't even allow users to specify it before this PR, this is fine as-is for now, mostly just making a note that we should follow up on this later!)
There was a problem hiding this comment.
This isn't a breaking change. The origin_mode variable is None, and replaced with OriginUpperLeft or OriginLowerLeft if specified. Before it was hardcoded in entry.rs for fragment shaders.
rustc_codegen_spirv/src/symbols.rs
Outdated
| } | ||
| result |
There was a problem hiding this comment.
This whole thing is super nested, it'd be really nice if we could split it into some smaller functions.
|
|
||
| fn main() { | ||
| let spirv = include_bytes_align_as!(u32, env!("wgpu_example_compute_shader.spv")); | ||
| println!("{}\n", disassemble_spirv(bytemuck::cast_slice(spirv))); |
There was a problem hiding this comment.
prooobably shouldn't include this, as all the other examples don't? but shrug, doesn't really matter either way.
…unner to be more similar to other wgpu example. Split of entry logic in symbol.rs to separate function. Fixed issue in builder/mod.rs.
|
I'm not sure why CI isn't running on this - perhaps the merge conflicts need to get resolved first? |
XAMPPRocky
added
the
s: waiting on author
…moved really_unsafe_ignore_bitcasts to its own Symbol match. In entry.rs, entry stubs now return the fn_id, so that entry_stub() can add the execution modes in one place. Passed all tests.
|
Yes, merge conflicts are fixed. |
XAMPPRocky
added
s: waiting on review
| let custom = std::iter::once(( | ||
| "really_unsafe_ignore_bitcasts", | ||
| SpirvAttribute::ReallyUnsafeIgnoreBitcasts, | ||
| )); |
There was a problem hiding this comment.
why split this out into its own thing?
There was a problem hiding this comment.
I figured it was commonly used. And this logic matches descriptor_set, binding. But it probably should be reverted to how it was, in attributes.
| .attributes | ||
| .get(&name.name) | ||
| .map(|a| match a { | ||
| SpirvAttribute::Entry(entry) => SpirvAttribute::Entry( |
There was a problem hiding this comment.
I would prefer this gets handled in the same way descriptor_set and binding is handled, but whatever, that can be cleaned up later.
There was a problem hiding this comment.
So you're saying add additional if else statements for each entry / execution_model? Wouldn't putting everything in the attributes HashMap scale better?
Adds a wgpu compute shader example, "wgpu-example-compute-runner" and "wgpu-example-compute-shader". At minimum to compile and execute a compute shader, ie GLCompute, the execution mode LocalSize must be set. So this PR additionally adds a mechanism to do that:
Emulating GLSL, dimensions not specified default to 1 (gl_compute only).
The execution modes specified within a execution model, are collected into a Vec along with their arguments, and then this is sent as a SpirvAttribute::Entry, which is now special Entry struct. In entry.rs, the stub functions submit these execution modes.
I also added the fn compute_shader_entry_stub as a duplicate of shader_entry_stub, which in the future may handle function parameters differently.
I added the complete list of execution modes. There may be some additional logic for default values that I'm not familiar with, notably you can now specify OriginLowerLeft to vertex shaders, (OriginUpperLeft is the default). There isn't any error handling about which modes are valid per execution model, or specifying multiple conflicting modes. In general they are collected and submitted left to right.
All the tests passed.