Skip to content

Conversation

syl20bnr
Copy link
Contributor

@syl20bnr syl20bnr commented Mar 12, 2025

Description
This PR adds the possibility to directly pass a metal shader and create a compute pipeline to execute the passed shader without any validation. This allows CubeCL to use its wgpu runtime to execute metal kernels directly compiled by CubeCL JIT compiler and benefit from support for advanced metal features.

Testing
Tested with a WIP metal compiler using the wgpu runtime of CubeCL.
More info about how to test this soon.

Squash or Rebase?

Ok to be squashed.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. 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.

@syl20bnr syl20bnr requested a review from a team as a code owner March 12, 2025 22:42
@syl20bnr syl20bnr marked this pull request as draft March 12, 2025 22:43
@syl20bnr syl20bnr force-pushed the feat/metal-passthrough branch 3 times, most recently from edef8fa to c049099 Compare March 12, 2025 23:04
Comment on lines 240 to 242
/// Descriptor for a shader module given by Metal MSL sourc, for use with
/// [`Device::create_shader_module_msl`].
///
/// This type is unique to the Rust API of `wgpu`. In the WebGPU specification,
/// only WGSL source code strings are accepted.
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to get some description of the shader ABI we present to shaders and how things get translated. It doesn't need to be as strict as in wgpu-hal, but should guide people on what wgpu expects the shader to have.

@syl20bnr syl20bnr force-pushed the feat/metal-passthrough branch 2 times, most recently from fe86233 to 17b472e Compare March 12, 2025 23:21
@DJMcNab
Copy link
Collaborator

DJMcNab commented Mar 13, 2025

(Linking to #3103)

@syl20bnr
Copy link
Contributor Author

@cwfitzgerald Thank you for the review.

I think we should unify this entrypoint with create_shader_module_spirv and call it create_shader_module_passthrough which takes a ShaderModulePassthrough enum with each descriptor as needed.

I pushed a simple change for this, is that what you had in mind ?

It would be great to get some description of the shader ABI we present to shaders and how things get translated. It doesn't need to be as strict as in wgpu-hal, but should guide people on what wgpu expects the shader to have.

Not sure what do you mean, do you have an example of such description ?

Also can you point me to where I could add a test for the metal passthrough if needed ?

@syl20bnr syl20bnr force-pushed the feat/metal-passthrough branch 2 times, most recently from 87006a1 to 624db2a Compare March 25, 2025 13:34
@cwfitzgerald cwfitzgerald marked this pull request as ready for review April 1, 2025 16:55
@cwfitzgerald cwfitzgerald self-assigned this Apr 2, 2025
@cwfitzgerald
Copy link
Member

cwfitzgerald commented Apr 2, 2025

Not sure what do you mean, do you have an example of such description ?

Take a look at https://wiki.libsdl.org/SDL3/SDL_CreateGPUShader#remarks which talks about the order that SDL requires the shader bindings to be, depending on the backend. Ideally we would have something like this for MSL as its not always straight forward, but we can punt this to another PR if it would be too much hassle for now.

Also can you point me to where I could add a test for the metal passthrough if needed ?

Make sure you update the PR to latest, but then tests/tests/wgpu-gpu has gpu-enabled tests. You can write a test which is skipped on all platforms other that metal, such as

.skip(FailureCase::backend(Backends::all() - Backends::DX12)),
defines a test which operates on all platforms except DX12.

@syl20bnr syl20bnr force-pushed the feat/metal-passthrough branch 2 times, most recently from 3a63b90 to c340e6b Compare April 8, 2025 13:41
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.

Looking good, some comments

@cwfitzgerald
Copy link
Member

WebAssembly is also failing due to missing methods in wgpu/src/backend/webgpu

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.

Alright, I'll drive this over the line, thanks!

@syl20bnr syl20bnr force-pushed the feat/metal-passthrough branch from bc68444 to 080e4d8 Compare April 8, 2025 19:44
@syl20bnr syl20bnr requested a review from crowlKats as a code owner April 8, 2025 19:44
@syl20bnr syl20bnr force-pushed the feat/metal-passthrough branch 4 times, most recently from 702e61a to 0d79112 Compare April 9, 2025 12:13
@syl20bnr syl20bnr force-pushed the feat/metal-passthrough branch from 0d79112 to e33c06c Compare April 9, 2025 12:16
@cwfitzgerald cwfitzgerald enabled auto-merge (squash) April 9, 2025 14:22
@cwfitzgerald cwfitzgerald merged commit 1469047 into gfx-rs:trunk Apr 9, 2025
37 checks passed
@LegNeato
Copy link
Contributor

LegNeato commented Apr 9, 2025

@syl20bnr @cwfitzgerald Pretty sure this broke spirv passthrough: #7502

LegNeato added a commit to LegNeato/wgpu that referenced this pull request Apr 9, 2025
@syl20bnr
Copy link
Contributor Author

syl20bnr commented Apr 9, 2025

@LegNeato do you have more info ?

I tested with our SpirV compiler which used the pass-through and it seems to work fine.

Oh OK just saw your issue. Looking into it. We indeed don't use this macro.

Edit: thanks for the fix 😁

Vecvec pushed a commit to Vecvec/wgpu that referenced this pull request Apr 11, 2025
jimblandy pushed a commit to jimblandy/wgpu that referenced this pull request Apr 18, 2025
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