From 7cd7b185603e12207e141e29d93f7edbf6e014b2 Mon Sep 17 00:00:00 2001 From: Alex S Date: Sun, 20 Jun 2021 15:17:46 +0300 Subject: [PATCH] Code review: - Remove Cow from wgpu-hal API surface. You graze on in our hearts. - Rename feature flag to SPIRV_SHADER_PASSTHROUGH in anticipation of crate feature `spirv` to reduce confusion. - Add some more documentation about behaviour and intended avenues of use to the feature. --- wgpu-core/src/device/mod.rs | 6 +++--- wgpu-hal/src/lib.rs | 4 ++-- wgpu-hal/src/metal/device.rs | 2 +- wgpu-hal/src/vulkan/adapter.rs | 2 +- wgpu-hal/src/vulkan/device.rs | 2 +- wgpu-types/src/lib.rs | 6 +++++- wgpu/examples/texture-arrays/main.rs | 2 +- wgpu/src/lib.rs | 6 ++++-- wgpu/src/macros.rs | 2 +- 9 files changed, 19 insertions(+), 13 deletions(-) diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index ff3d01c0e3..d3f9ff609a 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -939,9 +939,9 @@ impl Device { &self, self_id: id::DeviceId, desc: &pipeline::ShaderModuleDescriptor<'a>, - source: Cow<'a, [u32]>, + source: &'a [u32], ) -> Result, pipeline::CreateShaderModuleError> { - self.require_features(wgt::Features::SPIRV_SHADER_MODULES)?; + self.require_features(wgt::Features::SPIRV_SHADER_PASSTHROUGH)?; let hal_desc = hal::ShaderModuleDescriptor { label: desc.label.borrow_option(), }; @@ -3546,7 +3546,7 @@ impl Global { }); }; - let shader = match device.create_shader_module_spirv(device_id, desc, source) { + let shader = match device.create_shader_module_spirv(device_id, desc, &source) { Ok(shader) => shader, Err(e) => break e, }; diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index df94c605d8..225d021b15 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -64,7 +64,7 @@ pub mod api { } use std::{ - borrow::{Borrow, Cow}, + borrow::Borrow, fmt, num::NonZeroU8, ops::{Range, RangeInclusive}, @@ -842,7 +842,7 @@ pub struct NagaShader { /// Shader input. pub enum ShaderInput<'a> { Naga(NagaShader), - SpirV(Cow<'a, [u32]>), + SpirV(&'a [u32]), } pub struct ShaderModuleDescriptor<'a> { diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index f33f72e92b..f9a371d2a4 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -635,7 +635,7 @@ impl crate::Device for super::Device { match shader { crate::ShaderInput::Naga(raw) => Ok(super::ShaderModule { raw }), crate::ShaderInput::SpirV(_) => { - unreachable!("SPIRV_SHADER_MODULES is not enabled for this backend") + unreachable!("SPIRV_SHADER_PASSTHROUGH is not enabled for this backend") } } } diff --git a/wgpu-hal/src/vulkan/adapter.rs b/wgpu-hal/src/vulkan/adapter.rs index fcd0b4855d..017d772c56 100644 --- a/wgpu-hal/src/vulkan/adapter.rs +++ b/wgpu-hal/src/vulkan/adapter.rs @@ -203,7 +203,7 @@ impl PhysicalDeviceFeatures { fn to_wgpu(&self, caps: &PhysicalDeviceCapabilities) -> (wgt::Features, wgt::DownlevelFlags) { use wgt::{DownlevelFlags as Df, Features as F}; let mut features = F::empty() - | F::SPIRV_SHADER_MODULES + | F::SPIRV_SHADER_PASSTHROUGH | F::MAPPABLE_PRIMARY_BUFFERS | F::PUSH_CONSTANTS | F::ADDRESS_MODE_CLAMP_TO_BORDER diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index 7bc408951d..0c3828b851 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -1070,7 +1070,7 @@ impl crate::Device for super::Device { ) .map_err(|e| crate::ShaderError::Compilation(format!("{}", e)))?, ), - crate::ShaderInput::SpirV(spv) => spv, + crate::ShaderInput::SpirV(spv) => Cow::Borrowed(spv), }; let vk_info = vk::ShaderModuleCreateInfo::builder() diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 1028f7fb63..df253a91df 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -596,12 +596,16 @@ bitflags::bitflags! { const CLEAR_COMMANDS = 0x0000_0200_0000_0000; /// Enables creating shader modules from SPIR-V binary data (unsafe). /// + /// SPIR-V data is not parsed or interpreted in any way; you can use + /// [`wgpu::make_spirv_raw!`] to check for alignment and magic number when converting from + /// raw bytes. + /// /// Supported platforms: /// - Vulkan, in case shader's requested capabilities and extensions agree with /// Vulkan implementation. /// /// This is a native only feature. - const SPIRV_SHADER_MODULES = 0x0000_0400_0000_0000; + const SPIRV_SHADER_PASSTHROUGH = 0x0000_0400_0000_0000; /// Features which are part of the upstream WebGPU standard. const ALL_WEBGPU = 0x0000_0000_0000_FFFF; diff --git a/wgpu/examples/texture-arrays/main.rs b/wgpu/examples/texture-arrays/main.rs index 1723c0e7ca..5bd76fd3e6 100644 --- a/wgpu/examples/texture-arrays/main.rs +++ b/wgpu/examples/texture-arrays/main.rs @@ -77,7 +77,7 @@ impl framework::Example for Example { | wgpu::Features::PUSH_CONSTANTS } fn required_features() -> wgpu::Features { - wgpu::Features::SAMPLED_TEXTURE_BINDING_ARRAY | wgpu::Features::SPIRV_SHADER_MODULES + wgpu::Features::SAMPLED_TEXTURE_BINDING_ARRAY | wgpu::Features::SPIRV_SHADER_PASSTHROUGH } fn required_limits() -> wgpu::Limits { wgpu::Limits { diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 18da77e12b..8dd0caf7fd 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -1569,8 +1569,10 @@ impl Device { /// /// # Safety /// - /// This function passes SPIR-V binary to the backend as-is and can potentially result in a - /// driver crash. + /// This function passes binary data to the backend as-is and can potentially result in a + /// driver crash or bogus behaviour. No attempt is made to ensure that data is valid SPIR-V. + /// + /// See also [`crate::include_spirv_raw!`] and [`crate::make_spirv_raw`]. pub unsafe fn create_shader_module_spirv( &self, desc: &ShaderModuleDescriptorSpirV, diff --git a/wgpu/src/macros.rs b/wgpu/src/macros.rs index 97cd2689e8..09c1b56633 100644 --- a/wgpu/src/macros.rs +++ b/wgpu/src/macros.rs @@ -57,7 +57,7 @@ macro_rules! include_spirv { }; } -/// Macro to load raw SPIR-V data statically, for use with [`wgpu::Features::SPIRV_SHADER_MODULES`]. +/// Macro to load raw SPIR-V data statically, for use with [`wgpu::Features::SPIRV_SHADER_PASSTHROUGH`]. /// /// It ensures the word alignment as well as the magic number. #[macro_export]