From 8db0b198e1045801483981240201265e812a38d1 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Sun, 22 Dec 2024 15:08:35 -0800 Subject: [PATCH 01/37] type/expression tandem --- naga/src/compact/mod.rs | 91 +++++++++++++++++++++++++-------------- naga/src/compact/types.rs | 77 +++++++++++++++++++++------------ 2 files changed, 109 insertions(+), 59 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 9dff4a6cc2..40ee5680a3 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -63,16 +63,6 @@ pub fn compact(module: &mut crate::Module) { } } - for (_, ty) in module.types.iter() { - if let crate::TypeInner::Array { - size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(size_expr)), - .. - } = ty.inner - { - module_tracer.global_expressions_used.insert(size_expr); - } - } - for e in module.entry_points.iter() { if let Some(sizes) = e.workgroup_size_overrides { for size in sizes.iter().filter_map(|x| *x) { @@ -112,31 +102,41 @@ pub fn compact(module: &mut crate::Module) { }) .collect(); - // Given that the above steps have marked all the constant - // expressions used directly by globals, constants, functions, and - // entry points, walk the constant expression arena to find all - // constant expressions used, directly or indirectly. - module_tracer.as_const_expression().trace_expressions(); - - // Constants' initializers are taken care of already, because - // expression tracing sees through constants. But we still need to - // note type usage. - for (handle, constant) in module.constants.iter() { - if module_tracer.constants_used.contains(handle) { - module_tracer.types_used.insert(constant.ty); + module_tracer.type_expression_tandem(|module_tracer| { + // Given that the above steps have marked all the constant + // expressions used directly by globals, constants, functions, and + // entry points, walk the constant expression arena to find all + // constant expressions used, directly or indirectly. + + // Constants' initializers are taken care of already, because + // expression tracing sees through constants. But we still need to + // note type usage. + for (handle, constant) in module.constants.iter() { + if module_tracer.constants_used.contains(handle) { + module_tracer.types_used_insert(constant.ty); + } } - } - // Treat all named types as used. - for (handle, ty) in module.types.iter() { - log::trace!("tracing type {:?}, name {:?}", handle, ty.name); - if ty.name.is_some() { - module_tracer.types_used.insert(handle); + // Treat all named types as used. + for (handle, ty) in module.types.iter().rev() { + log::trace!("tracing type {:?}, name {:?}", handle, ty.name); + if ty.name.is_some() { + module_tracer.types_used_insert(handle); + } } - } - - // Propagate usage through types. - module_tracer.as_type().trace_types(); + }, |module_tracer| { + for (handle, ty) in module.types.iter() { + if module_tracer.types_used.contains(handle) { + if let crate::TypeInner::Array { + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(size_expr)), + .. + } = ty.inner + { + module_tracer.global_expressions_used.insert(size_expr); + } + } + } + }); // Now that we know what is used and what is never touched, // produce maps from the `Handle`s that appear in `module` now to @@ -268,6 +268,10 @@ struct ModuleTracer<'module> { global_expressions_used: HandleSet, } +fn handle2type(x: &mut types::TypeTracer, y: crate::Handle) { + x.trace_type(&x.types[y], handle2type); +} + impl<'module> ModuleTracer<'module> { fn new(module: &'module crate::Module) -> Self { Self { @@ -296,13 +300,36 @@ impl<'module> ModuleTracer<'module> { } } + fn type_expression_tandem(&mut self, e2t: impl FnOnce(&mut Self), t2e: impl FnOnce(&mut Self)) { + t2e(self); + self.as_const_expression().trace_expressions(); + e2t(self); + + // Propagate usage through types. + self.as_type().trace_types(); + } + + fn types_used_insert(&mut self, x: crate::Handle) -> bool { + self.trace_type(x); + self.types_used.insert(x) + } + fn as_type(&mut self) -> types::TypeTracer { types::TypeTracer { types: &self.module.types, types_used: &mut self.types_used, + expressions_used: &mut self.global_expressions_used, } } + fn trace_type(&mut self, x: crate::Handle) { + types::TypeTracer { + types: &self.module.types, + types_used: &mut self.types_used, + expressions_used: &mut self.global_expressions_used, + }.trace_type(&self.module.types[x], handle2type) + } + fn as_const_expression(&mut self) -> expressions::ExpressionTracer { expressions::ExpressionTracer { expressions: &self.module.global_expressions, diff --git a/naga/src/compact/types.rs b/naga/src/compact/types.rs index ec75e3ae2a..dee2865599 100644 --- a/naga/src/compact/types.rs +++ b/naga/src/compact/types.rs @@ -4,6 +4,7 @@ use crate::{Handle, UniqueArena}; pub struct TypeTracer<'a> { pub types: &'a UniqueArena, pub types_used: &'a mut HandleSet, + pub expressions_used: &'a mut HandleSet, } impl TypeTracer<'_> { @@ -24,34 +25,56 @@ impl TypeTracer<'_> { continue; } - use crate::TypeInner as Ti; - match ty.inner { - // Types that do not contain handles. - Ti::Scalar { .. } - | Ti::Vector { .. } - | Ti::Matrix { .. } - | Ti::Atomic { .. } - | Ti::ValuePointer { .. } - | Ti::Image { .. } - | Ti::Sampler { .. } - | Ti::AccelerationStructure - | Ti::RayQuery => {} + self.trace_type(ty, |x, y| { x.types_used.insert(y); }); + } + } - // Types that do contain handles. - Ti::Pointer { base, space: _ } - | Ti::Array { - base, - size: _, - stride: _, - } - | Ti::BindingArray { base, size: _ } => { - self.types_used.insert(base); - } - Ti::Struct { - ref members, - span: _, - } => { - self.types_used.insert_iter(members.iter().map(|m| m.ty)); + pub fn trace_type( + &mut self, + ty: &crate::Type, + callback: impl Fn(&mut Self, Handle) + ) { + use crate::TypeInner as Ti; + match ty.inner { + // Types that do not contain handles. + Ti::Scalar { .. } + | Ti::Vector { .. } + | Ti::Matrix { .. } + | Ti::Atomic { .. } + | Ti::ValuePointer { .. } + | Ti::Image { .. } + | Ti::Sampler { .. } + | Ti::AccelerationStructure + | Ti::RayQuery => {} + + // Types that do contain handles. + Ti::Array { + base, + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(expr)), + stride: _, + } + | Ti::BindingArray { + base, + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(expr)) + } => { + self.expressions_used.insert(expr); + callback(self, base); + } + Ti::Pointer { base, space: _ } + | Ti::Array { + base, + size: _, + stride: _, + } + | Ti::BindingArray { base, size: _ } => { + callback(self, base); + } + Ti::Struct { + ref members, + span: _, + } => { + for m in members.iter() { + callback(self, m.ty); } } } From 364b74e1354ac79e07730bd72172d59e66c436a8 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Sun, 29 Dec 2024 18:42:54 -0800 Subject: [PATCH 02/37] broken tandem test --- naga/src/compact/mod.rs | 59 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 40ee5680a3..927be417bb 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -404,3 +404,62 @@ impl From> for FunctionMap { } } } + +#[test] +fn type_expression_interdependence() { + let mut module: crate::Module = Default::default(); + let u32 = module.types.insert(crate::Type { + name: None, + inner: crate::TypeInner::Scalar(crate::Scalar { + kind: crate::ScalarKind::Uint, + width: 4, + })}, crate::Span::default()); + let expr = module.global_expressions.append( + crate::Expression::Literal(crate::Literal::U32(0)), + crate::Span::default()); + let type_needs_expression = |module: &mut crate::Module, handle| module.types.insert( + crate::Type { name: None, inner: crate::TypeInner::Array { + base: u32, + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(handle)), + stride: 4, + }}, crate::Span::default()); + let expression_needs_type = |module: &mut crate::Module, handle| + module.global_expressions.append( + crate::Expression::ZeroValue(handle), crate::Span::default()); + let expression_needs_expression = |module: &mut crate::Module, handle| + module.global_expressions.append( + crate::Expression::Load { pointer: handle }, crate::Span::default()); + let type_needs_type = |module: &mut crate::Module, handle| module.types.insert( + crate::Type { name: None, inner: crate::TypeInner::Array { + base: handle, size: crate::ArraySize::Dynamic, stride: 0, + }}, crate::Span::default()); + let mut type_name_counter = 0; + let mut type_needed = |module: &mut crate::Module, handle| { + let name = Some(format!("type{}", type_name_counter)); + type_name_counter += 1; + module.types.insert(crate::Type { name, inner: crate::TypeInner::Array { + base: handle, size: crate::ArraySize::Dynamic, stride: 0, + }}, crate::Span::default())}; + let mut override_name_counter = 0; + let mut expression_needed = |module: &mut crate::Module, handle| { + let name = Some(format!("override{}", override_name_counter)); + override_name_counter += 1; + module.overrides.append(crate::Override { + name, + id: None, + ty: u32, + init: Some(handle), + }, crate::Span::default())}; + let tmp0 = type_needs_expression(&mut module, expr); + let tmp1 = type_needs_type(&mut module, tmp0); + let tmp2 = expression_needs_type(&mut module, tmp1); + expression_needed(&mut module, tmp2); + let tmp3 = expression_needs_type(&mut module, u32); + let tmp4 = expression_needs_expression(&mut module, tmp3); + let tmp5 = type_needs_expression(&mut module, tmp4); + type_needed(&mut module, tmp5); + eprintln!("{:?}", module); + compact(&mut module); + eprintln!("{:?}", module); + assert!(false); +} From 3b56706743bda043f2e0adcd6cd1747eddf625ef Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 13:46:09 -0800 Subject: [PATCH 03/37] `fmt` --- naga/src/compact/mod.rs | 104 ++++++++++++++++++++++++++------------ naga/src/compact/types.rs | 8 +-- 2 files changed, 77 insertions(+), 35 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 927be417bb..0517901040 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -327,7 +327,8 @@ impl<'module> ModuleTracer<'module> { types: &self.module.types, types_used: &mut self.types_used, expressions_used: &mut self.global_expressions_used, - }.trace_type(&self.module.types[x], handle2type) + } + .trace_type(&self.module.types[x], handle2type) } fn as_const_expression(&mut self) -> expressions::ExpressionTracer { @@ -408,48 +409,87 @@ impl From> for FunctionMap { #[test] fn type_expression_interdependence() { let mut module: crate::Module = Default::default(); - let u32 = module.types.insert(crate::Type { - name: None, - inner: crate::TypeInner::Scalar(crate::Scalar { - kind: crate::ScalarKind::Uint, - width: 4, - })}, crate::Span::default()); + let u32 = module.types.insert( + crate::Type { + name: None, + inner: crate::TypeInner::Scalar(crate::Scalar { + kind: crate::ScalarKind::Uint, + width: 4, + }), + }, + crate::Span::default(), + ); let expr = module.global_expressions.append( crate::Expression::Literal(crate::Literal::U32(0)), - crate::Span::default()); - let type_needs_expression = |module: &mut crate::Module, handle| module.types.insert( - crate::Type { name: None, inner: crate::TypeInner::Array { - base: u32, - size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(handle)), - stride: 4, - }}, crate::Span::default()); - let expression_needs_type = |module: &mut crate::Module, handle| - module.global_expressions.append( - crate::Expression::ZeroValue(handle), crate::Span::default()); - let expression_needs_expression = |module: &mut crate::Module, handle| + crate::Span::default(), + ); + let type_needs_expression = |module: &mut crate::Module, handle| { + module.types.insert( + crate::Type { + name: None, + inner: crate::TypeInner::Array { + base: u32, + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(handle)), + stride: 4, + }, + }, + crate::Span::default(), + ) + }; + let expression_needs_type = |module: &mut crate::Module, handle| { + module + .global_expressions + .append(crate::Expression::ZeroValue(handle), crate::Span::default()) + }; + let expression_needs_expression = |module: &mut crate::Module, handle| { module.global_expressions.append( - crate::Expression::Load { pointer: handle }, crate::Span::default()); - let type_needs_type = |module: &mut crate::Module, handle| module.types.insert( - crate::Type { name: None, inner: crate::TypeInner::Array { - base: handle, size: crate::ArraySize::Dynamic, stride: 0, - }}, crate::Span::default()); + crate::Expression::Load { pointer: handle }, + crate::Span::default(), + ) + }; + let type_needs_type = |module: &mut crate::Module, handle| { + module.types.insert( + crate::Type { + name: None, + inner: crate::TypeInner::Array { + base: handle, + size: crate::ArraySize::Dynamic, + stride: 0, + }, + }, + crate::Span::default(), + ) + }; let mut type_name_counter = 0; let mut type_needed = |module: &mut crate::Module, handle| { let name = Some(format!("type{}", type_name_counter)); type_name_counter += 1; - module.types.insert(crate::Type { name, inner: crate::TypeInner::Array { - base: handle, size: crate::ArraySize::Dynamic, stride: 0, - }}, crate::Span::default())}; + module.types.insert( + crate::Type { + name, + inner: crate::TypeInner::Array { + base: handle, + size: crate::ArraySize::Dynamic, + stride: 0, + }, + }, + crate::Span::default(), + ) + }; let mut override_name_counter = 0; let mut expression_needed = |module: &mut crate::Module, handle| { let name = Some(format!("override{}", override_name_counter)); override_name_counter += 1; - module.overrides.append(crate::Override { - name, - id: None, - ty: u32, - init: Some(handle), - }, crate::Span::default())}; + module.overrides.append( + crate::Override { + name, + id: None, + ty: u32, + init: Some(handle), + }, + crate::Span::default(), + ) + }; let tmp0 = type_needs_expression(&mut module, expr); let tmp1 = type_needs_type(&mut module, tmp0); let tmp2 = expression_needs_type(&mut module, tmp1); diff --git a/naga/src/compact/types.rs b/naga/src/compact/types.rs index dee2865599..720ee72e86 100644 --- a/naga/src/compact/types.rs +++ b/naga/src/compact/types.rs @@ -25,14 +25,16 @@ impl TypeTracer<'_> { continue; } - self.trace_type(ty, |x, y| { x.types_used.insert(y); }); + self.trace_type(ty, |x, y| { + x.types_used.insert(y); + }); } } pub fn trace_type( &mut self, ty: &crate::Type, - callback: impl Fn(&mut Self, Handle) + callback: impl Fn(&mut Self, Handle), ) { use crate::TypeInner as Ti; match ty.inner { @@ -55,7 +57,7 @@ impl TypeTracer<'_> { } | Ti::BindingArray { base, - size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(expr)) + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(expr)), } => { self.expressions_used.insert(expr); callback(self, base); From cbd991e3440540a7ec7131bfece25082780d4627 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 14:35:29 -0800 Subject: [PATCH 04/37] tandem explanation --- naga/src/compact/mod.rs | 95 ++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 39 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 0517901040..b1bda2ede3 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -102,41 +102,39 @@ pub fn compact(module: &mut crate::Module) { }) .collect(); - module_tracer.type_expression_tandem(|module_tracer| { - // Given that the above steps have marked all the constant - // expressions used directly by globals, constants, functions, and - // entry points, walk the constant expression arena to find all - // constant expressions used, directly or indirectly. - - // Constants' initializers are taken care of already, because - // expression tracing sees through constants. But we still need to - // note type usage. - for (handle, constant) in module.constants.iter() { - if module_tracer.constants_used.contains(handle) { - module_tracer.types_used_insert(constant.ty); - } + // Given that the above steps have marked all the constant + // expressions used directly by globals, constants, functions, and + // entry points, walk the constant expression arena to find all + // constant expressions used, directly or indirectly. + module_tracer.as_const_expression().trace_expressions(); + + // Constants' initializers are taken care of already, because + // expression tracing sees through constants. But we still need to + // note type usage. + for (handle, constant) in module.constants.iter() { + if module_tracer.constants_used.contains(handle) { + module_tracer.types_used.insert(constant.ty); } + } - // Treat all named types as used. - for (handle, ty) in module.types.iter().rev() { - log::trace!("tracing type {:?}, name {:?}", handle, ty.name); - if ty.name.is_some() { - module_tracer.types_used_insert(handle); - } - } - }, |module_tracer| { - for (handle, ty) in module.types.iter() { - if module_tracer.types_used.contains(handle) { - if let crate::TypeInner::Array { - size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(size_expr)), - .. - } = ty.inner - { - module_tracer.global_expressions_used.insert(size_expr); - } - } + // Treat all named types as used. + for (handle, ty) in module.types.iter() { + log::trace!("tracing type {:?}, name {:?}", handle, ty.name); + if ty.name.is_some() { + module_tracer.types_used.insert(handle); } - }); + } + + // Propagate usage through types. + module_tracer.as_type().trace_types(); + + module_tracer.type_expression_tandem(); + + eprintln!( + "expr\n{:#?}\n\nty\n{:#?}", + module_tracer.global_expressions_used, + module_tracer.types_used + ); // Now that we know what is used and what is never touched, // produce maps from the `Handle`s that appear in `module` now to @@ -300,16 +298,35 @@ impl<'module> ModuleTracer<'module> { } } - fn type_expression_tandem(&mut self, e2t: impl FnOnce(&mut Self), t2e: impl FnOnce(&mut Self)) { - t2e(self); - self.as_const_expression().trace_expressions(); - e2t(self); - - // Propagate usage through types. - self.as_type().trace_types(); + fn type_expression_tandem(&mut self) { + // assume that the expressions are well ordered since they're not merged like types are + // ie. expression A referring to a type referring to expression B has A > B. + // this needs to be checked in the validator (TODO) + + // 1. for any type marked as used, mark its expressions as used + // 2. for any expression marked as used and referencing a type (while iterating backwards) + // a. walk the type's dependency tree + // i. mark the types and their referenced expressions as used + + // ┌───────────┐ ┌───────────┐ + // │Expressions│ │ Types │ + // │ │ │ │ + // │ covered by │ │ + // │ step 1 │ │ + // │ ◄────┬─┬─────┘ │ + // │ │ │ │ + // │ │ │ │ + // │ │ covered by │ + // │ │ step 2 │ + // │ └─────┬─┬────►│ │ + // │ │ │ │ │ + // │ ◄────┼─┼─────┘ │ + // │ │ │ │ + // └───────────┘ └───────────┘ } fn types_used_insert(&mut self, x: crate::Handle) -> bool { + eprintln!("tracing ty {:#?}", x); self.trace_type(x); self.types_used.insert(x) } From e41f5f3d59bc5abd631654f3c5e678586e81195b Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 14:40:52 -0800 Subject: [PATCH 05/37] explain tmp intermediaries --- naga/src/compact/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index b1bda2ede3..ed08842ada 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -507,6 +507,7 @@ fn type_expression_interdependence() { crate::Span::default(), ) }; + // borrow checker breaks without the tmp variables as of Rust 1.83.0 let tmp0 = type_needs_expression(&mut module, expr); let tmp1 = type_needs_type(&mut module, tmp0); let tmp2 = expression_needs_type(&mut module, tmp1); From 63b929083c26205f120fad37e17ecf8416782726 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 17:51:32 -0800 Subject: [PATCH 06/37] move all tracing into tandem fn --- naga/src/compact/mod.rs | 44 +++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index ed08842ada..d463fbc068 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -102,12 +102,6 @@ pub fn compact(module: &mut crate::Module) { }) .collect(); - // Given that the above steps have marked all the constant - // expressions used directly by globals, constants, functions, and - // entry points, walk the constant expression arena to find all - // constant expressions used, directly or indirectly. - module_tracer.as_const_expression().trace_expressions(); - // Constants' initializers are taken care of already, because // expression tracing sees through constants. But we still need to // note type usage. @@ -125,9 +119,6 @@ pub fn compact(module: &mut crate::Module) { } } - // Propagate usage through types. - module_tracer.as_type().trace_types(); - module_tracer.type_expression_tandem(); eprintln!( @@ -299,30 +290,41 @@ impl<'module> ModuleTracer<'module> { } fn type_expression_tandem(&mut self) { + // assume there are no cycles in the type/expression graph (guaranteed by validator) // assume that the expressions are well ordered since they're not merged like types are // ie. expression A referring to a type referring to expression B has A > B. - // this needs to be checked in the validator (TODO) - // 1. for any type marked as used, mark its expressions as used - // 2. for any expression marked as used and referencing a type (while iterating backwards) - // a. walk the type's dependency tree - // i. mark the types and their referenced expressions as used + // TODO: check dependency ordering in validator + + // 1. iterate over types, skipping unused ones + // a. if the type references an expression, mark it used + // b. repeat `a` while walking referenced types, marking them as used + // 2. iterate backwards over expressions, skipping unused ones + // a. if the expression references a type + // i. walk the type's dependency tree, marking the types and their referenced + // expressions as used + // b. if the expression references another expression, mark the latter as used // ┌───────────┐ ┌───────────┐ // │Expressions│ │ Types │ // │ │ │ │ - // │ covered by │ │ - // │ step 1 │ │ - // │ ◄────┬─┬─────┘ │ + // │ covered by │ │ So that back/forths starting with a type now start with an + // │ step 1 │ │ expression instead. + // │ ◄────┴─┴─────┘ │ // │ │ │ │ // │ │ │ │ - // │ │ covered by │ - // │ │ step 2 │ - // │ └─────┬─┬────►│ │ + // │ ◄────┼─┼─────┐ │ This arrow is only as needed. // │ │ │ │ │ - // │ ◄────┼─┼─────┘ │ + // │ ┌─────┴─┴────►│ │ + // │ │ covered by │ This covers back/forths starting with an expression. + // │ │ step 2 │ // │ │ │ │ // └───────────┘ └───────────┘ + + // 1 + module_tracer.as_type().trace_types(); + // 2b + module_tracer.as_const_expression().trace_expressions(); } fn types_used_insert(&mut self, x: crate::Handle) -> bool { From 4009251f5c3634d33fbc34938e9c273cb38fe060 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 17:55:57 -0800 Subject: [PATCH 07/37] fn guide --- naga/src/compact/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index d463fbc068..c0950555eb 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -302,23 +302,23 @@ impl<'module> ModuleTracer<'module> { // 2. iterate backwards over expressions, skipping unused ones // a. if the expression references a type // i. walk the type's dependency tree, marking the types and their referenced - // expressions as used + // expressions as used (types_used_insert instead of types_used.insert) // b. if the expression references another expression, mark the latter as used // ┌───────────┐ ┌───────────┐ // │Expressions│ │ Types │ - // │ │ │ │ + // │ ╵ ╵ │ // │ covered by │ │ So that back/forths starting with a type now start with an // │ step 1 │ │ expression instead. - // │ ◄────┴─┴─────┘ │ + // │ ◄────────────┘ │ // │ │ │ │ // │ │ │ │ - // │ ◄────┼─┼─────┐ │ This arrow is only as needed. + // │ ◄────────────┐ │ This arrow is only as needed. // │ │ │ │ │ - // │ ┌─────┴─┴────►│ │ + // │ ┌────────────►│ │ // │ │ covered by │ This covers back/forths starting with an expression. // │ │ step 2 │ - // │ │ │ │ + // │ ╷ ╷ │ // └───────────┘ └───────────┘ // 1 From d412b3e16c1c22cf07c2b338f390a848f56846b1 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 18:43:09 -0800 Subject: [PATCH 08/37] move types_used_insert into ExpressionTracer --- naga/src/compact/expressions.rs | 29 +++++++++++++++++++++++++---- naga/src/compact/functions.rs | 1 + naga/src/compact/mod.rs | 26 ++++---------------------- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index 7e611c35fe..b861cbc682 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -1,7 +1,9 @@ use super::{HandleMap, HandleSet, ModuleMap}; -use crate::arena::{Arena, Handle}; +use crate::arena::{UniqueArena, Arena, Handle}; +use crate::compact::types::{TypeTracer}; pub struct ExpressionTracer<'tracer> { + pub types: Option<&'tracer UniqueArena>, pub constants: &'tracer Arena, /// The arena in which we are currently tracing expressions. @@ -28,6 +30,25 @@ pub struct ExpressionTracer<'tracer> { } impl ExpressionTracer<'_> { + fn types_used_insert(&mut self, x: Handle) -> bool { + if self.types.is_some() { + self.trace_type(x); + } + self.types_used.insert(x) + } + + fn trace_type(&mut self, x: Handle) { + fn handle2type(x: &mut TypeTracer, y: Handle) { + x.trace_type(&x.types[y], handle2type); + } + TypeTracer { + types: &self.types.unwrap(), + types_used: &mut self.types_used, + expressions_used: &mut self.expressions_used, + } + .trace_type(&self.types.unwrap()[x], handle2type) + } + /// Propagate usage through `self.expressions`, starting with `self.expressions_used`. /// /// Treat `self.expressions_used` as the initial set of "known @@ -95,10 +116,10 @@ impl ExpressionTracer<'_> { // `compact::compact`, so we have no more work to do here. } Ex::ZeroValue(ty) => { - self.types_used.insert(ty); + self.types_used_insert(ty); } Ex::Compose { ty, ref components } => { - self.types_used.insert(ty); + self.types_used_insert(ty); self.expressions_used .insert_iter(components.iter().cloned()); } @@ -215,7 +236,7 @@ impl ExpressionTracer<'_> { Ex::AtomicResult { ty, comparison: _ } | Ex::WorkGroupUniformLoadResult { ty } | Ex::SubgroupOperationResult { ty } => { - self.types_used.insert(ty); + self.types_used_insert(ty); } Ex::RayQueryGetIntersection { query, diff --git a/naga/src/compact/functions.rs b/naga/src/compact/functions.rs index d523c1889f..9ab7bd792a 100644 --- a/naga/src/compact/functions.rs +++ b/naga/src/compact/functions.rs @@ -46,6 +46,7 @@ impl FunctionTracer<'_> { fn as_expression(&mut self) -> super::expressions::ExpressionTracer { super::expressions::ExpressionTracer { + types: None, constants: self.constants, expressions: &self.function.expressions, diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index c0950555eb..d248dcbcfa 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -257,10 +257,6 @@ struct ModuleTracer<'module> { global_expressions_used: HandleSet, } -fn handle2type(x: &mut types::TypeTracer, y: crate::Handle) { - x.trace_type(&x.types[y], handle2type); -} - impl<'module> ModuleTracer<'module> { fn new(module: &'module crate::Module) -> Self { Self { @@ -322,15 +318,9 @@ impl<'module> ModuleTracer<'module> { // └───────────┘ └───────────┘ // 1 - module_tracer.as_type().trace_types(); - // 2b - module_tracer.as_const_expression().trace_expressions(); - } - - fn types_used_insert(&mut self, x: crate::Handle) -> bool { - eprintln!("tracing ty {:#?}", x); - self.trace_type(x); - self.types_used.insert(x) + self.as_type().trace_types(); + // 2 + self.as_const_expression().trace_expressions(); } fn as_type(&mut self) -> types::TypeTracer { @@ -341,17 +331,9 @@ impl<'module> ModuleTracer<'module> { } } - fn trace_type(&mut self, x: crate::Handle) { - types::TypeTracer { - types: &self.module.types, - types_used: &mut self.types_used, - expressions_used: &mut self.global_expressions_used, - } - .trace_type(&self.module.types[x], handle2type) - } - fn as_const_expression(&mut self) -> expressions::ExpressionTracer { expressions::ExpressionTracer { + types: Some(&self.module.types), expressions: &self.module.global_expressions, constants: &self.module.constants, types_used: &mut self.types_used, From cc2cbfebdc0e3ee8edfb5f7292d1a0f1274e35a3 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 19:13:22 -0800 Subject: [PATCH 09/37] mark traced types as used --- naga/src/compact/expressions.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index b861cbc682..78ed2bf347 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -30,15 +30,16 @@ pub struct ExpressionTracer<'tracer> { } impl ExpressionTracer<'_> { - fn types_used_insert(&mut self, x: Handle) -> bool { + fn types_used_insert(&mut self, x: Handle) { if self.types.is_some() { self.trace_type(x); } - self.types_used.insert(x) + self.types_used.insert(x); } fn trace_type(&mut self, x: Handle) { fn handle2type(x: &mut TypeTracer, y: Handle) { + x.types_used.insert(y); x.trace_type(&x.types[y], handle2type); } TypeTracer { From 18a54e4f1de69d8ab78647f41d399e5b56bad05c Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 20:16:00 -0800 Subject: [PATCH 10/37] good-enough check that doesn't modify crate::Module --- naga/src/compact/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index fbab8b5157..5ebf2e2407 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -476,8 +476,7 @@ fn type_expression_interdependence() { let tmp4 = expression_needs_expression(&mut module, tmp3); let tmp5 = type_needs_expression(&mut module, tmp4); type_needed(&mut module, tmp5); - eprintln!("{:?}", module); + let b4 = format!("{:?}", module); compact(&mut module); - eprintln!("{:?}", module); - assert!(false); + assert!(b4 == format!("{:?}", module)); } From 5ead8a3d326041e95b12d1948c06b72951a97050 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Mon, 30 Dec 2024 20:31:35 -0800 Subject: [PATCH 11/37] remove debugging statements --- naga/src/compact/mod.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 5ebf2e2407..b3b5c96613 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -121,12 +121,6 @@ pub fn compact(module: &mut crate::Module) { module_tracer.type_expression_tandem(); - eprintln!( - "expr\n{:#?}\n\nty\n{:#?}", - module_tracer.global_expressions_used, - module_tracer.types_used - ); - // Now that we know what is used and what is never touched, // produce maps from the `Handle`s that appear in `module` now to // the corresponding `Handle`s that will refer to the same items From c9c38b2e850def57098c9d87e62eebc0ce1563a0 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Thu, 2 Jan 2025 21:23:00 -0800 Subject: [PATCH 12/37] validate order as defined in #6788 (mostly via #6800) --- naga/src/valid/handles.rs | 41 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index be4eb3dbac..913225daa5 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -788,3 +788,44 @@ fn constant_deps() { .is_err()); } } + +#[test] +fn well_ordered_expressions() { + use super::Validator; + use crate::{ArraySize, Expression, PendingArraySize, Scalar, Span, Type, TypeInner, Literal}; + + let nowhere = Span::default(); + + let mut m = crate::Module::default(); + + let ty_u32 = m.types.insert( + Type { + name: Some("u32".to_string()), + inner: TypeInner::Scalar(Scalar::U32), + }, + nowhere, + ); + let ex_zero = m + .global_expressions + .append(Expression::ZeroValue(ty_u32), nowhere); + let expr = m.global_expressions.append(Expression::Literal(Literal::U32(0)), nowhere); + let ty_arr = m.types.insert( + Type { + name: Some("bad_array".to_string()), + inner: TypeInner::Array { + base: ty_u32, + size: ArraySize::Pending(PendingArraySize::Expression(expr)), + stride: 4, + }, + }, + nowhere, + ); + + // Everything should be okay now. + assert!(Validator::validate_module_handles(&m).is_ok()); + + // Mutate `ex_zero`'s type to `ty_arr`, introducing an out of order dependency. + // Validation should catch the problem. + m.global_expressions[ex_zero] = Expression::ZeroValue(ty_arr); + assert!(Validator::validate_module_handles(&m).is_err()); +} From babbb8620c2dfe47dee96a9813965b70558b47ea Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Fri, 3 Jan 2025 12:33:09 -0800 Subject: [PATCH 13/37] insert order check --- naga/src/compact/expressions.rs | 313 ++++++++++++++++---------------- naga/src/compact/mod.rs | 5 +- naga/src/valid/handles.rs | 26 ++- 3 files changed, 185 insertions(+), 159 deletions(-) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index 78ed2bf347..be15e709c6 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -84,168 +84,171 @@ impl ExpressionTracer<'_> { } log::trace!("tracing new expression {:?}", expr); + self.trace_expression(expr); + } + } - use crate::Expression as Ex; - match *expr { - // Expressions that do not contain handles that need to be traced. - Ex::Literal(_) - | Ex::FunctionArgument(_) - | Ex::GlobalVariable(_) - | Ex::LocalVariable(_) - | Ex::CallResult(_) - | Ex::SubgroupBallotResult - | Ex::RayQueryProceedResult => {} + pub fn trace_expression(&mut self, expr: &crate::Expression) { + use crate::Expression as Ex; + match *expr { + // Expressions that do not contain handles that need to be traced. + Ex::Literal(_) + | Ex::FunctionArgument(_) + | Ex::GlobalVariable(_) + | Ex::LocalVariable(_) + | Ex::CallResult(_) + | Ex::SubgroupBallotResult + | Ex::RayQueryProceedResult => {} - Ex::Constant(handle) => { - self.constants_used.insert(handle); - // Constants and expressions are mutually recursive, which - // complicates our nice one-pass algorithm. However, since - // constants don't refer to each other, we can get around - // this by looking *through* each constant and marking its - // initializer as used. Since `expr` refers to the constant, - // and the constant refers to the initializer, it must - // precede `expr` in the arena. - let init = self.constants[handle].init; - match self.global_expressions_used { - Some(ref mut used) => used.insert(init), - None => self.expressions_used.insert(init), - }; - } - Ex::Override(_) => { - // All overrides are considered used by definition. We mark - // their types and initialization expressions as used in - // `compact::compact`, so we have no more work to do here. - } - Ex::ZeroValue(ty) => { - self.types_used_insert(ty); - } - Ex::Compose { ty, ref components } => { - self.types_used_insert(ty); - self.expressions_used - .insert_iter(components.iter().cloned()); - } - Ex::Access { base, index } => self.expressions_used.insert_iter([base, index]), - Ex::AccessIndex { base, index: _ } => { - self.expressions_used.insert(base); - } - Ex::Splat { size: _, value } => { - self.expressions_used.insert(value); - } - Ex::Swizzle { - size: _, - vector, - pattern: _, - } => { - self.expressions_used.insert(vector); - } - Ex::Load { pointer } => { - self.expressions_used.insert(pointer); - } - Ex::ImageSample { - image, - sampler, - gather: _, - coordinate, - array_index, - offset, - ref level, - depth_ref, - } => { - self.expressions_used - .insert_iter([image, sampler, coordinate]); - self.expressions_used.insert_iter(array_index); - match self.global_expressions_used { - Some(ref mut used) => used.insert_iter(offset), - None => self.expressions_used.insert_iter(offset), - } - use crate::SampleLevel as Sl; - match *level { - Sl::Auto | Sl::Zero => {} - Sl::Exact(expr) | Sl::Bias(expr) => { - self.expressions_used.insert(expr); - } - Sl::Gradient { x, y } => self.expressions_used.insert_iter([x, y]), - } - self.expressions_used.insert_iter(depth_ref); - } - Ex::ImageLoad { - image, - coordinate, - array_index, - sample, - level, - } => { - self.expressions_used.insert(image); - self.expressions_used.insert(coordinate); - self.expressions_used.insert_iter(array_index); - self.expressions_used.insert_iter(sample); - self.expressions_used.insert_iter(level); + Ex::Constant(handle) => { + self.constants_used.insert(handle); + // Constants and expressions are mutually recursive, which + // complicates our nice one-pass algorithm. However, since + // constants don't refer to each other, we can get around + // this by looking *through* each constant and marking its + // initializer as used. Since `expr` refers to the constant, + // and the constant refers to the initializer, it must + // precede `expr` in the arena. + let init = self.constants[handle].init; + match self.global_expressions_used { + Some(ref mut used) => used.insert(init), + None => self.expressions_used.insert(init), + }; + } + Ex::Override(_) => { + // All overrides are considered used by definition. We mark + // their types and initialization expressions as used in + // `compact::compact`, so we have no more work to do here. + } + Ex::ZeroValue(ty) => { + self.types_used_insert(ty); + } + Ex::Compose { ty, ref components } => { + self.types_used_insert(ty); + self.expressions_used + .insert_iter(components.iter().cloned()); + } + Ex::Access { base, index } => self.expressions_used.insert_iter([base, index]), + Ex::AccessIndex { base, index: _ } => { + self.expressions_used.insert(base); + } + Ex::Splat { size: _, value } => { + self.expressions_used.insert(value); + } + Ex::Swizzle { + size: _, + vector, + pattern: _, + } => { + self.expressions_used.insert(vector); + } + Ex::Load { pointer } => { + self.expressions_used.insert(pointer); + } + Ex::ImageSample { + image, + sampler, + gather: _, + coordinate, + array_index, + offset, + ref level, + depth_ref, + } => { + self.expressions_used + .insert_iter([image, sampler, coordinate]); + self.expressions_used.insert_iter(array_index); + match self.global_expressions_used { + Some(ref mut used) => used.insert_iter(offset), + None => self.expressions_used.insert_iter(offset), } - Ex::ImageQuery { image, ref query } => { - self.expressions_used.insert(image); - use crate::ImageQuery as Iq; - match *query { - Iq::Size { level } => self.expressions_used.insert_iter(level), - Iq::NumLevels | Iq::NumLayers | Iq::NumSamples => {} + use crate::SampleLevel as Sl; + match *level { + Sl::Auto | Sl::Zero => {} + Sl::Exact(expr) | Sl::Bias(expr) => { + self.expressions_used.insert(expr); } + Sl::Gradient { x, y } => self.expressions_used.insert_iter([x, y]), } - Ex::Unary { op: _, expr } => { - self.expressions_used.insert(expr); - } - Ex::Binary { op: _, left, right } => { - self.expressions_used.insert_iter([left, right]); - } - Ex::Select { - condition, - accept, - reject, - } => self - .expressions_used - .insert_iter([condition, accept, reject]), - Ex::Derivative { - axis: _, - ctrl: _, - expr, - } => { - self.expressions_used.insert(expr); - } - Ex::Relational { fun: _, argument } => { - self.expressions_used.insert(argument); - } - Ex::Math { - fun: _, - arg, - arg1, - arg2, - arg3, - } => { - self.expressions_used.insert(arg); - self.expressions_used.insert_iter(arg1); - self.expressions_used.insert_iter(arg2); - self.expressions_used.insert_iter(arg3); - } - Ex::As { - expr, - kind: _, - convert: _, - } => { - self.expressions_used.insert(expr); - } - Ex::ArrayLength(expr) => { - self.expressions_used.insert(expr); - } - Ex::AtomicResult { ty, comparison: _ } - | Ex::WorkGroupUniformLoadResult { ty } - | Ex::SubgroupOperationResult { ty } => { - self.types_used_insert(ty); - } - Ex::RayQueryGetIntersection { - query, - committed: _, - } => { - self.expressions_used.insert(query); + self.expressions_used.insert_iter(depth_ref); + } + Ex::ImageLoad { + image, + coordinate, + array_index, + sample, + level, + } => { + self.expressions_used.insert(image); + self.expressions_used.insert(coordinate); + self.expressions_used.insert_iter(array_index); + self.expressions_used.insert_iter(sample); + self.expressions_used.insert_iter(level); + } + Ex::ImageQuery { image, ref query } => { + self.expressions_used.insert(image); + use crate::ImageQuery as Iq; + match *query { + Iq::Size { level } => self.expressions_used.insert_iter(level), + Iq::NumLevels | Iq::NumLayers | Iq::NumSamples => {} } } + Ex::Unary { op: _, expr } => { + self.expressions_used.insert(expr); + } + Ex::Binary { op: _, left, right } => { + self.expressions_used.insert_iter([left, right]); + } + Ex::Select { + condition, + accept, + reject, + } => self + .expressions_used + .insert_iter([condition, accept, reject]), + Ex::Derivative { + axis: _, + ctrl: _, + expr, + } => { + self.expressions_used.insert(expr); + } + Ex::Relational { fun: _, argument } => { + self.expressions_used.insert(argument); + } + Ex::Math { + fun: _, + arg, + arg1, + arg2, + arg3, + } => { + self.expressions_used.insert(arg); + self.expressions_used.insert_iter(arg1); + self.expressions_used.insert_iter(arg2); + self.expressions_used.insert_iter(arg3); + } + Ex::As { + expr, + kind: _, + convert: _, + } => { + self.expressions_used.insert(expr); + } + Ex::ArrayLength(expr) => { + self.expressions_used.insert(expr); + } + Ex::AtomicResult { ty, comparison: _ } + | Ex::WorkGroupUniformLoadResult { ty } + | Ex::SubgroupOperationResult { ty } => { + self.types_used_insert(ty); + } + Ex::RayQueryGetIntersection { + query, + committed: _, + } => { + self.expressions_used.insert(query); + } } } } diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index b3b5c96613..88fa719df6 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -1,4 +1,4 @@ -mod expressions; +pub mod expressions; mod functions; mod handle_set_map; mod statements; @@ -259,8 +259,7 @@ impl<'module> ModuleTracer<'module> { // assume there are no cycles in the type/expression graph (guaranteed by validator) // assume that the expressions are well ordered since they're not merged like types are // ie. expression A referring to a type referring to expression B has A > B. - - // TODO: check dependency ordering in validator + // (also guaranteed by validator) // 1. iterate over types, skipping unused ones // a. if the type references an expression, mark it used diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 913225daa5..36bddb9a6d 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -1,8 +1,9 @@ //! Implementation of `Validator::validate_module_handles`. use crate::{ - arena::{BadHandle, BadRangeError}, + arena::{HandleSet, BadHandle, BadRangeError}, diagnostic_filter::DiagnosticFilterNode, + compact::expressions::ExpressionTracer, Handle, }; @@ -74,6 +75,7 @@ impl super::Validator { for handle_and_expr in global_expressions.iter() { Self::validate_const_expression_handles(handle_and_expr, constants, overrides, types)?; + Self::well_ordered_deps(handle_and_expr, constants, global_expressions, types)?; } let validate_type = |handle| Self::validate_type_handle(handle, types); @@ -629,6 +631,28 @@ impl super::Validator { | crate::Statement::Barrier(_) => Ok(()), }) } + + pub fn well_ordered_deps( + (handle, expression): (Handle, &crate::Expression), + constants: &Arena, + global_expressions: &Arena, + types: &UniqueArena, + ) -> Result<(), InvalidHandleError> { + let mut exprs = HandleSet::for_arena(global_expressions); + ExpressionTracer { + types: Some(types), + expressions: global_expressions, + constants: constants, + types_used: &mut HandleSet::for_arena(types), + constants_used: &mut HandleSet::for_arena(constants), + expressions_used: &mut exprs, + global_expressions_used: None, + }.trace_expression(expression); + if let Err(error) = handle.check_dep_iter(exprs.iter()) { + return Err(InvalidHandleError::ForwardDependency(error)); + } + Ok(()) + } } impl From for ValidationError { From c5726e5b2e4eb8fefafd0da803dc96a2b322ee81 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Fri, 3 Jan 2025 20:54:14 -0800 Subject: [PATCH 14/37] `fmt` & `clippy` --- naga/src/compact/expressions.rs | 10 +++++----- naga/src/valid/handles.rs | 15 +++++++++------ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index be15e709c6..b04476838f 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -1,6 +1,6 @@ use super::{HandleMap, HandleSet, ModuleMap}; -use crate::arena::{UniqueArena, Arena, Handle}; -use crate::compact::types::{TypeTracer}; +use crate::arena::{Arena, Handle, UniqueArena}; +use crate::compact::types::TypeTracer; pub struct ExpressionTracer<'tracer> { pub types: Option<&'tracer UniqueArena>, @@ -43,9 +43,9 @@ impl ExpressionTracer<'_> { x.trace_type(&x.types[y], handle2type); } TypeTracer { - types: &self.types.unwrap(), - types_used: &mut self.types_used, - expressions_used: &mut self.expressions_used, + types: self.types.unwrap(), + types_used: self.types_used, + expressions_used: self.expressions_used, } .trace_type(&self.types.unwrap()[x], handle2type) } diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 36bddb9a6d..9374457d99 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -1,9 +1,9 @@ //! Implementation of `Validator::validate_module_handles`. use crate::{ - arena::{HandleSet, BadHandle, BadRangeError}, - diagnostic_filter::DiagnosticFilterNode, + arena::{BadHandle, BadRangeError, HandleSet}, compact::expressions::ExpressionTracer, + diagnostic_filter::DiagnosticFilterNode, Handle, }; @@ -642,12 +642,13 @@ impl super::Validator { ExpressionTracer { types: Some(types), expressions: global_expressions, - constants: constants, + constants, types_used: &mut HandleSet::for_arena(types), constants_used: &mut HandleSet::for_arena(constants), expressions_used: &mut exprs, global_expressions_used: None, - }.trace_expression(expression); + } + .trace_expression(expression); if let Err(error) = handle.check_dep_iter(exprs.iter()) { return Err(InvalidHandleError::ForwardDependency(error)); } @@ -816,7 +817,7 @@ fn constant_deps() { #[test] fn well_ordered_expressions() { use super::Validator; - use crate::{ArraySize, Expression, PendingArraySize, Scalar, Span, Type, TypeInner, Literal}; + use crate::{ArraySize, Expression, Literal, PendingArraySize, Scalar, Span, Type, TypeInner}; let nowhere = Span::default(); @@ -832,7 +833,9 @@ fn well_ordered_expressions() { let ex_zero = m .global_expressions .append(Expression::ZeroValue(ty_u32), nowhere); - let expr = m.global_expressions.append(Expression::Literal(Literal::U32(0)), nowhere); + let expr = m + .global_expressions + .append(Expression::Literal(Literal::U32(0)), nowhere); let ty_arr = m.types.insert( Type { name: Some("bad_array".to_string()), From a3588df26c9c8f477329c2313cfa3eef21643908 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Fri, 3 Jan 2025 21:05:02 -0800 Subject: [PATCH 15/37] ignore well-order requirement for emscripten since it doesn't compact --- naga/src/valid/handles.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 9374457d99..ba85f56b80 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -2,7 +2,6 @@ use crate::{ arena::{BadHandle, BadRangeError, HandleSet}, - compact::expressions::ExpressionTracer, diagnostic_filter::DiagnosticFilterNode, Handle, }; @@ -10,6 +9,9 @@ use crate::{ use crate::non_max_u32::NonMaxU32; use crate::{Arena, UniqueArena}; +#[cfg(feature = "compact")] +use crate::compact::expressions::ExpressionTracer; + use super::ValidationError; use std::{convert::TryInto, hash::Hash}; @@ -639,6 +641,7 @@ impl super::Validator { types: &UniqueArena, ) -> Result<(), InvalidHandleError> { let mut exprs = HandleSet::for_arena(global_expressions); + #[cfg(feature = "compact")] ExpressionTracer { types: Some(types), expressions: global_expressions, From 7f5a858b32f75cacb68616c975397002d6b4aec2 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Fri, 3 Jan 2025 21:13:48 -0800 Subject: [PATCH 16/37] redeclare full check for compact --- naga/src/valid/handles.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index ba85f56b80..eb988ced53 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -634,6 +634,7 @@ impl super::Validator { }) } + #[cfg(feature = "compact")] pub fn well_ordered_deps( (handle, expression): (Handle, &crate::Expression), constants: &Arena, @@ -641,7 +642,6 @@ impl super::Validator { types: &UniqueArena, ) -> Result<(), InvalidHandleError> { let mut exprs = HandleSet::for_arena(global_expressions); - #[cfg(feature = "compact")] ExpressionTracer { types: Some(types), expressions: global_expressions, @@ -657,6 +657,16 @@ impl super::Validator { } Ok(()) } + + #[cfg(not(feature = "compact"))] + pub fn well_ordered_deps( + (_handle, _expression): (Handle, &crate::Expression), + _constants: &Arena, + _global_expressions: &Arena, + _types: &UniqueArena, + ) -> Result<(), InvalidHandleError> { + Ok(()) + } } impl From for ValidationError { From 9c7159b7454657c444aa97a50688a4d5ab212aed Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Fri, 3 Jan 2025 21:17:40 -0800 Subject: [PATCH 17/37] clippy via github actions --- naga/src/valid/handles.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index eb988ced53..fc25aa24a9 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -1,7 +1,7 @@ //! Implementation of `Validator::validate_module_handles`. use crate::{ - arena::{BadHandle, BadRangeError, HandleSet}, + arena::{BadHandle, BadRangeError}, diagnostic_filter::DiagnosticFilterNode, Handle, }; @@ -10,7 +10,7 @@ use crate::non_max_u32::NonMaxU32; use crate::{Arena, UniqueArena}; #[cfg(feature = "compact")] -use crate::compact::expressions::ExpressionTracer; +use crate::{arena::HandleSet, compact::expressions::ExpressionTracer}; use super::ValidationError; @@ -659,7 +659,7 @@ impl super::Validator { } #[cfg(not(feature = "compact"))] - pub fn well_ordered_deps( + pub const fn well_ordered_deps( (_handle, _expression): (Handle, &crate::Expression), _constants: &Arena, _global_expressions: &Arena, From b9a5e7ae532b22d213b36723a497ed03eeaa6c46 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Sat, 4 Jan 2025 09:18:49 -0800 Subject: [PATCH 18/37] changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a39cd68f8d..0df44dc4af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -199,7 +199,8 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] - Fix crash when a texture argument is missing. By @aedm in [#6486](https://github.com/gfx-rs/wgpu/pull/6486) - Emit an error in constant evaluation, rather than crash, in certain cases where `vecN` constructors have less than N arguments. By @ErichDonGubler in [#6508](https://github.com/gfx-rs/wgpu/pull/6508). - +- Fix a leak by ensuring that types that depend on expressions are correctly compacted. By @KentSlaney in [#6806](https://github.com/gfx-rs/wgpu/pull/6806). + ### Examples - Add multiple render targets example. By @kaphula in [#5297](https://github.com/gfx-rs/wgpu/pull/5313) From 5b327667bcdbadfd10428befc81674efe3ee705b Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Sun, 5 Jan 2025 21:35:34 -0800 Subject: [PATCH 19/37] better equality check --- naga/src/arena/unique_arena.rs | 9 +++++---- naga/src/block.rs | 1 + naga/src/compact/mod.rs | 4 ++-- naga/src/diagnostic_filter.rs | 2 ++ naga/src/lib.rs | 10 ++++++++++ naga/src/span.rs | 2 +- 6 files changed, 21 insertions(+), 7 deletions(-) diff --git a/naga/src/arena/unique_arena.rs b/naga/src/arena/unique_arena.rs index c64bb302eb..db4348aa5a 100644 --- a/naga/src/arena/unique_arena.rs +++ b/naga/src/arena/unique_arena.rs @@ -23,7 +23,8 @@ use std::{fmt, hash, ops}; /// /// [`Arena`]: super::Arena #[derive(Clone)] -pub struct UniqueArena { +#[cfg_attr(test, derive(PartialEq))] +pub struct UniqueArena { set: FastIndexSet, /// Spans for the elements, indexed by handle. @@ -35,7 +36,7 @@ pub struct UniqueArena { span_info: Vec, } -impl UniqueArena { +impl UniqueArena { /// Create a new arena with no initial capacity allocated. pub fn new() -> Self { UniqueArena { @@ -182,7 +183,7 @@ impl UniqueArena { } } -impl Default for UniqueArena { +impl Default for UniqueArena { fn default() -> Self { Self::new() } @@ -194,7 +195,7 @@ impl fmt::Debug for UniqueArena { } } -impl ops::Index> for UniqueArena { +impl ops::Index> for UniqueArena { type Output = T; fn index(&self, handle: Handle) -> &T { &self.set[handle.index()] diff --git a/naga/src/block.rs b/naga/src/block.rs index 2e86a928f1..f2d2d027b3 100644 --- a/naga/src/block.rs +++ b/naga/src/block.rs @@ -6,6 +6,7 @@ use std::ops::{Deref, DerefMut, RangeBounds}; #[cfg_attr(feature = "serialize", derive(serde::Serialize))] #[cfg_attr(feature = "serialize", serde(transparent))] #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct Block { body: Vec, #[cfg_attr(feature = "serialize", serde(skip))] diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 88fa719df6..a4b1a3149b 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -469,7 +469,7 @@ fn type_expression_interdependence() { let tmp4 = expression_needs_expression(&mut module, tmp3); let tmp5 = type_needs_expression(&mut module, tmp4); type_needed(&mut module, tmp5); - let b4 = format!("{:?}", module); + let b4 = module.clone(); compact(&mut module); - assert!(b4 == format!("{:?}", module)); + assert!(b4 == module); } diff --git a/naga/src/diagnostic_filter.rs b/naga/src/diagnostic_filter.rs index 2fa5464cdf..e3062bfa52 100644 --- a/naga/src/diagnostic_filter.rs +++ b/naga/src/diagnostic_filter.rs @@ -94,6 +94,7 @@ impl StandardFilterableTriggeringRule { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct DiagnosticFilter { pub new_severity: Severity, pub triggering_rule: FilterableTriggeringRule, @@ -235,6 +236,7 @@ pub(crate) struct ConflictingDiagnosticRuleError { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct DiagnosticFilterNode { pub inner: DiagnosticFilter, pub parent: Option>, diff --git a/naga/src/lib.rs b/naga/src/lib.rs index 687dc5b441..34f22e622a 100644 --- a/naga/src/lib.rs +++ b/naga/src/lib.rs @@ -999,6 +999,7 @@ pub struct GlobalVariable { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct LocalVariable { /// Name of the variable, if any. pub name: Option, @@ -1720,6 +1721,7 @@ pub enum SwitchValue { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct SwitchCase { /// Value, upon which the case is considered true. pub value: SwitchValue, @@ -1738,6 +1740,7 @@ pub struct SwitchCase { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub enum RayQueryFunction { /// Initialize the `RayQuery` object. Initialize { @@ -1782,6 +1785,7 @@ pub enum RayQueryFunction { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub enum Statement { /// Emit a range of expressions, visible to all statements that follow in this block. /// @@ -2077,6 +2081,7 @@ pub enum Statement { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct FunctionArgument { /// Name of the argument, if any. pub name: Option, @@ -2092,6 +2097,7 @@ pub struct FunctionArgument { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct FunctionResult { /// Type of the result. pub ty: Handle, @@ -2105,6 +2111,7 @@ pub struct FunctionResult { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct Function { /// Name of the function, if any. pub name: Option, @@ -2187,6 +2194,7 @@ pub struct Function { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct EntryPoint { /// Name of this entry point, visible externally. /// @@ -2230,6 +2238,7 @@ pub enum PredeclaredType { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct SpecialTypes { /// Type for `RayDesc`. /// @@ -2321,6 +2330,7 @@ pub enum RayQueryIntersection { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] +#[cfg_attr(test, derive(PartialEq))] pub struct Module { /// Arena for the types defined in this module. pub types: UniqueArena, diff --git a/naga/src/span.rs b/naga/src/span.rs index 7c1ce17dca..7f3a461db3 100644 --- a/naga/src/span.rs +++ b/naga/src/span.rs @@ -345,7 +345,7 @@ impl SpanProvider for Arena { } } -impl SpanProvider for UniqueArena { +impl SpanProvider for UniqueArena { fn get_span(&self, handle: Handle) -> Span { self.get_span(handle) } From fdc2cfc66f16e473d3411cf4557a3724eeae25ed Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Sun, 5 Jan 2025 21:42:27 -0800 Subject: [PATCH 20/37] better variable names --- naga/src/compact/mod.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index a4b1a3149b..0348df35a4 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -461,15 +461,15 @@ fn type_expression_interdependence() { ) }; // borrow checker breaks without the tmp variables as of Rust 1.83.0 - let tmp0 = type_needs_expression(&mut module, expr); - let tmp1 = type_needs_type(&mut module, tmp0); - let tmp2 = expression_needs_type(&mut module, tmp1); - expression_needed(&mut module, tmp2); - let tmp3 = expression_needs_type(&mut module, u32); - let tmp4 = expression_needs_expression(&mut module, tmp3); - let tmp5 = type_needs_expression(&mut module, tmp4); - type_needed(&mut module, tmp5); - let b4 = module.clone(); + let expr_end = type_needs_expression(&mut module, expr); + let ty_trace = type_needs_type(&mut module, expr_end); + let expr_init = expression_needs_type(&mut module, ty_trace); + expression_needed(&mut module, expr_init); + let ty_end = expression_needs_type(&mut module, u32); + let expr_trace = expression_needs_expression(&mut module, ty_end); + let ty_init = type_needs_expression(&mut module, expr_trace); + type_needed(&mut module, ty_init); + let untouched = module.clone(); compact(&mut module); - assert!(b4 == module); + assert!(module == untouched); } From c081c9907109c1db719dae0ce5441b6338835af6 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Sun, 5 Jan 2025 21:45:16 -0800 Subject: [PATCH 21/37] remove git remnants from changelog --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ace4c51eb1..d7f9afc906 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -205,7 +205,6 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] #### D3D12 - Fix no longer showing software rasterizer adapters. By @wumpf in [#6843](https://github.com/gfx-rs/wgpu/pull/6843). ->>>>>>> upstream/trunk ### Examples From 3e328582ebf48502d29bd30ae71c3e3f95fc0753 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 13:12:29 -0800 Subject: [PATCH 22/37] avoid deriving PartialEq for module --- naga/src/arena/unique_arena.rs | 9 ++++----- naga/src/block.rs | 1 - naga/src/compact/mod.rs | 6 +++++- naga/src/diagnostic_filter.rs | 2 -- naga/src/lib.rs | 10 ---------- naga/src/span.rs | 2 +- 6 files changed, 10 insertions(+), 20 deletions(-) diff --git a/naga/src/arena/unique_arena.rs b/naga/src/arena/unique_arena.rs index db4348aa5a..c64bb302eb 100644 --- a/naga/src/arena/unique_arena.rs +++ b/naga/src/arena/unique_arena.rs @@ -23,8 +23,7 @@ use std::{fmt, hash, ops}; /// /// [`Arena`]: super::Arena #[derive(Clone)] -#[cfg_attr(test, derive(PartialEq))] -pub struct UniqueArena { +pub struct UniqueArena { set: FastIndexSet, /// Spans for the elements, indexed by handle. @@ -36,7 +35,7 @@ pub struct UniqueArena { span_info: Vec, } -impl UniqueArena { +impl UniqueArena { /// Create a new arena with no initial capacity allocated. pub fn new() -> Self { UniqueArena { @@ -183,7 +182,7 @@ impl UniqueArena { } } -impl Default for UniqueArena { +impl Default for UniqueArena { fn default() -> Self { Self::new() } @@ -195,7 +194,7 @@ impl fmt::Debug for UniqueArena { } } -impl ops::Index> for UniqueArena { +impl ops::Index> for UniqueArena { type Output = T; fn index(&self, handle: Handle) -> &T { &self.set[handle.index()] diff --git a/naga/src/block.rs b/naga/src/block.rs index f2d2d027b3..2e86a928f1 100644 --- a/naga/src/block.rs +++ b/naga/src/block.rs @@ -6,7 +6,6 @@ use std::ops::{Deref, DerefMut, RangeBounds}; #[cfg_attr(feature = "serialize", derive(serde::Serialize))] #[cfg_attr(feature = "serialize", serde(transparent))] #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct Block { body: Vec, #[cfg_attr(feature = "serialize", serde(skip))] diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 0348df35a4..ef2b705e0c 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -471,5 +471,9 @@ fn type_expression_interdependence() { type_needed(&mut module, ty_init); let untouched = module.clone(); compact(&mut module); - assert!(module == untouched); + assert!(module.types.iter().collect::>() == untouched.types.iter().collect::>()); + assert!( + module.global_expressions.iter().collect::>() + == untouched.global_expressions.iter().collect::>() + ); } diff --git a/naga/src/diagnostic_filter.rs b/naga/src/diagnostic_filter.rs index e3062bfa52..2fa5464cdf 100644 --- a/naga/src/diagnostic_filter.rs +++ b/naga/src/diagnostic_filter.rs @@ -94,7 +94,6 @@ impl StandardFilterableTriggeringRule { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct DiagnosticFilter { pub new_severity: Severity, pub triggering_rule: FilterableTriggeringRule, @@ -236,7 +235,6 @@ pub(crate) struct ConflictingDiagnosticRuleError { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct DiagnosticFilterNode { pub inner: DiagnosticFilter, pub parent: Option>, diff --git a/naga/src/lib.rs b/naga/src/lib.rs index 0f34655d1f..8db5b676d6 100644 --- a/naga/src/lib.rs +++ b/naga/src/lib.rs @@ -999,7 +999,6 @@ pub struct GlobalVariable { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct LocalVariable { /// Name of the variable, if any. pub name: Option, @@ -1721,7 +1720,6 @@ pub enum SwitchValue { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct SwitchCase { /// Value, upon which the case is considered true. pub value: SwitchValue, @@ -1740,7 +1738,6 @@ pub struct SwitchCase { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub enum RayQueryFunction { /// Initialize the `RayQuery` object. Initialize { @@ -1785,7 +1782,6 @@ pub enum RayQueryFunction { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub enum Statement { /// Emit a range of expressions, visible to all statements that follow in this block. /// @@ -2081,7 +2077,6 @@ pub enum Statement { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct FunctionArgument { /// Name of the argument, if any. pub name: Option, @@ -2097,7 +2092,6 @@ pub struct FunctionArgument { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct FunctionResult { /// Type of the result. pub ty: Handle, @@ -2111,7 +2105,6 @@ pub struct FunctionResult { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct Function { /// Name of the function, if any. pub name: Option, @@ -2194,7 +2187,6 @@ pub struct Function { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct EntryPoint { /// Name of this entry point, visible externally. /// @@ -2238,7 +2230,6 @@ pub enum PredeclaredType { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct SpecialTypes { /// Type for `RayDesc`. /// @@ -2330,7 +2321,6 @@ pub enum RayQueryIntersection { #[cfg_attr(feature = "serialize", derive(Serialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] -#[cfg_attr(test, derive(PartialEq))] pub struct Module { /// Arena for the types defined in this module. pub types: UniqueArena, diff --git a/naga/src/span.rs b/naga/src/span.rs index 7f3a461db3..7c1ce17dca 100644 --- a/naga/src/span.rs +++ b/naga/src/span.rs @@ -345,7 +345,7 @@ impl SpanProvider for Arena { } } -impl SpanProvider for UniqueArena { +impl SpanProvider for UniqueArena { fn get_span(&self, handle: Handle) -> Span { self.get_span(handle) } From aa670cb5a042c9d0d5df7e4e7d2c910ee0903e4a Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 13:49:41 -0800 Subject: [PATCH 23/37] remove already-guaranteed check but not test --- naga/src/valid/handles.rs | 39 +++------------------------------------ 1 file changed, 3 insertions(+), 36 deletions(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 2e75485234..92bc5ecaff 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -734,40 +734,6 @@ impl super::Validator { | crate::Statement::Barrier(_) => Ok(()), }) } - - #[cfg(feature = "compact")] - pub fn well_ordered_deps( - (handle, expression): (Handle, &crate::Expression), - constants: &Arena, - global_expressions: &Arena, - types: &UniqueArena, - ) -> Result<(), InvalidHandleError> { - let mut exprs = HandleSet::for_arena(global_expressions); - ExpressionTracer { - types: Some(types), - expressions: global_expressions, - constants, - types_used: &mut HandleSet::for_arena(types), - constants_used: &mut HandleSet::for_arena(constants), - expressions_used: &mut exprs, - global_expressions_used: None, - } - .trace_expression(expression); - if let Err(error) = handle.check_dep_iter(exprs.iter()) { - return Err(InvalidHandleError::ForwardDependency(error)); - } - Ok(()) - } - - #[cfg(not(feature = "compact"))] - pub const fn well_ordered_deps( - (_handle, _expression): (Handle, &crate::Expression), - _constants: &Arena, - _global_expressions: &Arena, - _types: &UniqueArena, - ) -> Result<(), InvalidHandleError> { - Ok(()) - } } impl From for ValidationError { @@ -1004,8 +970,9 @@ fn well_ordered_expressions() { // Everything should be okay now. assert!(Validator::validate_module_handles(&m).is_ok()); - // Mutate `ex_zero`'s type to `ty_arr`, introducing an out of order dependency. - // Validation should catch the problem. + // Mutate `ex_zero`'s type to `ty_arr`, introducing an out of order dependency. This doesn't + // introduce a cycle but is caught as an error by the same code because of the way cycles are + // detected. m.global_expressions[ex_zero] = Expression::ZeroValue(ty_arr); assert!(Validator::validate_module_handles(&m).is_err()); } From c76784070a4ba483d3d2441f9f5c02de40fa6c1d Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 13:51:36 -0800 Subject: [PATCH 24/37] remove unused dependencies --- naga/src/valid/handles.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 92bc5ecaff..4666b6a466 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -9,9 +9,6 @@ use crate::{ use crate::non_max_u32::NonMaxU32; use crate::{Arena, UniqueArena}; -#[cfg(feature = "compact")] -use crate::{arena::HandleSet, compact::expressions::ExpressionTracer}; - use super::ValidationError; use std::{convert::TryInto, hash::Hash}; From b53d6caccbe6ecd29bfc35bceebb0088ced5df1c Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 14:06:51 -0800 Subject: [PATCH 25/37] remove call to well_ordered_deps --- naga/src/valid/handles.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index 4666b6a466..333caa23ad 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -127,7 +127,6 @@ impl super::Validator { // Validate the remaining expressions normally. for handle_and_expr in global_exprs_iter { Self::validate_const_expression_handles(handle_and_expr, constants, overrides)?; - Self::well_ordered_deps(handle_and_expr, constants, global_expressions, types)?; } let validate_type = |handle| Self::validate_type_handle(handle, types); From c20f54596792ded494ce743d59571a4bccdb5215 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 18:59:24 -0800 Subject: [PATCH 26/37] correct the block comment explaining type_expression_tandem --- naga/src/compact/mod.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index ef2b705e0c..ff91b7895a 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -256,14 +256,13 @@ impl<'module> ModuleTracer<'module> { } fn type_expression_tandem(&mut self) { - // assume there are no cycles in the type/expression graph (guaranteed by validator) - // assume that the expressions are well ordered since they're not merged like types are + // assumes there are no cycles in the type/expression graph + // assumes all types' and expressions' dependencies go "high references low index" + // assumes that the expressions are well ordered even through a type dependency // ie. expression A referring to a type referring to expression B has A > B. - // (also guaranteed by validator) - // 1. iterate over types, skipping unused ones - // a. if the type references an expression, mark it used - // b. repeat `a` while walking referenced types, marking them as used + // 1. iterate backwards over types, skipping unused ones + // a. if the type references a type or expression, mark it used // 2. iterate backwards over expressions, skipping unused ones // a. if the expression references a type // i. walk the type's dependency tree, marking the types and their referenced @@ -272,11 +271,11 @@ impl<'module> ModuleTracer<'module> { // ┌───────────┐ ┌───────────┐ // │Expressions│ │ Types │ - // │ ╵ ╵ │ + // │ │ │ │ + // │ ◄────────────┐ │ // │ covered by │ │ So that back/forths starting with a type now start with an // │ step 1 │ │ expression instead. - // │ ◄────────────┘ │ - // │ │ │ │ + // │ ╷ ╷ │ // │ │ │ │ // │ ◄────────────┐ │ This arrow is only as needed. // │ │ │ │ │ From 01e8e1c77baadd28c7f3327f25e171784191424a Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 19:18:12 -0800 Subject: [PATCH 27/37] better variable names --- naga/src/compact/expressions.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index b04476838f..83f94c2875 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -30,24 +30,25 @@ pub struct ExpressionTracer<'tracer> { } impl ExpressionTracer<'_> { - fn types_used_insert(&mut self, x: Handle) { + fn types_used_insert(&mut self, used: Handle) { if self.types.is_some() { - self.trace_type(x); + self.trace_type(used); } - self.types_used.insert(x); + self.types_used.insert(used); } - fn trace_type(&mut self, x: Handle) { - fn handle2type(x: &mut TypeTracer, y: Handle) { - x.types_used.insert(y); - x.trace_type(&x.types[y], handle2type); + fn trace_type(&mut self, tracing: Handle) { + fn type_used(caller: &mut TypeTracer, used: Handle) { + caller.types_used.insert(used); + caller.trace_type(&caller.types[used], type_used); } + TypeTracer { types: self.types.unwrap(), types_used: self.types_used, expressions_used: self.expressions_used, } - .trace_type(&self.types.unwrap()[x], handle2type) + .trace_type(&self.types.unwrap()[tracing], type_used) } /// Propagate usage through `self.expressions`, starting with `self.expressions_used`. From a527f57bc290154ff60889ec4909f5cfdd866291 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 19:23:41 -0800 Subject: [PATCH 28/37] stop if type is already marked used --- naga/src/compact/expressions.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index 83f94c2875..52c8fb13d8 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -39,8 +39,9 @@ impl ExpressionTracer<'_> { fn trace_type(&mut self, tracing: Handle) { fn type_used(caller: &mut TypeTracer, used: Handle) { - caller.types_used.insert(used); - caller.trace_type(&caller.types[used], type_used); + if caller.types_used.insert(used) { + caller.trace_type(&caller.types[used], type_used); + } } TypeTracer { From f48fb3da0041e41c91526053f966c8830c945035 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 19:35:34 -0800 Subject: [PATCH 29/37] explanation for the optional types --- naga/src/compact/expressions.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index 52c8fb13d8..6fab6c6a7a 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -3,6 +3,10 @@ use crate::arena::{Arena, Handle, UniqueArena}; use crate::compact::types::TypeTracer; pub struct ExpressionTracer<'tracer> { + /// The type arena used by `expressions`. + /// + /// Having this set as None will result in a shallow types_used set. + /// `Some` is used to ensure propagation through interleaved type and expression dependencies. pub types: Option<&'tracer UniqueArena>, pub constants: &'tracer Arena, From 3fcdae7d0269140549345b9d4f967b6c130dce66 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 19:38:40 -0800 Subject: [PATCH 30/37] 80 character limit for comments (?) --- naga/src/compact/expressions.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index 6fab6c6a7a..afb1366b9c 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -6,7 +6,8 @@ pub struct ExpressionTracer<'tracer> { /// The type arena used by `expressions`. /// /// Having this set as None will result in a shallow types_used set. - /// `Some` is used to ensure propagation through interleaved type and expression dependencies. + /// `Some` is used to ensure propagation through interleaved type + /// and expression dependencies. pub types: Option<&'tracer UniqueArena>, pub constants: &'tracer Arena, From 88ed425da2c4526a1163c772c1fb8bd5944e8d9a Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Tue, 14 Jan 2025 19:59:06 -0800 Subject: [PATCH 31/37] github's monospace breaks box-drawing characters --- naga/src/compact/mod.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index ff91b7895a..12150f16a5 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -269,21 +269,21 @@ impl<'module> ModuleTracer<'module> { // expressions as used (types_used_insert instead of types_used.insert) // b. if the expression references another expression, mark the latter as used - // ┌───────────┐ ┌───────────┐ - // │Expressions│ │ Types │ - // │ │ │ │ - // │ ◄────────────┐ │ - // │ covered by │ │ So that back/forths starting with a type now start with an - // │ step 1 │ │ expression instead. - // │ ╷ ╷ │ - // │ │ │ │ - // │ ◄────────────┐ │ This arrow is only as needed. - // │ │ │ │ │ - // │ ┌────────────►│ │ - // │ │ covered by │ This covers back/forths starting with an expression. - // │ │ step 2 │ - // │ ╷ ╷ │ - // └───────────┘ └───────────┘ + // +-----------+ +-----------+ + // |Expressions| | Types | + // | | | | + // | ◄------------+ | + // | covered by | | So that back/forths starting with a type now start with an + // | step 1 | | expression instead. + // | | | | + // | | | | + // | ◄------------+ | This arrow is only as needed. + // | | | | | + // | +------------►| | + // | | covered by | This covers back/forths starting with an expression. + // | | step 2 | + // | | | | + // +-----------+ +-----------+ // 1 self.as_type().trace_types(); From ba5335adcda882a7179c96683a699913e2d3ec67 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Wed, 15 Jan 2025 16:03:43 -0800 Subject: [PATCH 32/37] clippy --- naga/src/valid/handles.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/naga/src/valid/handles.rs b/naga/src/valid/handles.rs index ae4cb02421..7502d8f493 100644 --- a/naga/src/valid/handles.rs +++ b/naga/src/valid/handles.rs @@ -1032,6 +1032,7 @@ fn override_init_deps() { assert!(Validator::validate_module_handles(&m).is_err()); } +#[test] fn well_ordered_expressions() { use super::Validator; use crate::{ArraySize, Expression, Literal, PendingArraySize, Scalar, Span, Type, TypeInner}; From 9d3f40d4613845cdc7077ac698897d6e3a6a0e5d Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Thu, 16 Jan 2025 10:03:17 -0800 Subject: [PATCH 33/37] modify test to fail on trunk --- naga/src/compact/mod.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 21b6f1e9e5..2f671102fd 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -458,6 +458,12 @@ fn type_expression_interdependence() { crate::Span::default(), ) }; + let cmp_modules = |mod0: &crate::Module, mod1: &crate::Module| { + return (mod0.types.iter().collect::>() + == mod1.types.iter().collect::>()) + && (mod0.global_expressions.iter().collect::>() + == mod1.global_expressions.iter().collect::>()); + }; // borrow checker breaks without the tmp variables as of Rust 1.83.0 let expr_end = type_needs_expression(&mut module, expr); let ty_trace = type_needs_type(&mut module, expr_end); @@ -469,9 +475,8 @@ fn type_expression_interdependence() { type_needed(&mut module, ty_init); let untouched = module.clone(); compact(&mut module); - assert!(module.types.iter().collect::>() == untouched.types.iter().collect::>()); - assert!( - module.global_expressions.iter().collect::>() - == untouched.global_expressions.iter().collect::>() - ); + assert!(cmp_modules(&module, &untouched)); + expression_needs_type(&mut module, ty_trace); + compact(&mut module); + assert!(!cmp_modules(&module, &untouched)); } From 80ad878233283315f1c6ea1bcc5d25e457b6d9e8 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Thu, 16 Jan 2025 10:43:24 -0800 Subject: [PATCH 34/37] `fmt` --- naga/src/compact/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 2f671102fd..cc84273f87 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -459,8 +459,7 @@ fn type_expression_interdependence() { ) }; let cmp_modules = |mod0: &crate::Module, mod1: &crate::Module| { - return (mod0.types.iter().collect::>() - == mod1.types.iter().collect::>()) + return (mod0.types.iter().collect::>() == mod1.types.iter().collect::>()) && (mod0.global_expressions.iter().collect::>() == mod1.global_expressions.iter().collect::>()); }; From b8d5d0ab8a14aa174337f49fb854fdff9909a956 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Thu, 16 Jan 2025 10:54:27 -0800 Subject: [PATCH 35/37] `clippy` --- naga/src/compact/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index cc84273f87..9b3622f8c2 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -459,9 +459,9 @@ fn type_expression_interdependence() { ) }; let cmp_modules = |mod0: &crate::Module, mod1: &crate::Module| { - return (mod0.types.iter().collect::>() == mod1.types.iter().collect::>()) + (mod0.types.iter().collect::>() == mod1.types.iter().collect::>()) && (mod0.global_expressions.iter().collect::>() - == mod1.global_expressions.iter().collect::>()); + == mod1.global_expressions.iter().collect::>()) }; // borrow checker breaks without the tmp variables as of Rust 1.83.0 let expr_end = type_needs_expression(&mut module, expr); From 2a2c42b3e8844ae042fe460c4f4cd6a17a362ad6 Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Thu, 16 Jan 2025 12:29:49 -0800 Subject: [PATCH 36/37] valid test that fails on trunk --- naga/src/compact/mod.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 9b3622f8c2..a8970c3522 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -475,7 +475,12 @@ fn type_expression_interdependence() { let untouched = module.clone(); compact(&mut module); assert!(cmp_modules(&module, &untouched)); - expression_needs_type(&mut module, ty_trace); - compact(&mut module); + let unused_expr = module.global_expressions.append( + crate::Expression::Literal(crate::Literal::U32(1)), + crate::Span::default(), + ); + type_needs_expression(&mut module, unused_expr); assert!(!cmp_modules(&module, &untouched)); + compact(&mut module); + assert!(cmp_modules(&module, &untouched)); } From 553d39b7543456e4feffc53da62a179700ff0afd Mon Sep 17 00:00:00 2001 From: Kent Slaney Date: Thu, 16 Jan 2025 13:08:26 -0800 Subject: [PATCH 37/37] better variable names --- naga/src/compact/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/naga/src/compact/types.rs b/naga/src/compact/types.rs index e664ff7d63..05e46325b9 100644 --- a/naga/src/compact/types.rs +++ b/naga/src/compact/types.rs @@ -25,8 +25,8 @@ impl TypeTracer<'_> { continue; } - self.trace_type(ty, |x, y| { - x.types_used.insert(y); + self.trace_type(ty, |caller, used| { + caller.types_used.insert(used); }); } }