diff --git a/src/arena.rs b/src/arena.rs index 1e7b659371..5c2681c834 100644 --- a/src/arena.rs +++ b/src/arena.rs @@ -590,6 +590,19 @@ impl UniqueArena { Handle::from_usize(index) } + /// Replace an old value with a new value. + /// + /// # Panics + /// + /// - if the old value is not in the arena + /// - if the new value already exists in the arena + pub fn replace(&mut self, old: Handle, new: T) { + let (index, added) = self.set.insert_full(new); + assert!(added && index == self.set.len() - 1); + + self.set.swap_remove_index(old.index()).unwrap(); + } + /// Return this arena's handle for `value`, if present. /// /// If this arena already contains an element equal to `value`, diff --git a/src/back/glsl/features.rs b/src/back/glsl/features.rs index ca292364fe..8392969b56 100644 --- a/src/back/glsl/features.rs +++ b/src/back/glsl/features.rs @@ -99,9 +99,8 @@ impl FeaturesManager { check_feature!(CONSERVATIVE_DEPTH, 130, 300); check_feature!(NOPERSPECTIVE_QUALIFIER, 130); check_feature!(SAMPLE_QUALIFIER, 400, 320); - // gl_ClipDistance is supported by core versions > 1.3 and aren't supported by an es versions without extensions - check_feature!(CLIP_DISTANCE, 130, 300); - check_feature!(CULL_DISTANCE, 450, 300); + check_feature!(CLIP_DISTANCE, 130, 300 /* with extension */); + check_feature!(CULL_DISTANCE, 450, 300 /* with extension */); check_feature!(SAMPLE_VARIABLES, 400, 300); check_feature!(DYNAMIC_ARRAY_SIZE, 430, 310); match version { @@ -197,9 +196,8 @@ impl FeaturesManager { if (self.0.contains(Features::CLIP_DISTANCE) || self.0.contains(Features::CULL_DISTANCE)) && version.is_es() { - // TODO: handle gl_ClipDistance and gl_CullDistance usage in better way // https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_clip_cull_distance.txt - // writeln!(out, "#extension GL_EXT_clip_cull_distance : require")?; + writeln!(out, "#extension GL_EXT_clip_cull_distance : require")?; } if self.0.contains(Features::SAMPLE_VARIABLES) && version.is_es() { diff --git a/src/back/glsl/mod.rs b/src/back/glsl/mod.rs index dbcda3ada5..467d680f9a 100644 --- a/src/back/glsl/mod.rs +++ b/src/back/glsl/mod.rs @@ -2029,27 +2029,11 @@ impl<'a, W: Write> Writer<'a, W> { }; for (index, member) in members.iter().enumerate() { - // TODO: handle builtin in better way - if let Some(crate::Binding::BuiltIn(builtin)) = - member.binding + if let Some(crate::Binding::BuiltIn( + crate::BuiltIn::PointSize, + )) = member.binding { - has_point_size |= builtin == crate::BuiltIn::PointSize; - - match builtin { - crate::BuiltIn::ClipDistance - | crate::BuiltIn::CullDistance => { - if self.options.version.is_es() { - // Note that gl_ClipDistance and gl_CullDistance are listed in the GLSL ES 3.2 spec but shouldn't - // See https://github.com/KhronosGroup/GLSL/issues/132#issuecomment-685818465 - log::warn!( - "{:?} is not part of GLSL ES", - builtin - ); - continue; - } - } - _ => {} - } + has_point_size = true; } let varying_name = VaryingName { diff --git a/src/back/hlsl/conv.rs b/src/back/hlsl/conv.rs index 1773ed71bc..5eb24962f6 100644 --- a/src/back/hlsl/conv.rs +++ b/src/back/hlsl/conv.rs @@ -155,9 +155,6 @@ impl crate::BuiltIn { Self::ClipDistance => "SV_ClipDistance", Self::CullDistance => "SV_CullDistance", Self::InstanceIndex => "SV_InstanceID", - // based on this page https://docs.microsoft.com/en-us/windows/uwp/gaming/glsl-to-hlsl-reference#comparing-opengl-es-20-with-direct3d-11 - // No meaning unless you target Direct3D 9 - Self::PointSize => "PSIZE", Self::VertexIndex => "SV_VertexID", // fragment Self::FragDepth => "SV_Depth", @@ -177,7 +174,7 @@ impl crate::BuiltIn { Self::BaseInstance | Self::BaseVertex | Self::WorkGroupSize => { return Err(Error::Unimplemented(format!("builtin {self:?}"))) } - Self::ViewIndex | Self::PointCoord => { + Self::PointSize | Self::ViewIndex | Self::PointCoord => { return Err(Error::Custom(format!("Unsupported builtin {self:?}"))) } }) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index fb4b0929cb..bac178729f 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -2233,18 +2233,13 @@ impl Writer { let mut is_first = true; for (index, member) in members.iter().enumerate() { - 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"); + if let Some(crate::Binding::BuiltIn(crate::BuiltIn::PointSize)) = + member.binding + { + has_point_size = true; + if !context.pipeline_options.allow_point_size { continue; } - _ => {} } let comma = if is_first { "" } else { "," }; @@ -3563,24 +3558,11 @@ impl Writer { }; let binding = binding.ok_or(Error::Validation)?; - 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"); + if let crate::Binding::BuiltIn(crate::BuiltIn::PointSize) = *binding { + has_point_size = true; + if !pipeline_options.allow_point_size { continue; } - _ => {} } let array_len = match module.types[ty].inner { diff --git a/src/back/wgsl/writer.rs b/src/back/wgsl/writer.rs index 828cd79b0d..05f5b10943 100644 --- a/src/back/wgsl/writer.rs +++ b/src/back/wgsl/writer.rs @@ -335,11 +335,8 @@ impl Writer { match *attribute { Attribute::Location(id) => write!(self.out, "@location({id}) ")?, Attribute::BuiltIn(builtin_attrib) => { - if let Some(builtin) = builtin_str(builtin_attrib) { - write!(self.out, "@builtin({builtin}) ")?; - } else { - log::warn!("Unsupported builtin attribute: {:?}", builtin_attrib); - } + let builtin = builtin_str(builtin_attrib)?; + write!(self.out, "@builtin({builtin}) ")?; } Attribute::Stage(shader_stage) => { let stage_str = match shader_stage { @@ -401,14 +398,6 @@ impl Writer { write!(self.out, " {{")?; writeln!(self.out)?; for (index, member) in members.iter().enumerate() { - // Skip struct member with unsupported built in - if let Some(crate::Binding::BuiltIn(built_in)) = member.binding { - if builtin_str(built_in).is_none() { - log::warn!("Skip member with unsupported builtin {:?}", built_in); - continue; - } - } - // The indentation is only for readability write!(self.out, "{}", back::INDENT)?; if let Some(ref binding) = member.binding { @@ -669,22 +658,6 @@ impl Writer { _ => false, }; if min_ref_count <= info.ref_count || required_baking_expr { - // If expression contains unsupported builtin we should skip it - if let Expression::Load { pointer } = func_ctx.expressions[handle] { - if let Expression::AccessIndex { base, index } = - func_ctx.expressions[pointer] - { - if access_to_unsupported_builtin( - base, - index, - module, - func_ctx.info, - ) { - return Ok(()); - } - } - } - Some(format!("{}{}", back::BAKE_PREFIX, handle.index())) } else { None @@ -746,14 +719,6 @@ impl Writer { writeln!(self.out, "discard;")? } Statement::Store { pointer, value } => { - // WGSL does not support all SPIR-V builtins and we should skip it in generated shaders. - // We already skip them when we generate struct type. - // Now we need to find expression that used struct with ignored builtins - if let Expression::AccessIndex { base, index } = func_ctx.expressions[pointer] { - if access_to_unsupported_builtin(base, index, module, func_ctx.info) { - return Ok(()); - } - } write!(self.out, "{level}")?; let is_atomic = match *func_ctx.info[pointer].ty.inner_with(&module.types) { @@ -1150,42 +1115,10 @@ impl Writer { Expression::Compose { ty, ref components } => { self.write_type(module, ty)?; write!(self.out, "(")?; - // !spv-in specific notes! - // WGSL does not support all SPIR-V builtins and we should skip it in generated shaders. - // We already skip them when we generate struct type. - // Now we need to find components that used struct with ignored builtins. - - // So, why we can't just return the error to a user? - // We can, but otherwise, we can't generate WGSL shader from any glslang SPIR-V shaders. - // glslang generates gl_PerVertex struct with gl_CullDistance, gl_ClipDistance and gl_PointSize builtin inside by default. - // All of them are not supported by WGSL. - - // We need to copy components to another vec because we don't know which of them we should write. - let mut components_to_write = Vec::with_capacity(components.len()); - for component in components { - let mut skip_component = false; - if let Expression::Load { pointer } = func_ctx.expressions[*component] { - if let Expression::AccessIndex { base, index } = - func_ctx.expressions[pointer] - { - if access_to_unsupported_builtin(base, index, module, func_ctx.info) { - skip_component = true; - } - } - } - if skip_component { - continue; - } else { - components_to_write.push(*component); - } - } - - // non spv-in specific notes! - // Real `Expression::Compose` logic generates here. - for (index, component) in components_to_write.iter().enumerate() { + for (index, component) in components.iter().enumerate() { self.write_expr(module, *component, func_ctx)?; // Only write a comma if isn't the last element - if index != components_to_write.len().saturating_sub(1) { + if index != components.len().saturating_sub(1) { // The leading space is for readability only write!(self.out, ", ")?; } @@ -1764,25 +1697,8 @@ impl Writer { self.write_type(module, ty)?; write!(self.out, "(")?; - let members = match module.types[ty].inner { - TypeInner::Struct { ref members, .. } => Some(members), - _ => None, - }; - // Write the comma separated constants for (index, constant) in components.iter().enumerate() { - if let Some(&crate::Binding::BuiltIn(built_in)) = - members.and_then(|members| members.get(index)?.binding.as_ref()) - { - if builtin_str(built_in).is_none() { - log::warn!( - "Skip constant for struct member with unsupported builtin {:?}", - built_in - ); - continue; - } - } - self.write_constant(module, *constant)?; // Only write a comma if isn't the last element if index != components.len().saturating_sub(1) { @@ -1869,26 +1785,34 @@ impl Writer { } } -const fn builtin_str(built_in: crate::BuiltIn) -> Option<&'static str> { +fn builtin_str(built_in: crate::BuiltIn) -> Result<&'static str, Error> { use crate::BuiltIn as Bi; - match built_in { - Bi::VertexIndex => Some("vertex_index"), - Bi::InstanceIndex => Some("instance_index"), - Bi::Position { .. } => Some("position"), - Bi::FrontFacing => Some("front_facing"), - Bi::FragDepth => Some("frag_depth"), - Bi::LocalInvocationId => Some("local_invocation_id"), - Bi::LocalInvocationIndex => Some("local_invocation_index"), - Bi::GlobalInvocationId => Some("global_invocation_id"), - Bi::WorkGroupId => Some("workgroup_id"), - Bi::NumWorkGroups => Some("num_workgroups"), - Bi::SampleIndex => Some("sample_index"), - Bi::SampleMask => Some("sample_mask"), - Bi::PrimitiveIndex => Some("primitive_index"), - Bi::ViewIndex => Some("view_index"), - _ => None, - } + Ok(match built_in { + Bi::VertexIndex => "vertex_index", + Bi::InstanceIndex => "instance_index", + Bi::Position { .. } => "position", + Bi::FrontFacing => "front_facing", + Bi::FragDepth => "frag_depth", + Bi::LocalInvocationId => "local_invocation_id", + Bi::LocalInvocationIndex => "local_invocation_index", + Bi::GlobalInvocationId => "global_invocation_id", + Bi::WorkGroupId => "workgroup_id", + Bi::NumWorkGroups => "num_workgroups", + Bi::SampleIndex => "sample_index", + Bi::SampleMask => "sample_mask", + Bi::PrimitiveIndex => "primitive_index", + Bi::ViewIndex => "view_index", + Bi::BaseInstance + | Bi::BaseVertex + | Bi::ClipDistance + | Bi::CullDistance + | Bi::PointSize + | Bi::PointCoord + | Bi::WorkGroupSize => { + return Err(Error::Custom(format!("Unsupported builtin {built_in:?}"))) + } + }) } const fn image_dimension_str(dim: crate::ImageDimension) -> &'static str { @@ -2032,31 +1956,3 @@ fn map_binding_to_attribute( }, } } - -/// Helper function that check that expression don't access to structure member with unsupported builtin. -fn access_to_unsupported_builtin( - expr: Handle, - index: u32, - module: &Module, - info: &valid::FunctionInfo, -) -> bool { - let base_ty_res = &info[expr].ty; - let resolved = base_ty_res.inner_with(&module.types); - if let TypeInner::Pointer { - base: pointer_base_handle, - .. - } = *resolved - { - // Let's check that we try to access a struct member with unsupported built-in and skip it. - if let TypeInner::Struct { ref members, .. } = module.types[pointer_base_handle].inner { - if let Some(crate::Binding::BuiltIn(built_in)) = members[index as usize].binding { - if builtin_str(built_in).is_none() { - log::warn!("Skip component with unsupported builtin {:?}", built_in); - return true; - } - } - } - } - - false -} diff --git a/src/front/glsl/variables.rs b/src/front/glsl/variables.rs index 73fcb6e5e6..4c0e0a927d 100644 --- a/src/front/glsl/variables.rs +++ b/src/front/glsl/variables.rs @@ -200,8 +200,8 @@ impl Frontend { stride: 4, }, builtin: match name { - "gl_ClipDistance" => BuiltIn::PointSize, - "gl_CullDistance" => BuiltIn::FragDepth, + "gl_ClipDistance" => BuiltIn::ClipDistance, + "gl_CullDistance" => BuiltIn::CullDistance, _ => unreachable!(), }, mutable: self.meta.stage == ShaderStage::Vertex, diff --git a/src/front/spv/function.rs b/src/front/spv/function.rs index c26c61e1e9..5dc781504e 100644 --- a/src/front/spv/function.rs +++ b/src/front/spv/function.rs @@ -370,22 +370,50 @@ impl> super::Frontend { let expr_handle = function .expressions .append(crate::Expression::GlobalVariable(lvar.handle), span); + + // Cull problematic builtins of gl_PerVertex. + // See the docs for `Frontend::gl_per_vertex_builtin_access`. + { + let ty = &module.types[result.ty]; + match ty.inner { + crate::TypeInner::Struct { + members: ref original_members, + span, + } if ty.name.as_deref() == Some("gl_PerVertex") => { + let mut new_members = original_members.clone(); + for member in &mut new_members { + if let Some(crate::Binding::BuiltIn(built_in)) = member.binding + { + if !self.gl_per_vertex_builtin_access.contains(&built_in) { + member.binding = None + } + } + } + if &new_members != original_members { + module.types.replace( + result.ty, + crate::Type { + name: ty.name.clone(), + inner: crate::TypeInner::Struct { + members: new_members, + span, + }, + }, + ); + } + } + _ => {} + } + } + match module.types[result.ty].inner { crate::TypeInner::Struct { members: ref sub_members, .. } => { for (index, sm) in sub_members.iter().enumerate() { - match sm.binding { - Some(crate::Binding::BuiltIn(built_in)) => { - // Cull unused builtins to preserve performances - if !self.builtin_usage.contains(&built_in) { - continue; - } - } - // unrecognized binding, skip - None => continue, - _ => {} + if sm.binding.is_none() { + continue; } let mut sm = sm.clone(); diff --git a/src/front/spv/mod.rs b/src/front/spv/mod.rs index e5e9d4700f..82a86a36a2 100644 --- a/src/front/spv/mod.rs +++ b/src/front/spv/mod.rs @@ -560,9 +560,15 @@ pub struct Frontend { std::hash::BuildHasherDefault, >, - /// Tracks usage of builtins, used to cull unused builtins since they can - /// have serious performance implications. - builtin_usage: FastHashSet, + /// Tracks access to gl_PerVertex's builtins, it is used to cull unused builtins since initializing those can + /// affect performance and the mere presence of some of these builtins might cause backends to error since they + /// might be unsupported. + /// + /// The problematic builtins are: PointSize, ClipDistance and CullDistance. + /// + /// glslang declares those by default even though they are never written to + /// (see ) + gl_per_vertex_builtin_access: FastHashSet, } impl> Frontend { @@ -596,7 +602,7 @@ impl> Frontend { index_constants: Vec::new(), index_constant_expressions: Vec::new(), switch_cases: indexmap::IndexMap::default(), - builtin_usage: FastHashSet::default(), + gl_per_vertex_builtin_access: FastHashSet::default(), } } @@ -1480,7 +1486,8 @@ impl> Frontend { log::trace!("\t\t\tlooking up type {:?}", acex.type_id); let type_lookup = self.lookup_type.lookup(acex.type_id)?; - acex = match ctx.type_arena[type_lookup.handle].inner { + let ty = &ctx.type_arena[type_lookup.handle]; + acex = match ty.inner { // can only index a struct with a constant crate::TypeInner::Struct { ref members, .. } => { let index = index_maybe @@ -1498,10 +1505,12 @@ impl> Frontend { span, ); - if let Some(crate::Binding::BuiltIn(built_in)) = - members[index as usize].binding - { - self.builtin_usage.insert(built_in); + if ty.name.as_deref() == Some("gl_PerVertex") { + if let Some(crate::Binding::BuiltIn(built_in)) = + members[index as usize].binding + { + self.gl_per_vertex_builtin_access.insert(built_in); + } } AccessExpression { diff --git a/tests/out/hlsl/quad-vert.hlsl b/tests/out/hlsl/quad-vert.hlsl index a6c7a23b8b..1fdcc43f53 100644 --- a/tests/out/hlsl/quad-vert.hlsl +++ b/tests/out/hlsl/quad-vert.hlsl @@ -1,9 +1,10 @@ struct gl_PerVertex { float4 gl_Position : SV_Position; - float gl_PointSize : PSIZE; - float gl_ClipDistance[1] : SV_ClipDistance; - float gl_CullDistance[1] : SV_CullDistance; + float gl_PointSize; + float gl_ClipDistance[1]; + float gl_CullDistance[1]; + int _end_pad_0; }; struct type_9 { diff --git a/tests/out/wgsl/quad-vert.wgsl b/tests/out/wgsl/quad-vert.wgsl index fec0c94af8..97eee6a75d 100644 --- a/tests/out/wgsl/quad-vert.wgsl +++ b/tests/out/wgsl/quad-vert.wgsl @@ -1,5 +1,8 @@ struct gl_PerVertex { @builtin(position) gl_Position: vec4, + gl_PointSize: f32, + gl_ClipDistance: array, + gl_CullDistance: array, } struct VertexOutput { @@ -9,7 +12,7 @@ struct VertexOutput { var v_uv: vec2; var a_uv_1: vec2; -var perVertexStruct: gl_PerVertex = gl_PerVertex(vec4(0.0, 0.0, 0.0, 1.0), ); +var perVertexStruct: gl_PerVertex = gl_PerVertex(vec4(0.0, 0.0, 0.0, 1.0), 1.0, array(0.0), array(0.0)); var a_pos_1: vec2; fn main_1() {