diff --git a/CHANGELOG.md b/CHANGELOG.md index d69de7f0c9..34307eba67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,7 +54,11 @@ For naga changelogs at or before v0.14.0. See [naga's changelog](naga/CHANGELOG. - Create a hidden window per `wgpu::Instance` instead of sharing a global one. -#### Naga MSL-OUT +#### Naga + +- Improve algorithm used by module compaction. By @jimblandy in [#4662](https://github.com/gfx-rs/wgpu/pull/4662). + +##### MSL-OUT - Fix issue where local variables were sometimes using variable names from previous functions. diff --git a/naga/src/compact/expressions.rs b/naga/src/compact/expressions.rs index c1326e92be..301bbe3240 100644 --- a/naga/src/compact/expressions.rs +++ b/naga/src/compact/expressions.rs @@ -1,8 +1,7 @@ use super::{HandleMap, HandleSet, ModuleMap}; -use crate::arena::{Arena, Handle, UniqueArena}; +use crate::arena::{Arena, Handle}; pub struct ExpressionTracer<'tracer> { - pub types: &'tracer UniqueArena, pub constants: &'tracer Arena, /// The arena in which we are currently tracing expressions. @@ -21,34 +20,51 @@ pub struct ExpressionTracer<'tracer> { /// the module's constant expression arena. pub expressions_used: &'tracer mut HandleSet, - /// The constant expression arena and its used map, if we haven't - /// switched to tracing constant expressions yet. - pub const_expressions: Option<( - &'tracer Arena, - &'tracer mut HandleSet, - )>, + /// The used set for the module's `const_expressions` arena. + /// + /// If `None`, we are already tracing the constant expressions, + /// and `expressions_used` already refers to their handle set. + pub const_expressions_used: Option<&'tracer mut HandleSet>, } impl<'tracer> ExpressionTracer<'tracer> { - pub fn trace_expression(&mut self, expr: Handle) { + /// Propagate usage through `self.expressions`, starting with `self.expressions_used`. + /// + /// Treat `self.expressions_used` as the initial set of "known + /// live" expressions, and follow through to identify all + /// transitively used expressions. + /// + /// Mark types, constants, and constant expressions used directly + /// by `self.expressions` as used. Items used indirectly are not + /// marked. + /// + /// [fe]: crate::Function::expressions + /// [ce]: crate::Module::const_expressions + pub fn trace_expressions(&mut self) { log::trace!( "entering trace_expression of {}", - if self.const_expressions.is_some() { + if self.const_expressions_used.is_some() { "function expressions" } else { "const expressions" } ); - let mut work_list = vec![expr]; - while let Some(expr) = work_list.pop() { - // If we've already seen this expression, no need to trace further. - if !self.expressions_used.insert(expr) { + + // 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, expr) in self.expressions.iter().rev() { + // If this expression isn't used, it doesn't matter what it uses. + if !self.expressions_used.contains(handle) { continue; } + log::trace!("tracing new expression {:?}", expr); use crate::Expression as Ex; - match self.expressions[expr] { + match *expr { // Expressions that do not contain handles that need to be traced. Ex::Literal(_) | Ex::FunctionArgument(_) @@ -59,24 +75,34 @@ impl<'tracer> ExpressionTracer<'tracer> { Ex::Constant(handle) => { self.constants_used.insert(handle); - let constant = &self.constants[handle]; - self.trace_type(constant.ty); - self.trace_const_expression(constant.init); + // 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.const_expressions_used { + Some(ref mut used) => used.insert(init), + None => self.expressions_used.insert(init), + } } - Ex::ZeroValue(ty) => self.trace_type(ty), + Ex::ZeroValue(ty) => self.types_used.insert(ty), Ex::Compose { ty, ref components } => { - self.trace_type(ty); - work_list.extend(components); + self.types_used.insert(ty); + self.expressions_used + .insert_iter(components.iter().cloned()); } - Ex::Access { base, index } => work_list.extend([base, index]), - Ex::AccessIndex { base, index: _ } => work_list.push(base), - Ex::Splat { size: _, value } => work_list.push(value), + 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: _, - } => work_list.push(vector), - Ex::Load { pointer } => work_list.push(pointer), + } => self.expressions_used.insert(vector), + Ex::Load { pointer } => self.expressions_used.insert(pointer), Ex::ImageSample { image, sampler, @@ -87,20 +113,20 @@ impl<'tracer> ExpressionTracer<'tracer> { ref level, depth_ref, } => { - work_list.push(image); - work_list.push(sampler); - work_list.push(coordinate); - work_list.extend(array_index); - if let Some(offset) = offset { - self.trace_const_expression(offset); + self.expressions_used + .insert_iter([image, sampler, coordinate]); + self.expressions_used.insert_iter(array_index); + match self.const_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) => work_list.push(expr), - Sl::Gradient { x, y } => work_list.extend([x, y]), + Sl::Exact(expr) | Sl::Bias(expr) => self.expressions_used.insert(expr), + Sl::Gradient { x, y } => self.expressions_used.insert_iter([x, y]), } - work_list.extend(depth_ref); + self.expressions_used.insert_iter(depth_ref); } Ex::ImageLoad { image, @@ -109,33 +135,37 @@ impl<'tracer> ExpressionTracer<'tracer> { sample, level, } => { - work_list.push(image); - work_list.push(coordinate); - work_list.extend(array_index); - work_list.extend(sample); - work_list.extend(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 } => { - work_list.push(image); + self.expressions_used.insert(image); use crate::ImageQuery as Iq; match *query { - Iq::Size { level } => work_list.extend(level), + Iq::Size { level } => self.expressions_used.insert_iter(level), Iq::NumLevels | Iq::NumLayers | Iq::NumSamples => {} } } - Ex::Unary { op: _, expr } => work_list.push(expr), - Ex::Binary { op: _, left, right } => work_list.extend([left, right]), + 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, - } => work_list.extend([condition, accept, reject]), + } => self + .expressions_used + .insert_iter([condition, accept, reject]), Ex::Derivative { axis: _, ctrl: _, expr, - } => work_list.push(expr), - Ex::Relational { fun: _, argument } => work_list.push(argument), + } => self.expressions_used.insert(expr), + Ex::Relational { fun: _, argument } => self.expressions_used.insert(argument), Ex::Math { fun: _, arg, @@ -143,61 +173,26 @@ impl<'tracer> ExpressionTracer<'tracer> { arg2, arg3, } => { - work_list.push(arg); - work_list.extend(arg1); - work_list.extend(arg2); - work_list.extend(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: _, - } => work_list.push(expr), - Ex::AtomicResult { ty, comparison: _ } => self.trace_type(ty), - Ex::WorkGroupUniformLoadResult { ty } => self.trace_type(ty), - Ex::ArrayLength(expr) => work_list.push(expr), + } => self.expressions_used.insert(expr), + Ex::AtomicResult { ty, comparison: _ } => self.types_used.insert(ty), + Ex::WorkGroupUniformLoadResult { ty } => self.types_used.insert(ty), + Ex::ArrayLength(expr) => self.expressions_used.insert(expr), Ex::RayQueryGetIntersection { query, committed: _, - } => work_list.push(query), + } => self.expressions_used.insert(query), } } } - - fn trace_type(&mut self, ty: Handle) { - let mut types_used = super::types::TypeTracer { - types: self.types, - types_used: self.types_used, - }; - types_used.trace_type(ty); - } - - pub fn as_const_expression(&mut self) -> ExpressionTracer { - match self.const_expressions { - Some((ref mut exprs, ref mut exprs_used)) => ExpressionTracer { - expressions: exprs, - expressions_used: exprs_used, - types: self.types, - constants: self.constants, - types_used: self.types_used, - constants_used: self.constants_used, - const_expressions: None, - }, - None => ExpressionTracer { - types: self.types, - constants: self.constants, - expressions: self.expressions, - types_used: self.types_used, - constants_used: self.constants_used, - expressions_used: self.expressions_used, - const_expressions: None, - }, - } - } - - fn trace_const_expression(&mut self, const_expr: Handle) { - self.as_const_expression().trace_expression(const_expr); - } } impl ModuleMap { diff --git a/naga/src/compact/functions.rs b/naga/src/compact/functions.rs index 752c3eb7f1..b0d08c7e96 100644 --- a/naga/src/compact/functions.rs +++ b/naga/src/compact/functions.rs @@ -1,10 +1,9 @@ use super::handle_set_map::HandleSet; use super::{FunctionMap, ModuleMap}; -use crate::arena::Handle; pub struct FunctionTracer<'a> { - pub module: &'a crate::Module, pub function: &'a crate::Function, + pub constants: &'a crate::Arena, pub types_used: &'a mut HandleSet, pub constants_used: &'a mut HandleSet, @@ -17,57 +16,43 @@ pub struct FunctionTracer<'a> { impl<'a> FunctionTracer<'a> { pub fn trace(&mut self) { for argument in self.function.arguments.iter() { - self.trace_type(argument.ty); + self.types_used.insert(argument.ty); } if let Some(ref result) = self.function.result { - self.trace_type(result.ty); + self.types_used.insert(result.ty); } for (_, local) in self.function.local_variables.iter() { - self.trace_type(local.ty); + self.types_used.insert(local.ty); if let Some(init) = local.init { - self.trace_expression(init); + self.expressions_used.insert(init); } } // Treat named expressions as alive, for the sake of our test suite, // which uses `let blah = expr;` to exercise lots of things. - for (value, _name) in &self.function.named_expressions { - self.trace_expression(*value); + for (&value, _name) in &self.function.named_expressions { + self.expressions_used.insert(value); } self.trace_block(&self.function.body); - } - - pub fn trace_type(&mut self, ty: Handle) { - self.as_type().trace_type(ty) - } - pub fn trace_expression(&mut self, expr: Handle) { - self.as_expression().trace_expression(expr); - } - - fn as_type(&mut self) -> super::types::TypeTracer { - super::types::TypeTracer { - types: &self.module.types, - types_used: self.types_used, - } + // Given that `trace_block` has marked the expressions used + // directly by statements, walk the arena to find all + // expressions used, directly or indirectly. + self.as_expression().trace_expressions(); } fn as_expression(&mut self) -> super::expressions::ExpressionTracer { super::expressions::ExpressionTracer { - types: &self.module.types, - constants: &self.module.constants, + constants: self.constants, expressions: &self.function.expressions, types_used: self.types_used, constants_used: self.constants_used, expressions_used: &mut self.expressions_used, - const_expressions: Some(( - &self.module.const_expressions, - &mut self.const_expressions_used, - )), + const_expressions_used: Some(&mut self.const_expressions_used), } } } diff --git a/naga/src/compact/handle_set_map.rs b/naga/src/compact/handle_set_map.rs index bf74d3f0b9..c716ca8294 100644 --- a/naga/src/compact/handle_set_map.rs +++ b/naga/src/compact/handle_set_map.rs @@ -26,13 +26,23 @@ impl HandleSet { } /// Add `handle` to the set. - /// - /// Return `true` if the handle was not already in the set. In - /// other words, return true if it was newly inserted. - pub fn insert(&mut self, handle: Handle) -> bool { + pub fn insert(&mut self, handle: Handle) { + // Note that, oddly, `Handle::index` does not return a 1-based + // `Index`, but rather a zero-based `usize`. + self.members.insert(handle.index()); + } + + /// Add handles from `iter` to the set. + pub fn insert_iter(&mut self, iter: impl IntoIterator>) { + for handle in iter { + self.insert(handle); + } + } + + pub fn contains(&self, handle: Handle) -> bool { // Note that, oddly, `Handle::index` does not return a 1-based // `Index`, but rather a zero-based `usize`. - self.members.insert(handle.index()) + self.members.contains(handle.index()) } } @@ -148,6 +158,8 @@ impl HandleMap { // Build a zero-based end-exclusive range, given one-based handle indices. compacted = first1.get() - 1..last1.get(); } else { + // The range contains only a single live handle, which + // we identified with the first `find_map` call. compacted = first1.get() - 1..first1.get(); } } else { diff --git a/naga/src/compact/mod.rs b/naga/src/compact/mod.rs index 137f3bbe30..6f6fe3d9da 100644 --- a/naga/src/compact/mod.rs +++ b/naga/src/compact/mod.rs @@ -36,9 +36,9 @@ pub fn compact(module: &mut crate::Module) { { for (_, global) in module.global_variables.iter() { log::trace!("tracing global {:?}", global.name); - module_tracer.as_type().trace_type(global.ty); + module_tracer.types_used.insert(global.ty); if let Some(init) = global.init { - module_tracer.as_const_expression().trace_expression(init); + module_tracer.const_expressions_used.insert(init); } } } @@ -50,25 +50,23 @@ pub fn compact(module: &mut crate::Module) { for (handle, constant) in module.constants.iter() { if constant.name.is_some() { module_tracer.constants_used.insert(handle); - module_tracer.as_type().trace_type(constant.ty); - module_tracer - .as_const_expression() - .trace_expression(constant.init); + module_tracer.const_expressions_used.insert(constant.init); } } // We assume that all functions are used. // // Observe which types, constant expressions, constants, and - // expressions each function uses, and produce maps from - // pre-compaction to post-compaction handles. + // expressions each function uses, and produce maps for each + // function from pre-compaction to post-compaction expression + // handles. log::trace!("tracing functions"); let function_maps: Vec = module .functions .iter() .map(|(_, f)| { log::trace!("tracing function {:?}", f.name); - let mut function_tracer = module_tracer.enter_function(f); + let mut function_tracer = module_tracer.as_function(f); function_tracer.trace(); FunctionMap::from(function_tracer) }) @@ -81,12 +79,30 @@ pub fn compact(module: &mut crate::Module) { .iter() .map(|e| { log::trace!("tracing entry point {:?}", e.function.name); - let mut used = module_tracer.enter_function(&e.function); + let mut used = module_tracer.as_function(&e.function); used.trace(); FunctionMap::from(used) }) .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); + } + } + + // Propagate usage through types. + module_tracer.as_type().trace_types(); + // 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 @@ -189,15 +205,14 @@ impl<'module> ModuleTracer<'module> { ref predeclared_types, } = *special_types; - let mut type_tracer = self.as_type(); if let Some(ray_desc) = *ray_desc { - type_tracer.trace_type(ray_desc); + self.types_used.insert(ray_desc); } if let Some(ray_intersection) = *ray_intersection { - type_tracer.trace_type(ray_intersection); + self.types_used.insert(ray_intersection); } for (_, &handle) in predeclared_types { - type_tracer.trace_type(handle); + self.types_used.insert(handle); } } @@ -210,24 +225,22 @@ impl<'module> ModuleTracer<'module> { fn as_const_expression(&mut self) -> expressions::ExpressionTracer { expressions::ExpressionTracer { - types: &self.module.types, - constants: &self.module.constants, expressions: &self.module.const_expressions, + constants: &self.module.constants, types_used: &mut self.types_used, constants_used: &mut self.constants_used, expressions_used: &mut self.const_expressions_used, - const_expressions: None, + const_expressions_used: None, } } - pub fn enter_function<'tracer>( + pub fn as_function<'tracer>( &'tracer mut self, function: &'tracer crate::Function, ) -> FunctionTracer<'tracer> { FunctionTracer { - module: self.module, function, - + constants: &self.module.constants, types_used: &mut self.types_used, constants_used: &mut self.constants_used, const_expressions_used: &mut self.const_expressions_used, diff --git a/naga/src/compact/statements.rs b/naga/src/compact/statements.rs index 4c62771023..0698b57258 100644 --- a/naga/src/compact/statements.rs +++ b/naga/src/compact/statements.rs @@ -22,7 +22,7 @@ impl FunctionTracer<'_> { ref accept, ref reject, } => { - self.trace_expression(condition); + self.expressions_used.insert(condition); worklist.push(accept); worklist.push(reject); } @@ -30,7 +30,7 @@ impl FunctionTracer<'_> { selector, ref cases, } => { - self.trace_expression(selector); + self.expressions_used.insert(selector); for case in cases { worklist.push(&case.body); } @@ -41,15 +41,17 @@ impl FunctionTracer<'_> { break_if, } => { if let Some(break_if) = break_if { - self.trace_expression(break_if); + self.expressions_used.insert(break_if); } worklist.push(body); worklist.push(continuing); } - St::Return { value: Some(value) } => self.trace_expression(value), + St::Return { value: Some(value) } => { + self.expressions_used.insert(value); + } St::Store { pointer, value } => { - self.trace_expression(pointer); - self.trace_expression(value); + self.expressions_used.insert(pointer); + self.expressions_used.insert(value); } St::ImageStore { image, @@ -57,12 +59,12 @@ impl FunctionTracer<'_> { array_index, value, } => { - self.trace_expression(image); - self.trace_expression(coordinate); + self.expressions_used.insert(image); + self.expressions_used.insert(coordinate); if let Some(array_index) = array_index { - self.trace_expression(array_index); + self.expressions_used.insert(array_index); } - self.trace_expression(value); + self.expressions_used.insert(value); } St::Atomic { pointer, @@ -70,14 +72,14 @@ impl FunctionTracer<'_> { value, result, } => { - self.trace_expression(pointer); + self.expressions_used.insert(pointer); self.trace_atomic_function(fun); - self.trace_expression(value); - self.trace_expression(result); + self.expressions_used.insert(value); + self.expressions_used.insert(result); } St::WorkGroupUniformLoad { pointer, result } => { - self.trace_expression(pointer); - self.trace_expression(result); + self.expressions_used.insert(pointer); + self.expressions_used.insert(result); } St::Call { function: _, @@ -85,14 +87,14 @@ impl FunctionTracer<'_> { result, } => { for expr in arguments { - self.trace_expression(*expr); + self.expressions_used.insert(*expr); } if let Some(result) = result { - self.trace_expression(result); + self.expressions_used.insert(result); } } St::RayQuery { query, ref fun } => { - self.trace_expression(query); + self.expressions_used.insert(query); self.trace_ray_query_function(fun); } @@ -112,7 +114,9 @@ impl FunctionTracer<'_> { match *fun { Af::Exchange { compare: Some(expr), - } => self.trace_expression(expr), + } => { + self.expressions_used.insert(expr); + } Af::Exchange { compare: None } | Af::Add | Af::Subtract @@ -131,10 +135,12 @@ impl FunctionTracer<'_> { acceleration_structure, descriptor, } => { - self.trace_expression(acceleration_structure); - self.trace_expression(descriptor); + self.expressions_used.insert(acceleration_structure); + self.expressions_used.insert(descriptor); + } + Qf::Proceed { result } => { + self.expressions_used.insert(result); } - Qf::Proceed { result } => self.trace_expression(result), Qf::Terminate => {} } } diff --git a/naga/src/compact/types.rs b/naga/src/compact/types.rs index d11ab8a731..3b3d319204 100644 --- a/naga/src/compact/types.rs +++ b/naga/src/compact/types.rs @@ -7,16 +7,25 @@ pub struct TypeTracer<'a> { } impl<'a> TypeTracer<'a> { - pub fn trace_type(&mut self, ty: Handle) { - let mut work_list = vec![ty]; - while let Some(ty) = work_list.pop() { - // If we've already seen this type, no need to traverse further. - if !self.types_used.insert(ty) { + /// 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; } use crate::TypeInner as Ti; - match self.types[ty].inner { + match ty.inner { // Types that do not contain handles. Ti::Scalar { .. } | Ti::Vector { .. } @@ -29,19 +38,19 @@ impl<'a> TypeTracer<'a> { | Ti::RayQuery => {} // Types that do contain handles. - Ti::Pointer { base, space: _ } => work_list.push(base), - Ti::Array { + Ti::Pointer { base, space: _ } + | Ti::Array { base, size: _, stride: _, - } => work_list.push(base), + } + | Ti::BindingArray { base, size: _ } => self.types_used.insert(base), Ti::Struct { ref members, span: _, } => { - work_list.extend(members.iter().map(|m| m.ty)); + self.types_used.insert_iter(members.iter().map(|m| m.ty)); } - Ti::BindingArray { base, size: _ } => work_list.push(base), } } }