Address review

* Add note that nesting structs for the inter-stage interface can't
  happen.
* Remove new TODO notes (some addressed and some transferred to an issue
  gfx-rs#5577)
* Changed issue that regression test refers to 3748 -> 5553
* Add debug_assert that binding.is_some() in hlsl writer

naga/src/back/hlsl/writer.rs

@@ -25,8 +25,8 @@ pub(crate) const INSERT_BITS_FUNCTION: &str = "naga_insertBits";
 struct EpStructMember {
  name: String,
  ty: Handle<crate::Type>,
- // TODO: log error if binding is none?
  // technically, this should always be `Some`
+ // (we `debug_assert!` this in `write_interface_struct`)
  binding: Option<crate::Binding>,
  index: u32,
 }
@@ -481,6 +481,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;
  }
@@ -540,10 +544,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;
@@ -600,19 +606,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<crate::Binding>| {
- 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<crate::Binding>| 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);
  }
@@ -635,9 +637,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
  continue;
  }
  }
- Some(crate::Binding::BuiltIn(_)) => {}
- // Log error?
- None => {}
+ Some(crate::Binding::BuiltIn(_)) | None => {}
  }
  }
 

tests/tests/regression/issue_3748.rs → tests/tests/regression/issue_5553.rs

@@ -8,7 +8,7 @@ use wgpu::*;
 /// 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 {

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;

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,

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<super::Api>,
  ) -> Result<super::ComputePipeline, crate::PipelineError> {
- 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");