diff --git a/CHANGELOG.md b/CHANGELOG.md index a6281f8de0..6cd534f327 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -364,6 +364,7 @@ By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148] - In validation, forbid cycles between global expressions and types. By @jimblandy in [#6800](https://github.com/gfx-rs/wgpu-pull/6800) - Allow abstract scalars in modf and frexp results. By @jimblandy in [#6821](https://github.com/gfx-rs/wgpu-pull/6821) - In the WGSL front end, apply automatic conversions to values being assigned. By @jimblandy in [#6822](https://github.com/gfx-rs/wgpu-pull/6822) +- Fix a leak by ensuring that types that depend on expressions are correctly compacted. By @KentSlaney in [#6934](https://github.com/gfx-rs/wgpu/pull/6934). #### Vulkan diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index 7e611c35fe..78c3f0de7e 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -62,168 +62,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::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::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); } - 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); + Sl::Gradient { x, y } => self.expressions_used.insert_iter([x, y]), } - 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); + 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 9414a9804c..ef41a611cb 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); - } - } - // We assume that all functions are used. // // Observe which types, constant expressions, constants, and @@ -111,12 +101,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. @@ -134,8 +118,7 @@ pub fn compact(module: &mut crate::Module) { } } - // Propagate usage through types. - module_tracer.as_type().trace_types(); + module_tracer.type_expression_tandem(); // Now that we know what is used and what is never touched, // produce maps from the `Handle`s that appear in `module` now to @@ -271,10 +254,77 @@ impl<'module> ModuleTracer<'module> { } } + /// Traverse types and global expressions in tandem to determine which are used. + /// + /// Assuming that all types and global expressions used by other parts of + /// the module have been added to [`types_used`] and + /// [`global_expressions_used`], expand those sets to include all types and + /// global expressions reachable from those. + /// + /// [`types_used`]: ModuleTracer::types_used + /// [`global_expressions_used`]: ModuleTracer::global_expressions_used + fn type_expression_tandem(&mut self) { + // For each type T, compute the latest global expression E that T and + // its predecessors refer to. Given the ordering rules on types and + // global expressions in valid modules, we can do this with a single + // forward scan of the type arena. The rules further imply that T can + // only be referred to by expressions after E. + let mut max_dep = Vec::with_capacity(self.module.types.len()); + let mut previous = None; + for (_handle, ty) in self.module.types.iter() { + previous = std::cmp::max( + previous, + match ty.inner { + crate::TypeInner::Array { + base: _, + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(expr)), + stride: _, + } + | crate::TypeInner::BindingArray { + base: _, + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(expr)), + } => Some(expr), + _ => None, + }, + ); + max_dep.push(previous); + } + + // Visit types and global expressions from youngest to oldest. + // + // The outer loop visits types. Before visiting each type, the inner + // loop ensures that all global expressions that could possibly refer to + // it have been visited. And since the inner loop stop at the latest + // expression that the type could possibly refer to, we know that we + // have previously visited any types that might refer to each expression + // we visit. + // + // This lets us assume that any type or expression that is *not* marked + // as used by the time we visit it is genuinely unused, and can be + // ignored. + let mut exprs = self.module.global_expressions.iter().rev().peekable(); + for ((ty_handle, ty), dep) in self.module.types.iter().rev().zip(max_dep.iter().rev()) { + while let Some((expr_handle, expr)) = exprs.next_if(|&(h, _)| Some(h) > *dep) { + if self.global_expressions_used.contains(expr_handle) { + self.as_const_expression().trace_expression(expr); + } + } + if self.types_used.contains(ty_handle) { + self.as_type().trace_type(ty); + } + } + // Visit any remaining expressions. + for (expr_handle, expr) in exprs { + if self.global_expressions_used.contains(expr_handle) { + self.as_const_expression().trace_expression(expr); + } + } + } + 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, } } @@ -352,3 +402,114 @@ 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 cmp_modules = |mod0: &crate::Module, mod1: &crate::Module| { + (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); + 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!(cmp_modules(&module, &untouched)); + 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)); +} diff --git a/naga/src/compact/types.rs b/naga/src/compact/types.rs index ae4ae35580..068d251b87 100644 --- a/naga/src/compact/types.rs +++ b/naga/src/compact/types.rs @@ -1,58 +1,53 @@ use super::{HandleSet, ModuleMap}; -use crate::{Handle, UniqueArena}; +use crate::Handle; pub struct TypeTracer<'a> { - pub types: &'a UniqueArena, pub types_used: &'a mut HandleSet, + pub expressions_used: &'a mut HandleSet, } impl TypeTracer<'_> { - /// Propagate usage through `self.types`, starting with `self.types_used`. - /// - /// Treat `self.types_used` as the initial set of "known - /// live" types, and follow through to identify all - /// transitively used types. - pub fn trace_types(&mut self) { - // We don't need recursion or a work list. Because an - // expression may only refer to other expressions that precede - // it in the arena, it suffices to make a single pass over the - // arena from back to front, marking the referents of used - // expressions as used themselves. - for (handle, ty) in self.types.iter().rev() { - // If this type isn't used, it doesn't matter what it uses. - if !self.types_used.contains(handle) { - continue; + pub fn trace_type(&mut self, ty: &crate::Type) { + 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: _, } - - 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::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)); - } + | Ti::BindingArray { + base, + size: crate::ArraySize::Pending(crate::PendingArraySize::Expression(expr)), + } => { + self.expressions_used.insert(expr); + self.types_used.insert(base); + } + 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)); } } }