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

Unchecked shader creation is not honored on metal #3136

Closed
raphlinus opened this issue Oct 22, 2022 · 4 comments
Closed

Unchecked shader creation is not honored on metal #3136

raphlinus opened this issue Oct 22, 2022 · 4 comments
Labels
api: metal Issues with Metal area: validation Issues related to validation, diagnostics, and error handling help required We need community help to make this happen. type: bug Something isn't working

Comments

@raphlinus
Copy link
Contributor

Description
Even when shaders are created with create_shader_module_unchecked, bounds checks are still in place in the Metal backend.

Repro steps

  1. Clone https://github.com/googlefonts/compute-shader-101, go into compute-shader-hello
  2. Change the body of shader.wgsl to:
@group(0) @binding(0)
var<storage, read_write> data: array<atomic<u32>>;

@compute @workgroup_size(1)
fn main(@builtin(global_invocation_id) global_id: vec3<u32>) {
    atomicExchange(&data[global_id.x], 1u);
}
  1. Change the shader creation in main.rs to:
    let cs_module = unsafe { device.create_shader_module_unchecked(wgpu::ShaderModuleDescriptor {
        label: None,
        source: wgpu::ShaderSource::Wgsl(include_str!("shader.wgsl").into()),
    })};
  1. Do cargo run

Note: this is a repro of gfx-rs/naga#1848. It was suggested to use the unchecked variant of shader creation, but that workaround is blocked by this bug.

Expected vs observed behavior

thread 'main' panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_compute_pipeline
    Internal error: Metal: program_source:27:48: error: cannot take the address of an rvalue of type 'metal::uint' (aka 'unsigned int')
    uint _e5 = metal::atomic_exchange_explicit(&uint(_e2) < 1 + (_buffer_sizes.size0 - 0 - 4) / 4 ? data[_e2] : DefaultConstructible(), 1u, metal::memory_order_relaxed);
                                               ^~~~~~~~~~


', /Users/raph/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-0.14.0/src/backend/direct.rs:2403:5

Extra materials
I did a little digging, and it seems like there is logic in the vulkan back-end to plumb runtime_checks == false to the appropriate naga BoundsCheckPolicy values, but that appears to be missing in the metal back-end; the corresponding code seems to pass naga_options to the writer directly. Perhaps a better fix would be setting naga_options upstream rather than having to do it separately each back-end.

Platform
macOS 12.6, wgpu 0.14 and HEAD.

@raphlinus
Copy link
Contributor Author

Also note, the naga cli behaves as expected: when the bounds check policy is not explicitly set, the shader is generated with no bounds check.

@cwfitzgerald cwfitzgerald added type: bug Something isn't working help required We need community help to make this happen. area: validation Issues related to validation, diagnostics, and error handling api: metal Issues with Metal labels Oct 23, 2022
@jinleili
Copy link
Contributor

This issue has disappeared on master.

I think create_shader_module_unchecked is just telling naga to uncheck, and the error above comes from Metal's internal error message when calling newLibraryWithSource:

let library = self
.shared
.device
.lock()
.new_library_with_source(source.as_ref(), &options)
.map_err(|err| {
log::warn!("Naga generated shader:\n{}", source);
crate::PipelineError::Linkage(stage_bit, format!("Metal: {}", err))
})?;

This validatiion error is unavoidable when the naga translated code does not conform to the MSL spec.

@raphlinus
Copy link
Contributor Author

The translated code would not have violated the MSL spec had the unchecked request been honored.

@FL33TW00D
Copy link
Contributor

Fixed and released: #3603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: metal Issues with Metal area: validation Issues related to validation, diagnostics, and error handling help required We need community help to make this happen. type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants