From 64bb2104cf14c84c4f39c61df2da379f0ff0427d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Capucho?= Date: Thu, 27 Aug 2020 15:26:55 +0100 Subject: [PATCH] glsl-out: Less allocations, handle struct member origin and correctly handle empty names (#160) * Removed half of the clones Fixed struct field names with invalid ident * Fix struct member origin not being respected * is_valid_ident handles empty names --- src/back/glsl.rs | 201 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 136 insertions(+), 65 deletions(-) diff --git a/src/back/glsl.rs b/src/back/glsl.rs index 8e1a5cbb0e..4a9eaec42c 100644 --- a/src/back/glsl.rs +++ b/src/back/glsl.rs @@ -1,8 +1,8 @@ use crate::{ Arena, ArraySize, BinaryOperator, BuiltIn, Constant, ConstantInner, DerivativeAxis, Expression, FastHashMap, Function, FunctionOrigin, GlobalVariable, Handle, ImageClass, Interpolation, - IntrinsicFunction, LocalVariable, Module, ScalarKind, ShaderStage, Statement, StorageClass, - StructMember, Type, TypeInner, UnaryOperator, + IntrinsicFunction, LocalVariable, MemberOrigin, Module, ScalarKind, ShaderStage, Statement, + StorageClass, StructMember, Type, TypeInner, UnaryOperator, }; use std::{ borrow::Cow, @@ -102,12 +102,7 @@ pub fn write<'a>(module: &'a Module, out: &mut impl Write, options: Options) -> let mut namer = |name: Option<&'a String>| { if let Some(name) = name { - if name.starts_with(|c: char| !(c.is_ascii_alphabetic() || c == '_')) - || name.contains(|c: char| !(c.is_ascii_alphanumeric() || c == '_')) - || name.starts_with("gl_") - || name == "main" - || names.get(name.as_str()).is_some() - { + if !is_valid_ident(name) || names.get(name.as_str()).is_some() { counter += 1; while names.get(format!("_{}", counter).as_str()).is_some() { counter += 1; @@ -258,7 +253,7 @@ pub fn write<'a>(module: &'a Module, out: &mut impl Write, options: Options) -> if let Some(ref binding) = global.binding { if !es { - write!(out, "layout({}) ", Binding(binding.clone()))?; + write!(out, "layout({}) ", Binding(binding))?; } } @@ -405,8 +400,8 @@ pub fn write<'a>(module: &'a Module, out: &mut impl Write, options: Options) -> Ok(()) } -struct Binding(crate::Binding); -impl fmt::Display for Binding { +struct Binding<'a>(&'a crate::Binding); +impl<'a> fmt::Display for Binding<'a> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self.0 { crate::Binding::BuiltIn(_) => write!(f, ""), // Ignore because they are variables with a predefined name @@ -429,10 +424,10 @@ struct StatementBuilder<'a> { pub features: SupportedFeatures, } -fn write_statement( +fn write_statement<'a, 'b>( sta: &Statement, - module: &Module, - builder: &mut StatementBuilder<'_>, + module: &'a Module, + builder: &'b mut StatementBuilder<'a>, indent: usize, ) -> Result { Ok(match sta { @@ -566,29 +561,39 @@ fn write_statement( }) } -fn write_expression<'a>( +fn write_expression<'a, 'b>( expr: &Expression, module: &'a Module, - builder: &mut StatementBuilder<'_>, -) -> Result<(String, Cow<'a, TypeInner>), Error> { + builder: &'b mut StatementBuilder<'a>, +) -> Result<(Cow<'a, str>, Cow<'a, TypeInner>), Error> { Ok(match *expr { Expression::Access { base, index } => { let (base_expr, ty) = write_expression(&builder.expressions[base], module, builder)?; let inner = match *ty.as_ref() { - TypeInner::Vector { kind, width, .. } | TypeInner::Matrix { kind, width, .. } => { + TypeInner::Vector { kind, width, .. } => { Cow::Owned(TypeInner::Scalar { kind, width }) } + TypeInner::Matrix { + kind, + width, + columns, + .. + } => Cow::Owned(TypeInner::Vector { + kind, + width, + size: columns, + }), TypeInner::Array { base, .. } => Cow::Borrowed(&module.types[base].inner), _ => return Err(Error::Custom(format!("Cannot dynamically index {:?}", ty))), }; ( - format!( + Cow::Owned(format!( "{}[{}]", base_expr, write_expression(&builder.expressions[index], module, builder)?.0 - ), + )), inner, ) } @@ -596,35 +601,53 @@ fn write_expression<'a>( let (base_expr, ty) = write_expression(&builder.expressions[base], module, builder)?; match *ty.as_ref() { - TypeInner::Vector { kind, width, .. } | TypeInner::Matrix { kind, width, .. } => ( - format!("{}[{}]", base_expr, index), + TypeInner::Vector { kind, width, .. } => ( + Cow::Owned(format!("{}[{}]", base_expr, index)), Cow::Owned(TypeInner::Scalar { kind, width }), ), + TypeInner::Matrix { + kind, + width, + columns, + .. + } => ( + Cow::Owned(format!("{}[{}]", base_expr, index)), + Cow::Owned(TypeInner::Vector { + kind, + width, + size: columns, + }), + ), TypeInner::Array { base, .. } => ( - format!("{}[{}]", base_expr, index), + Cow::Owned(format!("{}[{}]", base_expr, index)), Cow::Borrowed(&module.types[base].inner), ), TypeInner::Struct { ref members } => ( - format!( - "{}.{}", - base_expr, - members[index as usize] - .name - .as_ref() - .unwrap_or(&format!("_{}", index)) - ), + if let MemberOrigin::BuiltIn(builtin) = members[index as usize].origin { + Cow::Borrowed(builtin_to_glsl(builtin)) + } else { + Cow::Owned(format!( + "{}.{}", + base_expr, + members[index as usize] + .name + .as_ref() + .filter(|s| is_valid_ident(s)) + .unwrap_or(&format!("_{}", index)) + )) + }, Cow::Borrowed(&module.types[members[index as usize].ty].inner), ), _ => return Err(Error::Custom(format!("Cannot index {:?}", ty))), } } Expression::Constant(constant) => ( - write_constant( + Cow::Owned(write_constant( &module.constants[constant], module, builder, builder.features, - )?, + )?), Cow::Borrowed(&module.types[module.constants[constant].ty].inner), ), Expression::Compose { ty, ref components } => { @@ -685,7 +708,7 @@ fn write_expression<'a>( }; ( - format!( + Cow::Owned(format!( "{}({})", constructor, components @@ -698,21 +721,21 @@ fn write_expression<'a>( .0)) .collect::, _>>()? .join(","), - ), + )), Cow::Borrowed(&module.types[ty].inner), ) } Expression::FunctionParameter(pos) => { - let (arg, ty) = builder.args.get(&pos).unwrap().clone(); + let (arg, ty) = builder.args.get(&pos).unwrap(); - (arg, Cow::Borrowed(&module.types[ty].inner)) + (Cow::Borrowed(arg), Cow::Borrowed(&module.types[*ty].inner)) } Expression::GlobalVariable(handle) => ( - builder.globals.get(&handle).unwrap().clone(), + Cow::Borrowed(builder.globals.get(&handle).unwrap()), Cow::Borrowed(&module.types[module.global_variables[handle].ty].inner), ), Expression::LocalVariable(handle) => ( - builder.locals_lookup.get(&handle).unwrap().clone(), + Cow::Borrowed(builder.locals_lookup.get(&handle).unwrap()), Cow::Borrowed(&module.types[builder.locals[handle].ty].inner), ), Expression::Load { pointer } => { @@ -782,12 +805,12 @@ fn write_expression<'a>( ); let coordinate = if let Some(depth_ref) = depth_ref { - format!( + Cow::Owned(format!( "vec{}({},{})", size as u8 + 1, coordinate_expr, write_expression(&builder.expressions[depth_ref], module, builder)?.0 - ) + )) } else { coordinate_expr }; @@ -815,7 +838,7 @@ fn write_expression<'a>( Cow::Owned(TypeInner::Vector { kind, width, size }) }; - (expr, ty) + (Cow::Owned(expr), ty) } Expression::ImageLoad { image, @@ -873,20 +896,23 @@ fn write_expression<'a>( }; let width = 4; - (expr, Cow::Owned(TypeInner::Vector { kind, width, size })) + ( + Cow::Owned(expr), + Cow::Owned(TypeInner::Vector { kind, width, size }), + ) } Expression::Unary { op, expr } => { let (expr, ty) = write_expression(&builder.expressions[expr], module, builder)?; ( - format!( + Cow::Owned(format!( "({} {})", match op { UnaryOperator::Negate => "-", UnaryOperator::Not => "~", }, expr - ), + )), ty, ) } @@ -936,13 +962,16 @@ fn write_expression<'a>( } }; - (format!("({} {} {})", left_expr, op, right_expr), ty) + ( + Cow::Owned(format!("({} {} {})", left_expr, op, right_expr)), + ty, + ) } Expression::Intrinsic { fun, argument } => { let (expr, ty) = write_expression(&builder.expressions[argument], module, builder)?; ( - format!( + Cow::Owned(format!( "{:?}({})", match fun { IntrinsicFunction::IsFinite => "!isinf", @@ -953,7 +982,7 @@ fn write_expression<'a>( IntrinsicFunction::Any => "any", }, expr - ), + )), ty, ) } @@ -975,20 +1004,23 @@ fn write_expression<'a>( } }; - (format!("dot({},{})", left_expr, right_expr), ty) + (Cow::Owned(format!("dot({},{})", left_expr, right_expr)), ty) } Expression::CrossProduct(left, right) => { let (left_expr, left_ty) = write_expression(&builder.expressions[left], module, builder)?; let (right_expr, _) = write_expression(&builder.expressions[right], module, builder)?; - (format!("cross({},{})", left_expr, right_expr), left_ty) + ( + Cow::Owned(format!("cross({},{})", left_expr, right_expr)), + left_ty, + ) } Expression::Derivative { axis, expr } => { let (expr, ty) = write_expression(&builder.expressions[expr], module, builder)?; ( - format!( + Cow::Owned(format!( "{}({})", match axis { DerivativeAxis::X => "dFdx", @@ -996,7 +1028,7 @@ fn write_expression<'a>( DerivativeAxis::Width => "fwidth", }, expr - ), + )), ty, ) } @@ -1017,7 +1049,7 @@ fn write_expression<'a>( }; ( - format!( + Cow::Owned(format!( "{}({})", match *origin { FunctionOrigin::External(ref name) => name, @@ -1033,7 +1065,7 @@ fn write_expression<'a>( .0)) .collect::, _>>()? .join(","), - ), + )), ty, ) } @@ -1054,13 +1086,13 @@ fn write_constant( ConstantInner::Composite(ref components) => format!( "{}({})", match module.types[constant.ty].inner { - TypeInner::Vector { size, .. } => format!("vec{}", size as u8,), + TypeInner::Vector { size, .. } => Cow::Owned(format!("vec{}", size as u8,)), TypeInner::Matrix { columns, rows, .. } => - format!("mat{}x{}", columns as u8, rows as u8,), - TypeInner::Struct { .. } => builder.structs.get(&constant.ty).unwrap().clone(), + Cow::Owned(format!("mat{}x{}", columns as u8, rows as u8,)), + TypeInner::Struct { .. } => + Cow::::Borrowed(builder.structs.get(&constant.ty).unwrap()), TypeInner::Array { .. } => - write_type(constant.ty, &module.types, builder.structs, None, features)? - .into_owned(), + write_type(constant.ty, &module.types, builder.structs, None, features)?, _ => return Err(Error::Custom(format!( "Cannot build constant of type {}", @@ -1081,13 +1113,13 @@ fn write_constant( }) } -fn write_type( +fn write_type<'a>( ty: Handle, types: &Arena, - structs: &FastHashMap, String>, + structs: &'a FastHashMap, String>, block: Option, features: SupportedFeatures, -) -> Result, Error> { +) -> Result, Error> { Ok(match types[ty].inner { TypeInner::Scalar { kind, width } => match kind { ScalarKind::Sint => Cow::Borrowed("int"), @@ -1170,7 +1202,11 @@ fn write_type( &mut out, "\t{} {};", write_type(member.ty, types, structs, None, features)?, - member.name.clone().unwrap_or_else(|| idx.to_string()) + member + .name + .clone() + .filter(|s| is_valid_ident(s)) + .unwrap_or_else(|| format!("_{}", idx)) )?; } @@ -1178,7 +1214,7 @@ fn write_type( Cow::Owned(out) } else { - Cow::Owned(structs.get(&ty).unwrap().clone()) + Cow::Borrowed(structs.get(&ty).unwrap()) } } TypeInner::Image { @@ -1335,6 +1371,10 @@ fn write_struct( writeln!(&mut tmp, "struct {} {{", name)?; for (idx, member) in members.iter().enumerate() { + if let MemberOrigin::BuiltIn(_) = member.origin { + continue; + } + if let TypeInner::Struct { ref members } = module.types[member.ty].inner { write_struct( member.ty, @@ -1351,7 +1391,11 @@ fn write_struct( &mut tmp, "\t{} {};", write_type(member.ty, &module.types, &structs, None, features)?, - member.name.clone().unwrap_or_else(|| format!("_{}", idx)) + member + .name + .clone() + .filter(|s| is_valid_ident(s)) + .unwrap_or_else(|| format!("_{}", idx)) )?; } writeln!(&mut tmp, "}};")?; @@ -1362,3 +1406,30 @@ fn write_struct( Ok(()) } + +fn is_valid_ident(ident: &str) -> bool { + ident.starts_with(|c: char| c.is_ascii_alphabetic() || c == '_') + || ident.contains(|c: char| c.is_ascii_alphanumeric() || c == '_') + || !ident.starts_with("gl_") + || ident != "main" +} + +fn builtin_to_glsl(builtin: BuiltIn) -> &'static str { + match builtin { + BuiltIn::Position => "gl_Position", + BuiltIn::GlobalInvocationId => "gl_GlobalInvocationID", + BuiltIn::BaseInstance => "gl_BaseInstance", + BuiltIn::BaseVertex => "gl_BaseVertex", + BuiltIn::ClipDistance => "gl_ClipDistance", + BuiltIn::InstanceIndex => "gl_InstanceIndex", + BuiltIn::VertexIndex => "gl_VertexIndex", + BuiltIn::PointSize => "gl_PointSize", + BuiltIn::FragCoord => "gl_FragCoord", + BuiltIn::FrontFacing => "gl_FrontFacing", + BuiltIn::SampleIndex => "gl_SampleID", + BuiltIn::FragDepth => "gl_FragDepth", + BuiltIn::LocalInvocationId => "gl_LocalInvocationID", + BuiltIn::LocalInvocationIndex => "gl_LocalInvocationIndex", + BuiltIn::WorkGroupId => "gl_WorkGroupID", + } +}