diff --git a/naga/src/back/hlsl/mod.rs b/naga/src/back/hlsl/mod.rs index 5cbfffc474..49ff07ebf2 100644 --- a/naga/src/back/hlsl/mod.rs +++ b/naga/src/back/hlsl/mod.rs @@ -290,7 +290,7 @@ impl Wrapped { /// A fragment entry point to be considered when generating HLSL for the output interface of vertex /// entry points. /// -/// This is provided as an optional paramter to [`Writer::write`]. +/// This is provided as an optional parameter to [`Writer::write`]. /// /// If this is provided, vertex outputs will be removed if they are not inputs of this fragment /// entry point. This is necessary for generating correct HLSL when some of the vertex shader diff --git a/naga/src/back/hlsl/writer.rs b/naga/src/back/hlsl/writer.rs index 5289002a15..3fb201fdf5 100644 --- a/naga/src/back/hlsl/writer.rs +++ b/naga/src/back/hlsl/writer.rs @@ -28,8 +28,8 @@ pub(crate) const INSERT_BITS_FUNCTION: &str = "naga_insertBits"; struct EpStructMember { name: String, ty: Handle, - // TODO: log error if binding is none? // technically, this should always be `Some` + // (we `debug_assert!` this in `write_interface_struct`) binding: Option, index: u32, } @@ -485,6 +485,10 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { write!(self.out, "struct {struct_name}")?; writeln!(self.out, " {{")?; for m in members.iter() { + // Sanity check that each IO member is a built-in or is assigned a + // location. Also see note about nesting in `write_ep_input_struct`. + debug_assert!(m.binding.is_some()); + if is_subgroup_builtin_binding(&m.binding) { continue; } @@ -544,10 +548,12 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { let mut fake_members = Vec::new(); for arg in func.arguments.iter() { + // NOTE: We don't need to handle nesting structs. All members must + // be either built-ins or assigned a location. I.E. `binding` is + // `Some`. This is checked in `VaryingContext::validate`. See: + // https://gpuweb.github.io/gpuweb/wgsl/#input-output-locations match module.types[arg.ty].inner { TypeInner::Struct { ref members, .. } => { - // TODO: what about nested structs? Is that possible? Maybe try an unwrap on - // the binding? for member in members.iter() { let name = self.namer.call_or(&member.name, "member"); let index = fake_members.len() as u32; @@ -604,19 +610,15 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { let fs_input_locs = if let (Some(frag_ep), ShaderStage::Vertex) = (frag_ep, stage) { let mut fs_input_locs = Vec::new(); for arg in frag_ep.func.arguments.iter() { - let mut push_if_location = |binding: &Option| { - match *binding { - Some(crate::Binding::Location { location, .. }) => { - fs_input_locs.push(location) - } - Some(crate::Binding::BuiltIn(_)) => {} - // Log error? - None => {} - } + let mut push_if_location = |binding: &Option| match *binding { + Some(crate::Binding::Location { location, .. }) => fs_input_locs.push(location), + Some(crate::Binding::BuiltIn(_)) | None => {} }; + + // NOTE: We don't need to handle struct nesting. See note in + // `write_ep_input_struct`. match frag_ep.module.types[arg.ty].inner { TypeInner::Struct { ref members, .. } => { - // TODO: nesting? for member in members.iter() { push_if_location(&member.binding); } @@ -639,9 +641,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { continue; } } - Some(crate::Binding::BuiltIn(_)) => {} - // Log error? - None => {} + Some(crate::Binding::BuiltIn(_)) | None => {} } } diff --git a/naga/tests/snapshots.rs b/naga/tests/snapshots.rs index e98d368c03..3dbe95c9e9 100644 --- a/naga/tests/snapshots.rs +++ b/naga/tests/snapshots.rs @@ -259,6 +259,11 @@ impl Input { } } +#[cfg(feature = "hlsl-out")] +type FragmentEntryPoint<'a> = naga::back::hlsl::FragmentEntryPoint<'a>; +#[cfg(not(feature = "hlsl-out"))] +type FragmentEntryPoint<'a> = (); + #[allow(unused_variables)] fn check_targets( input: &Input, @@ -266,7 +271,7 @@ fn check_targets( targets: Targets, source_code: Option<&str>, // For testing hlsl generation when fragment shader doesn't consume all vertex outputs. - frag_ep: Option, + frag_ep: Option, ) { if frag_ep.is_some() && !targets.contains(Targets::HLSL) { panic!("Providing FragmentEntryPoint only makes sense when testing hlsl-out"); @@ -932,7 +937,7 @@ fn convert_wgsl() { } } -#[cfg(feature = "wgsl-in")] +#[cfg(all(feature = "wgsl-in", feature = "hlsl-out"))] #[test] fn unconsumed_vertex_outputs_hlsl_out() { let load_and_parse = |name| { diff --git a/tests/tests/regression/issue_3748.rs b/tests/tests/regression/issue_5553.rs similarity index 94% rename from tests/tests/regression/issue_3748.rs rename to tests/tests/regression/issue_5553.rs index c38235021e..b15d7dfb35 100644 --- a/tests/tests/regression/issue_3748.rs +++ b/tests/tests/regression/issue_5553.rs @@ -3,18 +3,18 @@ use wgpu_test::{gpu_test, GpuTestConfiguration}; use wgpu::*; /// Previously, for every user-defined vertex output a fragment shader had to have a corresponding -/// user-defined input. This would generate `StageError::InputNotComsumed`. +/// user-defined input. This would generate `StageError::InputNotConsumed`. /// /// This requirement was removed from the WebGPU spec. Now, when generating hlsl, wgpu will /// automatically remove any user-defined outputs from the vertex shader that are not present in /// the fragment inputs. This is necessary for generating correct hlsl: -/// https://github.com/gfx-rs/naga/issues/1945 +/// https://github.com/gfx-rs/wgpu/issues/5553 #[gpu_test] static ALLOW_INPUT_NOT_CONSUMED: GpuTestConfiguration = GpuTestConfiguration::new().run_async(|ctx| async move { let module = ctx .device - .create_shader_module(include_wgsl!("issue_3748.wgsl")); + .create_shader_module(include_wgsl!("issue_5553.wgsl")); let pipeline_layout = ctx .device diff --git a/tests/tests/regression/issue_3748.wgsl b/tests/tests/regression/issue_5553.wgsl similarity index 100% rename from tests/tests/regression/issue_3748.wgsl rename to tests/tests/regression/issue_5553.wgsl diff --git a/tests/tests/root.rs b/tests/tests/root.rs index 91a3542c1b..75510a032b 100644 --- a/tests/tests/root.rs +++ b/tests/tests/root.rs @@ -1,9 +1,9 @@ mod regression { mod issue_3349; mod issue_3457; - mod issue_3748; mod issue_4024; mod issue_4122; + mod issue_5553; } mod bgra8unorm_storage; diff --git a/wgpu-core/src/validation.rs b/wgpu-core/src/validation.rs index 79e023283c..f7adb806ea 100644 --- a/wgpu-core/src/validation.rs +++ b/wgpu-core/src/validation.rs @@ -1189,11 +1189,6 @@ impl Interface { )); } ( - // TODO: is a subtype allowed here? This isn't clear - // from the line in the spec: "For each user-defined - // input of descriptor.fragment there must be a - // user-defined output of descriptor.vertex that - // location, type, and interpolation of the input." iv.ty.is_subtype_of(&provided.ty), iv.ty.dim.num_components(), ) @@ -1219,23 +1214,14 @@ impl Interface { } } } - // TODO: front_facing, sample_index, and sample_mask builtin's should all increase - // components count for fragment input. Varying::BuiltIn(_) => {} } } if shader_stage == naga::ShaderStage::Vertex { - // TODO: if topology is point we should add 1 to inter_stage_components? for output in entry_point.outputs.iter() { //TODO: count builtins towards the limit? inter_stage_components += match *output { - // TODO: Spec mentions "Each user-defined output of descriptor.vertex consumes - // 4 scalar components". Not that it varies based on the type. So is there an - // inconsistency here? Also are all these "user-defined" or is that unknown at - // this stage? - // https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-inter-stage-interfaces - // (same applies to counting components for fragment inputs) Varying::Local { ref iv, .. } => iv.ty.dim.num_components(), Varying::BuiltIn(_) => 0, }; @@ -1257,9 +1243,6 @@ impl Interface { } } - // TODO: spec also has a max_inter_stage_shader_variables - // https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-inter-stage-interfaces and - // the location of user defined outputs(vertex)/inputs(fragment) must all be less than this if inter_stage_components > self.limits.max_inter_stage_shader_components { return Err(StageError::TooManyVaryings { used: inter_stage_components, diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index c37289f474..d416304195 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -223,9 +223,10 @@ impl super::Device { let frag_ep = fragment_stage .map(|fs_stage| { - hlsl::FragmentEntryPoint::new(&fs_stage.module.naga.module, fs_stage.entry_point).ok_or( - crate::PipelineError::EntryPoint(naga::ShaderStage::Fragment), - ) + hlsl::FragmentEntryPoint::new(&fs_stage.module.naga.module, fs_stage.entry_point) + .ok_or(crate::PipelineError::EntryPoint( + naga::ShaderStage::Fragment, + )) }) .transpose()?; @@ -1500,7 +1501,8 @@ impl crate::Device for super::Device { &self, desc: &crate::ComputePipelineDescriptor, ) -> Result { - let blob_cs = self.load_shader(&desc.stage, desc.layout, naga::ShaderStage::Compute, None)?; + let blob_cs = + self.load_shader(&desc.stage, desc.layout, naga::ShaderStage::Compute, None)?; let pair = { profiling::scope!("ID3D12Device::CreateComputePipelineState");