Skip to content

Commit

Permalink
Skip gl_PerVertex unused builtins in the SPIR-V frontend (#2272)
Browse files Browse the repository at this point in the history 
Co-authored-by: Jim Blandy <jimb@red-bean.com>
  • Loading branch information
@teoxoy @jimblandy
teoxoy and jimblandy authored Mar 15, 2023
1 parent 7f829c6 commit 63e91fa
Show file tree
Hide file tree
Showing 11 changed files with 125 additions and 214 deletions.
13 changes: 13 additions & 0 deletions src/arena.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,19 @@ impl<T: Eq + hash::Hash> UniqueArena<T> {
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<T>, 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`,
Expand Down
8 changes: 3 additions & 5 deletions src/back/glsl/features.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand Down
24 changes: 4 additions & 20 deletions src/back/glsl/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 1 addition & 4 deletions src/back/hlsl/conv.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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:?}")))
}
})
Expand Down
34 changes: 8 additions & 26 deletions src/back/msl/writer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2233,18 +2233,13 @@ impl<W: Write> Writer<W> {
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 { "," };
Expand Down Expand Up @@ -3563,24 +3558,11 @@ impl<W: Write> Writer<W> {
};
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 {
Expand Down
164 changes: 30 additions & 134 deletions src/back/wgsl/writer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,11 +335,8 @@ impl<W: Write> Writer<W> {
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 {
Expand Down Expand Up @@ -401,14 +398,6 @@ impl<W: Write> Writer<W> {
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 {
Expand Down Expand Up @@ -669,22 +658,6 @@ impl<W: Write> Writer<W> {
_ => 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
Expand Down Expand Up @@ -746,14 +719,6 @@ impl<W: Write> Writer<W> {
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) {
Expand Down Expand Up @@ -1150,42 +1115,10 @@ impl<W: Write> Writer<W> {
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, ", ")?;
}
Expand Down Expand Up @@ -1764,25 +1697,8 @@ impl<W: Write> Writer<W> {
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) {
Expand Down Expand Up @@ -1869,26 +1785,34 @@ impl<W: Write> Writer<W> {
}
}

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 {
Expand Down Expand Up @@ -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<crate::Expression>,
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
}
4 changes: 2 additions & 2 deletions src/front/glsl/variables.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 63e91fa

Please sign in to comment.