Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip gl_PerVertex unused builtins in the SPIR-V frontend #2272

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Skip gl_PerVertex unused builtins in the SPIR-V frontend #2272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a0ea7f
Skip `gl_PerVertex` unused builtins in the SPIR-V frontend
teoxoy Mar 9, 2023
bb5b03c
Update comment
teoxoy Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/arena.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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) => {
teoxoy marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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,
teoxoy marked this conversation as resolved.
Show resolved Hide resolved
_ => unreachable!(),
},
mutable: self.meta.stage == ShaderStage::Vertex,
Expand Down
Loading