From 76814a83a20391b690ecf4e4a455d8a5768ac3be Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Tue, 18 Jan 2022 15:17:59 -0500 Subject: [PATCH] msl: inject point size if needed --- src/back/msl/mod.rs | 10 +--- src/back/msl/writer.rs | 94 +++++++++++++++++++++++++----------- tests/in/interface.param.ron | 3 ++ tests/out/msl/interface.msl | 3 +- tests/snapshots.rs | 11 +++-- 5 files changed, 78 insertions(+), 43 deletions(-) diff --git a/src/back/msl/mod.rs b/src/back/msl/mod.rs index 4295ae0e95..311f5e3741 100644 --- a/src/back/msl/mod.rs +++ b/src/back/msl/mod.rs @@ -196,7 +196,7 @@ impl Default for Options { } // A subset of options that are meant to be changed per pipeline. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Default, Clone, PartialEq, Eq, Hash)] #[cfg_attr(feature = "serialize", derive(serde::Serialize))] #[cfg_attr(feature = "deserialize", derive(serde::Deserialize))] pub struct PipelineOptions { @@ -205,14 +205,6 @@ pub struct PipelineOptions { pub allow_point_size: bool, } -impl Default for PipelineOptions { - fn default() -> Self { - PipelineOptions { - allow_point_size: true, - } - } -} - impl Options { fn resolve_local_binding( &self, diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index 2af88e2407..0e2f53a645 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -46,10 +46,7 @@ impl<'a> Display for TypeContext<'a> { match kind { // work around Metal toolchain bug with `uint` typedef crate::ScalarKind::Uint => write!(out, "{}::uint", NAMESPACE), - _ => { - let kind_str = kind.to_msl_name(); - write!(out, "{}", kind_str) - } + _ => write!(out, "{}", kind.to_msl_name()), } } crate::TypeInner::Atomic { kind, .. } => { @@ -96,7 +93,11 @@ impl<'a> Display for TypeContext<'a> { Some(name) => name, None => return Ok(()), }; - write!(out, "{} {}&", class_name, kind.to_msl_name(),) + write!(out, "{} ", class_name)?; + if kind == crate::ScalarKind::Uint { + write!(out, "{}::", NAMESPACE)?; + } + write!(out, "{}&", kind.to_msl_name()) } crate::TypeInner::ValuePointer { size: Some(size), @@ -1708,20 +1709,25 @@ impl Writer { self.put_expression(expr_handle, context, true)?; writeln!(self.out, ";")?; write!(self.out, "{}return {} {{", level, struct_name)?; + let mut is_first = true; + let mut has_point_size = false; + for (index, member) in members.iter().enumerate() { - if !context.pipeline_options.allow_point_size - && member.binding - == Some(crate::Binding::BuiltIn(crate::BuiltIn::PointSize)) - { - continue; - } - if member.binding - == Some(crate::Binding::BuiltIn(crate::BuiltIn::CullDistance)) - { - log::warn!("Ignoring CullDistance BuiltIn"); - continue; + match member.binding { + Some(crate::Binding::BuiltIn(crate::BuiltIn::PointSize)) => { + has_point_size = true; + if !context.pipeline_options.allow_point_size { + continue; + } + } + Some(crate::Binding::BuiltIn(crate::BuiltIn::CullDistance)) => { + log::warn!("Ignoring CullDistance built-in"); + continue; + } + _ => {} } + let comma = if is_first { "" } else { "," }; is_first = false; let name = &self.names[&NameKey::StructMember(result_ty, index as u32)]; @@ -1752,6 +1758,17 @@ impl Writer { write!(self.out, "{} {}.{}", comma, tmp, name)?; } } + + if let FunctionOrigin::EntryPoint(ep_index) = context.origin { + let stage = context.module.entry_points[ep_index as usize].stage; + if context.pipeline_options.allow_point_size + && stage == crate::ShaderStage::Vertex + && !has_point_size + { + // point size was injected and comes last + write!(self.out, ", 1.0")?; + } + } } _ => { write!(self.out, "{}return {} {{ ", level, struct_name)?; @@ -2874,6 +2891,7 @@ impl Writer { } writeln!(self.out, "struct {} {{", stage_out_name)?; + let mut has_point_size = false; for (name, ty, binding) in result_members { let ty_name = TypeContext { handle: ty, @@ -2883,19 +2901,27 @@ impl Writer { first_time: true, }; let binding = binding.ok_or(Error::Validation)?; - // Cull Distance is not supported in Metal. - // But we can't return UnsupportedBuiltIn error to user. - // Because otherwise we can't generate msl shader from any glslang SPIR-V shaders. - // glslang generates gl_PerVertex struct with gl_CullDistance builtin inside by default. - if *binding == crate::Binding::BuiltIn(crate::BuiltIn::CullDistance) { - log::warn!("Ignoring CullDistance BuiltIn"); - continue; - } - if !pipeline_options.allow_point_size - && *binding == crate::Binding::BuiltIn(crate::BuiltIn::PointSize) - { - continue; + + match *binding { + // Point size is only supported in VS of pipelines with + // point primitive topology. + crate::Binding::BuiltIn(crate::BuiltIn::PointSize) => { + has_point_size = true; + if !pipeline_options.allow_point_size { + continue; + } + } + // Cull Distance is not supported in Metal. + // But we can't return UnsupportedBuiltIn error to user. + // Because otherwise we can't generate msl shader from any glslang SPIR-V shaders. + // glslang generates gl_PerVertex struct with gl_CullDistance builtin inside by default. + crate::Binding::BuiltIn(crate::BuiltIn::CullDistance) => { + log::warn!("Ignoring CullDistance BuiltIn"); + continue; + } + _ => {} } + let array_len = match module.types[ty].inner { crate::TypeInner::Array { size: crate::ArraySize::Constant(handle), @@ -2911,6 +2937,18 @@ impl Writer { } writeln!(self.out, ";")?; } + + if pipeline_options.allow_point_size + && ep.stage == crate::ShaderStage::Vertex + && !has_point_size + { + // inject the point size output last + writeln!( + self.out, + "{}float _point_size [[point_size]];", + back::INDENT + )?; + } writeln!(self.out, "}};")?; &stage_out_name } diff --git a/tests/in/interface.param.ron b/tests/in/interface.param.ron index e2f2b32d5b..9cf3110232 100644 --- a/tests/in/interface.param.ron +++ b/tests/in/interface.param.ron @@ -16,4 +16,7 @@ wgsl: ( explicit_types: true, ), + msl_pipeline: ( + allow_point_size: true, + ), ) diff --git a/tests/out/msl/interface.msl b/tests/out/msl/interface.msl index 9dca449a69..f95620c375 100644 --- a/tests/out/msl/interface.msl +++ b/tests/out/msl/interface.msl @@ -21,6 +21,7 @@ struct vertex_Input { struct vertex_Output { metal::float4 position [[position]]; float varying [[user(loc1), center_perspective]]; + float _point_size [[point_size]]; }; vertex vertex_Output vertex_( vertex_Input varyings [[stage_in]] @@ -30,7 +31,7 @@ vertex vertex_Output vertex_( const auto color = varyings.color; metal::uint tmp = (vertex_index + instance_index) + color; const auto _tmp = VertexOutput {metal::float4(1.0), static_cast(tmp)}; - return vertex_Output { _tmp.position, _tmp.varying }; + return vertex_Output { _tmp.position, _tmp.varying, 1.0 }; } diff --git a/tests/snapshots.rs b/tests/snapshots.rs index dd3bd0ccc0..56378e2ac1 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -63,6 +63,9 @@ struct Parameters { #[cfg(all(feature = "deserialize", feature = "msl-out"))] #[serde(default)] msl: naga::back::msl::Options, + #[cfg(all(feature = "deserialize", feature = "msl-out"))] + #[serde(default)] + msl_pipeline: naga::back::msl::PipelineOptions, #[cfg(all(feature = "deserialize", feature = "glsl-out"))] #[serde(default)] glsl: naga::back::glsl::Options, @@ -135,6 +138,7 @@ fn check_targets(module: &naga::Module, name: &str, targets: Targets) { &dest, name, ¶ms.msl, + ¶ms.msl_pipeline, params.bounds_check_policies, ); } @@ -235,20 +239,17 @@ fn write_output_msl( destination: &PathBuf, file_name: &str, options: &naga::back::msl::Options, + pipeline_options: &naga::back::msl::PipelineOptions, bounds_check_policies: naga::proc::BoundsCheckPolicies, ) { use naga::back::msl; println!("writing MSL"); - let pipeline_options = msl::PipelineOptions { - allow_point_size: true, - }; - let mut options = options.clone(); options.bounds_check_policies = bounds_check_policies; let (string, tr_info) = - msl::write_string(module, info, &options, &pipeline_options).expect("Metal write failed"); + msl::write_string(module, info, &options, pipeline_options).expect("Metal write failed"); for (ep, result) in module.entry_points.iter().zip(tr_info.entry_point_names) { if let Err(error) = result {