From 605febedbbf322badd02316d0db3282360570d60 Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Sat, 24 Apr 2021 00:31:56 +0200 Subject: [PATCH 1/8] [msl-out] wrap arrays in structs so that they can be returned by functions --- src/back/msl/writer.rs | 43 +++++++++++++++++++++++++++++++++++------ tests/out/access.msl | 6 ++++-- tests/out/quad-vert.msl | 4 +++- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index 638ec4e8e8..cba34f00fd 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -17,6 +17,7 @@ use std::{ const NAMESPACE: &str = "metal"; const INDENT: &str = " "; const BAKE_PREFIX: &str = "_e"; +const WRAPPED_ARRAY_FIELD: &str = "inner"; #[derive(Clone)] struct Level(usize); @@ -594,8 +595,31 @@ impl Writer { log::trace!("expression {:?} = {:?}", expr_handle, expression); match *expression { crate::Expression::Access { base, index } => { + let accessing_wrapped_array = { + let ty = match &context.info[base].ty { + TypeResolution::Value(value) => value, + TypeResolution::Handle(handle) => &context.module.types[*handle].inner, + }; + + match ty { + crate::TypeInner::Array { .. } => true, + crate::TypeInner::Pointer { + base: pointer_base, + class, + } => match (&context.module.types[*pointer_base].inner, class) { + (crate::TypeInner::Array { .. }, crate::StorageClass::Function) => true, + _ => false, + }, + _ => false, + } + }; + self.put_expression(base, context, false)?; - write!(self.out, "[")?; + if accessing_wrapped_array { + write!(self.out, ".{}[", WRAPPED_ARRAY_FIELD)?; + } else { + write!(self.out, "[")?; + } self.put_expression(index, context, true)?; write!(self.out, "]")?; } @@ -1370,9 +1394,9 @@ impl Writer { .unwrap(); write!(self.out, "{}for(int _i=0; _i<{}; ++_i) ", level, size)?; self.put_expression(pointer, &context.expression, true)?; - write!(self.out, "[_i] = ")?; + write!(self.out, ".{}[_i] = ", WRAPPED_ARRAY_FIELD)?; self.put_expression(value, &context.expression, true)?; - writeln!(self.out, "[_i];")?; + writeln!(self.out, ".{}[_i];", WRAPPED_ARRAY_FIELD)?; } None => { write!(self.out, "{}", level)?; @@ -1493,7 +1517,7 @@ impl Writer { access: crate::StorageAccess::empty(), first_time: false, }; - write!(self.out, "typedef {} {}", base_name, name)?; + match size { crate::ArraySize::Constant(const_handle) => { let coco = ConstantContext { @@ -1502,10 +1526,17 @@ impl Writer { names: &self.names, first_time: false, }; - writeln!(self.out, "[{}];", coco)?; + + writeln!(self.out, "struct {} {{", name)?; + writeln!( + self.out, + "{}{} {}[{}];", + INDENT, base_name, WRAPPED_ARRAY_FIELD, coco + )?; + writeln!(self.out, "}};")?; } crate::ArraySize::Dynamic => { - writeln!(self.out, "[1];")?; + writeln!(self.out, "typedef {} {}[1];", base_name, name)?; } } } diff --git a/tests/out/access.msl b/tests/out/access.msl index 2c3da3e904..36ed564822 100644 --- a/tests/out/access.msl +++ b/tests/out/access.msl @@ -1,7 +1,9 @@ #include #include -typedef int type3[5]; +struct type3 { + int inner[5]; +}; struct fooInput { }; @@ -11,5 +13,5 @@ struct fooOutput { vertex fooOutput foo( metal::uint vi [[vertex_id]] ) { - return fooOutput { static_cast(int4(type3 {1, 2, 3, 4, 5}[vi])) }; + return fooOutput { static_cast(int4(type3 {1, 2, 3, 4, 5}.inner[vi])) }; } diff --git a/tests/out/quad-vert.msl b/tests/out/quad-vert.msl index 9e0ff87e48..3e40adc194 100644 --- a/tests/out/quad-vert.msl +++ b/tests/out/quad-vert.msl @@ -1,7 +1,9 @@ #include #include -typedef float type6[1u]; +struct type6 { + float inner[1u]; +}; struct gl_PerVertex { metal::float4 gl_Position; float gl_PointSize; From fdce46bbbf94accd4d959e79f908d4aedc1f9237 Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Sat, 24 Apr 2021 01:14:25 +0200 Subject: [PATCH 2/8] Fix clippy problems --- src/back/msl/writer.rs | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index cba34f00fd..4820b15590 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -595,23 +595,19 @@ impl Writer { log::trace!("expression {:?} = {:?}", expr_handle, expression); match *expression { crate::Expression::Access { base, index } => { - let accessing_wrapped_array = { - let ty = match &context.info[base].ty { - TypeResolution::Value(value) => value, - TypeResolution::Handle(handle) => &context.module.types[*handle].inner, - }; - - match ty { - crate::TypeInner::Array { .. } => true, - crate::TypeInner::Pointer { - base: pointer_base, - class, - } => match (&context.module.types[*pointer_base].inner, class) { - (crate::TypeInner::Array { .. }, crate::StorageClass::Function) => true, - _ => false, - }, + let accessing_wrapped_array = match *context.info[base] + .ty + .inner_with(&context.module.types) + { + crate::TypeInner::Array { .. } => true, + crate::TypeInner::Pointer { + base: pointer_base, + class, + } => match (&context.module.types[pointer_base].inner, class) { + (&crate::TypeInner::Array { .. }, crate::StorageClass::Function) => true, _ => false, - } + }, + _ => false, }; self.put_expression(base, context, false)?; From 0b376fa4768fa77f9ad9d38060067c8d266cbda9 Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Sat, 24 Apr 2021 11:27:16 +0200 Subject: [PATCH 3/8] use a raw array for output fields --- src/back/msl/writer.rs | 58 +++++++++++++++++++++++++++++------------ tests/out/quad-vert.msl | 6 ++--- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index 4820b15590..85f4b3b770 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -125,7 +125,20 @@ impl<'a> Display for TypeContext<'a> { vector_size_string(size), ) } - crate::TypeInner::Array { .. } | crate::TypeInner::Struct { .. } => unreachable!(), + crate::TypeInner::Array { base, .. } => { + let sub = Self { + arena: self.arena, + names: self.names, + handle: base, + usage: self.usage, + access: self.access, + first_time: false, + }; + // Array lengths go at the end of the type definition, + // so just print the element type here. + write!(out, "{}", sub) + } + crate::TypeInner::Struct { .. } => unreachable!(), crate::TypeInner::Image { dim, arrayed, @@ -565,7 +578,7 @@ impl Writer { write!(self.out, ",")?; } self.put_expression(component, context, false)?; - write!(self.out, "[{}]", j)?; + write!(self.out, ".inner[{}]", j)?; } write!(self.out, "}}")?; } else { @@ -595,20 +608,21 @@ impl Writer { log::trace!("expression {:?} = {:?}", expr_handle, expression); match *expression { crate::Expression::Access { base, index } => { - let accessing_wrapped_array = match *context.info[base] - .ty - .inner_with(&context.module.types) - { - crate::TypeInner::Array { .. } => true, - crate::TypeInner::Pointer { - base: pointer_base, - class, - } => match (&context.module.types[pointer_base].inner, class) { - (&crate::TypeInner::Array { .. }, crate::StorageClass::Function) => true, + let accessing_wrapped_array = + match *context.info[base].ty.inner_with(&context.module.types) { + crate::TypeInner::Array { .. } => true, + crate::TypeInner::Pointer { + base: pointer_base, + class, + } => match &context.module.types[pointer_base].inner { + &crate::TypeInner::Array { .. } => { + class == crate::StorageClass::Function + || class == crate::StorageClass::Private + } + _ => false, + }, _ => false, - }, - _ => false, - }; + }; self.put_expression(base, context, false)?; if accessing_wrapped_array { @@ -1180,7 +1194,7 @@ impl Writer { if j != 0 { write!(self.out, ",")?; } - write!(self.out, "{}.{}[{}]", tmp, name, j)?; + write!(self.out, "{}.{}.inner[{}]", tmp, name, j)?; } write!(self.out, "}}")?; } else { @@ -1994,7 +2008,7 @@ impl Writer { names: &self.names, usage: GlobalUse::empty(), access: crate::StorageAccess::empty(), - first_time: false, + first_time: true, }; let binding = binding.ok_or(Error::Validation)?; if !pipeline_options.allow_point_size @@ -2002,9 +2016,19 @@ impl Writer { { continue; } + let array_len = match &module.types[ty].inner { + crate::TypeInner::Array { + size: crate::ArraySize::Constant(handle), + .. + } => module.constants[*handle].to_array_length(), + _ => None, + }; let resolved = options.resolve_local_binding(binding, out_mode)?; write!(self.out, "{}{} {}", INDENT, ty_name, name)?; resolved.try_fmt_decorated(&mut self.out, "")?; + if let Some(array_len) = array_len { + write!(self.out, " [{}]", array_len)?; + } writeln!(self.out, ";")?; } writeln!(self.out, "}};")?; diff --git a/tests/out/quad-vert.msl b/tests/out/quad-vert.msl index 3e40adc194..e322e3585d 100644 --- a/tests/out/quad-vert.msl +++ b/tests/out/quad-vert.msl @@ -37,7 +37,7 @@ struct main2Output { metal::float2 member [[user(loc0), center_perspective]]; metal::float4 gl_Position1 [[position]]; float gl_PointSize1 [[point_size]]; - type6 gl_ClipDistance1 [[clip_distance]]; + float gl_ClipDistance1 [[clip_distance]] [1]; }; vertex main2Output main2( main2Input varyings [[stage_in]] @@ -51,6 +51,6 @@ vertex main2Output main2( a_uv = a_uv1; a_pos = a_pos1; main1(v_uv, a_uv, _, a_pos); - const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, {_.gl_ClipDistance[0]}}; - return main2Output { _tmp.member, _tmp.gl_Position1, _tmp.gl_PointSize1, {_tmp.gl_ClipDistance1[0]} }; + const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, {_.gl_ClipDistance.inner[0]}}; + return main2Output { _tmp.member, _tmp.gl_Position1, _tmp.gl_PointSize1, {_tmp.gl_ClipDistance1.inner[0]} }; } From 68d757d0c72ab4b50e6fc0ab34cecf09894856be Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Sat, 24 Apr 2021 11:32:23 +0200 Subject: [PATCH 4/8] Fix clippy problems --- src/back/msl/writer.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index 85f4b3b770..aff6bdc352 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -578,7 +578,7 @@ impl Writer { write!(self.out, ",")?; } self.put_expression(component, context, false)?; - write!(self.out, ".inner[{}]", j)?; + write!(self.out, ".{}[{}]", WRAPPED_ARRAY_FIELD, j)?; } write!(self.out, "}}")?; } else { @@ -614,8 +614,8 @@ impl Writer { crate::TypeInner::Pointer { base: pointer_base, class, - } => match &context.module.types[pointer_base].inner { - &crate::TypeInner::Array { .. } => { + } => match context.module.types[pointer_base].inner { + crate::TypeInner::Array { .. } => { class == crate::StorageClass::Function || class == crate::StorageClass::Private } @@ -1194,7 +1194,11 @@ impl Writer { if j != 0 { write!(self.out, ",")?; } - write!(self.out, "{}.{}.inner[{}]", tmp, name, j)?; + write!( + self.out, + "{}.{}.{}[{}]", + tmp, name, WRAPPED_ARRAY_FIELD, j + )?; } write!(self.out, "}}")?; } else { @@ -2016,11 +2020,11 @@ impl Writer { { continue; } - let array_len = match &module.types[ty].inner { + let array_len = match module.types[ty].inner { crate::TypeInner::Array { size: crate::ArraySize::Constant(handle), .. - } => module.constants[*handle].to_array_length(), + } => module.constants[handle].to_array_length(), _ => None, }; let resolved = options.resolve_local_binding(binding, out_mode)?; From 749eb594bc1282af2a07dd1dae9bd2500bba337e Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Mon, 26 Apr 2021 09:34:54 +0200 Subject: [PATCH 5/8] Apply suggestions --- src/back/msl/writer.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index aff6bdc352..5e43a4b2bd 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -81,12 +81,9 @@ impl<'a> Display for TypeContext<'a> { } crate::TypeInner::Pointer { base, class } => { let sub = Self { - arena: self.arena, - names: self.names, handle: base, - usage: self.usage, - access: self.access, first_time: false, + ..*self }; let class_name = match class.get_name(self.usage) { Some(name) => name, @@ -127,12 +124,9 @@ impl<'a> Display for TypeContext<'a> { } crate::TypeInner::Array { base, .. } => { let sub = Self { - arena: self.arena, - names: self.names, handle: base, - usage: self.usage, - access: self.access, first_time: false, + ..*self }; // Array lengths go at the end of the type definition, // so just print the element type here. @@ -626,10 +620,9 @@ impl Writer { self.put_expression(base, context, false)?; if accessing_wrapped_array { - write!(self.out, ".{}[", WRAPPED_ARRAY_FIELD)?; - } else { - write!(self.out, "[")?; + write!(self.out, ".{}", WRAPPED_ARRAY_FIELD)?; } + write!(self.out, "[")?; self.put_expression(index, context, true)?; write!(self.out, "]")?; } From 5c0336ad3eb3eb9e1c528a141e592bdbf691eed7 Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Mon, 26 Apr 2021 09:39:50 +0200 Subject: [PATCH 6/8] Remove put_initialization_component --- src/back/msl/writer.rs | 39 ++++----------------------------------- tests/out/quad-vert.msl | 2 +- 2 files changed, 5 insertions(+), 36 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index 5e43a4b2bd..98eabb6791 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -548,39 +548,6 @@ impl Writer { Ok(()) } - fn put_initialization_component( - &mut self, - component: Handle, - context: &ExpressionContext, - ) -> Result<(), Error> { - // we can't initialize the array members just like other members, - // we have to unwrap them one level deeper... - let component_res = &context.info[component].ty; - if let crate::TypeInner::Array { - size: crate::ArraySize::Constant(const_handle), - .. - } = *component_res.inner_with(&context.module.types) - { - //HACK: we are forcefully duplicating the expression here, - // it would be nice to find a more C++ idiomatic solution for initializing array members - let size = context.module.constants[const_handle] - .to_array_length() - .unwrap(); - write!(self.out, "{{")?; - for j in 0..size { - if j != 0 { - write!(self.out, ",")?; - } - self.put_expression(component, context, false)?; - write!(self.out, ".{}[{}]", WRAPPED_ARRAY_FIELD, j)?; - } - write!(self.out, "}}")?; - } else { - self.put_expression(component, context, true)?; - } - Ok(()) - } - fn put_expression( &mut self, expr_handle: Handle, @@ -724,7 +691,7 @@ impl Writer { if i != 0 { write!(self.out, ", ")?; } - self.put_initialization_component(component, context)?; + self.put_expression(component, context, true)?; } write!(self.out, "}}")?; } @@ -1173,7 +1140,9 @@ impl Writer { let comma = if is_first { "" } else { "," }; is_first = false; let name = &self.names[&NameKey::StructMember(result_ty, index as u32)]; - // logic similar to `put_initialization_component` + // HACK: we are forcefully deduplicating the expression here + // to convert from a wrapped struct to a raw array, e.g. + // `float gl_ClipDistance1 [[clip_distance]] [1];`. if let crate::TypeInner::Array { size: crate::ArraySize::Constant(const_handle), .. diff --git a/tests/out/quad-vert.msl b/tests/out/quad-vert.msl index e322e3585d..584a9a748f 100644 --- a/tests/out/quad-vert.msl +++ b/tests/out/quad-vert.msl @@ -51,6 +51,6 @@ vertex main2Output main2( a_uv = a_uv1; a_pos = a_pos1; main1(v_uv, a_uv, _, a_pos); - const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, {_.gl_ClipDistance.inner[0]}}; + const auto _tmp = type10 {v_uv, _.gl_Position, _.gl_PointSize, _.gl_ClipDistance}; return main2Output { _tmp.member, _tmp.gl_Position1, _tmp.gl_PointSize1, {_tmp.gl_ClipDistance1.inner[0]} }; } From 1daaaa06f26e2894c5e751b1c870fdc637e9c557 Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Mon, 26 Apr 2021 17:29:21 +0200 Subject: [PATCH 7/8] Check if the array is a constant size --- src/back/msl/writer.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index 98eabb6791..88501f4082 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -576,10 +576,10 @@ impl Writer { base: pointer_base, class, } => match context.module.types[pointer_base].inner { - crate::TypeInner::Array { .. } => { - class == crate::StorageClass::Function - || class == crate::StorageClass::Private - } + crate::TypeInner::Array { + size: crate::ArraySize::Constant(_), + .. + } => true, _ => false, }, _ => false, From 1c3a2a960f200263231a4344e762e14a8260f117 Mon Sep 17 00:00:00 2001 From: Ashley Ruglys Date: Mon, 26 Apr 2021 17:38:23 +0200 Subject: [PATCH 8/8] Don't use the pointer class --- src/back/msl/writer.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index b5ffb09f3b..fc5745865b 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -566,8 +566,7 @@ impl Writer { match *context.info[base].ty.inner_with(&context.module.types) { crate::TypeInner::Array { .. } => true, crate::TypeInner::Pointer { - base: pointer_base, - class, + base: pointer_base, .. } => match context.module.types[pointer_base].inner { crate::TypeInner::Array { size: crate::ArraySize::Constant(_),