From 67e193df87774d3a2aaebe676efef16596a6d5c9 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 30 Aug 2024 12:11:03 +0000 Subject: [PATCH 1/5] first version of liveness analysis for constants --- .../src/brillig/brillig_gen.rs | 1 + .../src/brillig/brillig_gen/brillig_block.rs | 78 ++++++---- .../brillig_gen/brillig_block_variables.rs | 62 +++----- .../src/brillig/brillig_gen/brillig_fn.rs | 10 +- .../brillig_gen/constant_allocation.rs | 135 ++++++++++++++++++ .../brillig/brillig_gen/variable_liveness.rs | 86 +++++++---- .../noirc_evaluator/src/brillig/brillig_ir.rs | 4 + .../noirc_evaluator/src/ssa/ir/dom.rs | 2 +- 8 files changed, 274 insertions(+), 104 deletions(-) create mode 100644 noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index b256c2b85ab..628ec9657f2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -4,6 +4,7 @@ pub(crate) mod brillig_block_variables; pub(crate) mod brillig_directive; pub(crate) mod brillig_fn; pub(crate) mod brillig_slice_ops; +mod constant_allocation; mod variable_liveness; use acvm::FieldElement; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 54dbd4716a9..acda11b0e12 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -29,6 +29,7 @@ use std::rc::Rc; use super::brillig_black_box::convert_black_box_call; use super::brillig_block_variables::BlockVariables; use super::brillig_fn::FunctionContext; +use super::constant_allocation::InstructionLocation; /// Generate the compilation artifacts for compiling a function into brillig bytecode. pub(crate) struct BrilligBlock<'block> { @@ -117,6 +118,13 @@ impl<'block> BrilligBlock<'block> { terminator_instruction: &TerminatorInstruction, dfg: &DataFlowGraph, ) { + self.initialize_constants( + &self + .function_context + .constant_allocation + .allocated_at_location(self.block_id, InstructionLocation::Terminator), + dfg, + ); match terminator_instruction { TerminatorInstruction::JmpIf { condition, @@ -241,6 +249,14 @@ impl<'block> BrilligBlock<'block> { /// Converts an SSA instruction into a sequence of Brillig opcodes. fn convert_ssa_instruction(&mut self, instruction_id: InstructionId, dfg: &DataFlowGraph) { + self.initialize_constants( + &self.function_context.constant_allocation.allocated_at_location( + self.block_id, + InstructionLocation::Instruction(instruction_id), + ), + dfg, + ); + let instruction = &dfg[instruction_id]; self.brillig_context.set_call_stack(dfg.get_call_stack(instruction_id)); @@ -267,6 +283,7 @@ impl<'block> BrilligBlock<'block> { ); match assert_message { Some(ConstrainError::UserDefined(selector, values)) => { + // let opcode_count_before = self.brillig_context.get_current_opcode_count(); let payload_values = vecmap(values, |value| self.convert_ssa_value(*value, dfg)); let payload_as_params = vecmap(values, |value| { @@ -279,6 +296,10 @@ impl<'block> BrilligBlock<'block> { payload_as_params, selector.as_u64(), ); + // println!( + // "Opcode count for revert data: {}", + // self.brillig_context.get_current_opcode_count() - opcode_count_before + // ); } Some(ConstrainError::Intrinsic(message)) => { self.brillig_context.codegen_constrain(condition, Some(message.clone())); @@ -777,9 +798,6 @@ impl<'block> BrilligBlock<'block> { .brillig_context .codegen_pre_call_save_registers_prep_args(&argument_registers, &variables_to_save); - // We don't save and restore constants, so we dump them before a external call since the callee might use the registers where they are allocated. - self.variables.dump_constants(); - // Call instruction, which will interpret above registers 0..num args self.brillig_context.add_external_call_instruction(func_id); @@ -1515,6 +1533,12 @@ impl<'block> BrilligBlock<'block> { } } + fn initialize_constants(&mut self, constants: &[ValueId], dfg: &DataFlowGraph) { + for &constant_id in constants { + self.convert_ssa_value(constant_id, dfg); + } + } + /// Converts an SSA `ValueId` into a `RegisterOrMemory`. Initializes if necessary. fn convert_ssa_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> BrilligVariable { let value_id = dfg.resolve(value_id); @@ -1530,11 +1554,15 @@ impl<'block> BrilligBlock<'block> { Value::NumericConstant { constant, .. } => { // Constants might have been converted previously or not, so we get or create and // (re)initialize the value inside. - if let Some(variable) = self.variables.get_constant(value_id, dfg) { - variable + if self.variables.is_allocated(&value_id) { + self.variables.get_allocation(self.function_context, value_id, dfg) } else { - let new_variable = - self.variables.allocate_constant(self.brillig_context, value_id, dfg); + let new_variable = self.variables.define_variable( + self.function_context, + self.brillig_context, + value_id, + dfg, + ); self.brillig_context .const_instruction(new_variable.extract_single_addr(), *constant); @@ -1542,11 +1570,15 @@ impl<'block> BrilligBlock<'block> { } } Value::Array { array, typ } => { - if let Some(variable) = self.variables.get_constant(value_id, dfg) { - variable + if self.variables.is_allocated(&value_id) { + self.variables.get_allocation(self.function_context, value_id, dfg) } else { - let new_variable = - self.variables.allocate_constant(self.brillig_context, value_id, dfg); + let new_variable = self.variables.define_variable( + self.function_context, + self.brillig_context, + value_id, + dfg, + ); // Initialize the variable let pointer = match new_variable { @@ -1586,8 +1618,12 @@ impl<'block> BrilligBlock<'block> { // around values representing function pointers, even though // there is no interaction with the function possible given that // value. - let new_variable = - self.variables.allocate_constant(self.brillig_context, value_id, dfg); + let new_variable = self.variables.define_variable( + self.function_context, + self.brillig_context, + value_id, + dfg, + ); self.brillig_context.const_instruction( new_variable.extract_single_addr(), @@ -1735,18 +1771,10 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.mov_instruction(write_pointer_register, pointer); for (element_idx, element_id) in data.iter().enumerate() { - if let Some((constant, typ)) = dfg.get_numeric_constant_with_type(*element_id) { - self.brillig_context.indirect_const_instruction( - write_pointer_register, - typ.bit_size(), - constant, - ); - } else { - let element_variable = self.convert_ssa_value(*element_id, dfg); - // Store the item in memory - self.brillig_context - .codegen_store_variable_in_pointer(write_pointer_register, element_variable); - } + let element_variable = self.convert_ssa_value(*element_id, dfg); + // Store the item in memory + self.brillig_context + .codegen_store_variable_in_pointer(write_pointer_register, element_variable); if element_idx != data.len() - 1 { // Increment the write_pointer_register diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index 63b2073c654..90af2e42211 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -1,5 +1,5 @@ use acvm::FieldElement; -use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use fxhash::FxHashSet as HashSet; use crate::{ brillig::brillig_ir::{ @@ -22,16 +22,15 @@ use super::brillig_fn::FunctionContext; #[derive(Debug, Default)] pub(crate) struct BlockVariables { available_variables: HashSet, - available_constants: HashMap, } impl BlockVariables { /// Creates a BlockVariables instance. It uses the variables that are live in to the block and the global available variables (block parameters) pub(crate) fn new(live_in: HashSet) -> Self { - BlockVariables { available_variables: live_in, ..Default::default() } + BlockVariables { available_variables: live_in } } - /// Returns all non-constant variables that have not been removed at this point. + /// Returns all variables that have not been removed at this point. pub(crate) fn get_available_variables( &self, function_context: &FunctionContext, @@ -48,7 +47,7 @@ impl BlockVariables { .collect() } - /// For a given SSA non constant value id, define the variable and return the corresponding cached allocation. + /// For a given SSA value id, define the variable and return the corresponding cached allocation. pub(crate) fn define_variable( &mut self, function_context: &mut FunctionContext, @@ -97,6 +96,11 @@ impl BlockVariables { }); } + /// Checks if a variable is allocated. + pub(crate) fn is_allocated(&self, value_id: &ValueId) -> bool { + self.available_variables.contains(value_id) + } + /// For a given SSA value id, return the corresponding cached allocation. pub(crate) fn get_allocation( &mut self, @@ -105,48 +109,16 @@ impl BlockVariables { dfg: &DataFlowGraph, ) -> BrilligVariable { let value_id = dfg.resolve(value_id); - if let Some(constant) = self.available_constants.get(&value_id) { - *constant - } else { - assert!( - self.available_variables.contains(&value_id), - "ICE: ValueId {value_id:?} is not available" - ); - - *function_context - .ssa_value_allocations - .get(&value_id) - .unwrap_or_else(|| panic!("ICE: Value not found in cache {value_id}")) - } - } - - /// Creates a constant. Constants are a special case in SSA, since they are "defined" every time they are used. - /// We keep constants block-local. - pub(crate) fn allocate_constant( - &mut self, - brillig_context: &mut BrilligContext, - value_id: ValueId, - dfg: &DataFlowGraph, - ) -> BrilligVariable { - let value_id = dfg.resolve(value_id); - let constant = allocate_value(value_id, brillig_context, dfg); - self.available_constants.insert(value_id, constant); - constant - } - /// Gets a constant. - pub(crate) fn get_constant( - &mut self, - value_id: ValueId, - dfg: &DataFlowGraph, - ) -> Option { - let value_id = dfg.resolve(value_id); - self.available_constants.get(&value_id).cloned() - } + assert!( + self.available_variables.contains(&value_id), + "ICE: ValueId {value_id:?} is not available" + ); - /// Removes the allocations of all constants. Constants will need to be reallocated and reinitialized after this. - pub(crate) fn dump_constants(&mut self) { - self.available_constants.clear(); + *function_context + .ssa_value_allocations + .get(&value_id) + .unwrap_or_else(|| panic!("ICE: Value not found in cache {value_id}")) } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index c1abad17a8f..2779be103cd 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -15,7 +15,7 @@ use crate::{ }; use fxhash::FxHashMap as HashMap; -use super::variable_liveness::VariableLiveness; +use super::{constant_allocation::ConstantAllocation, variable_liveness::VariableLiveness}; pub(crate) struct FunctionContext { pub(crate) function_id: FunctionId, @@ -25,6 +25,8 @@ pub(crate) struct FunctionContext { pub(crate) blocks: Vec, /// Liveness information for each variable in the function. pub(crate) liveness: VariableLiveness, + /// Information on where to allocate constants + pub(crate) constant_allocation: ConstantAllocation, } impl FunctionContext { @@ -36,11 +38,15 @@ impl FunctionContext { reverse_post_order.extend_from_slice(PostOrder::with_function(function).as_slice()); reverse_post_order.reverse(); + let constants = ConstantAllocation::from_function(function); + let liveness = VariableLiveness::from_function(function, &constants); + Self { function_id: id, ssa_value_allocations: HashMap::default(), blocks: reverse_post_order, - liveness: VariableLiveness::from_function(function), + liveness, + constant_allocation: constants, } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs new file mode 100644 index 00000000000..13f3a2a6681 --- /dev/null +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -0,0 +1,135 @@ +use fxhash::FxHashMap as HashMap; + +use crate::ssa::ir::{ + basic_block::BasicBlockId, + cfg::ControlFlowGraph, + dfg::DataFlowGraph, + dom::DominatorTree, + function::Function, + instruction::InstructionId, + post_order::PostOrder, + value::{Value, ValueId}, +}; + +use super::variable_liveness::{collect_variables_of_value, variables_used_in_instruction}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub(crate) enum InstructionLocation { + Instruction(InstructionId), + Terminator, +} + +pub(crate) struct ConstantAllocation { + constant_usage: HashMap>>, + allocation_points: HashMap>>, + dominator_tree: DominatorTree, +} + +impl ConstantAllocation { + pub(crate) fn from_function(func: &Function) -> Self { + let cfg = ControlFlowGraph::with_function(func); + let post_order = PostOrder::with_function(func); + let dominator_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); + let mut instance = ConstantAllocation { + constant_usage: HashMap::default(), + allocation_points: HashMap::default(), + dominator_tree, + }; + instance.collect_constant_usage(func); + instance.decide_allocation_points(); + + instance + } + + pub(crate) fn allocated_in_block(&self, block_id: BasicBlockId) -> Vec { + self.allocation_points.get(&block_id).map_or(Vec::default(), |allocations| { + allocations.iter().flat_map(|(_, constants)| constants.iter()).copied().collect() + }) + } + + pub(crate) fn allocated_at_location(&self, block_id: BasicBlockId, location: InstructionLocation) -> Vec { + self.allocation_points.get(&block_id).map_or(Vec::default(), |allocations| { + allocations.get(&location).map_or(Vec::default(), |constants| constants.clone()) + }) + } + + fn collect_constant_usage(&mut self, func: &Function) { + let mut record_if_constant = + |block_id: BasicBlockId, value_id: ValueId, location: InstructionLocation| { + if is_constant_value(value_id, &func.dfg) { + self.constant_usage + .entry(value_id) + .or_default() + .entry(block_id) + .or_default() + .push(location); + } + }; + for block_id in func.reachable_blocks() { + let block = &func.dfg[block_id]; + for &inst_id in block.instructions() { + let variables = variables_used_in_instruction(&func.dfg[inst_id], &func.dfg); + for variable in variables { + record_if_constant( + block_id, + variable, + InstructionLocation::Instruction(inst_id), + ); + } + } + if let Some(terminator_instruction) = block.terminator() { + terminator_instruction.for_each_value(|value_id| { + let variables = collect_variables_of_value(value_id, &func.dfg); + for variable in variables { + record_if_constant(block_id, variable, InstructionLocation::Terminator); + } + }); + } + } + } + + fn decide_allocation_points(&mut self) { + for (constant_id, usage_in_blocks) in self.constant_usage.iter() { + let block_ids: Vec<_> = usage_in_blocks.iter().map(|(block_id, _)| *block_id).collect(); + + let common_dominator = self.common_dominator(&block_ids); + + // If the common dominator is one of the places where it's used, we take the first usage in the common dominator. + // Otherwise, we allocate it at the terminator of the common dominator. + let location = if let Some(locations_in_common_dominator) = + usage_in_blocks.get(&common_dominator) + { + *locations_in_common_dominator + .first() + .expect("At least one location must have been found") + } else { + InstructionLocation::Terminator + }; + + self.allocation_points + .entry(common_dominator) + .or_default() + .entry(location) + .or_default() + .push(*constant_id); + } + } + + fn common_dominator(&self, block_ids: &[BasicBlockId]) -> BasicBlockId { + if block_ids.len() == 1 { + return block_ids[0]; + } + + let mut common_dominator = block_ids[0]; + + for block_id in block_ids.iter().skip(1) { + common_dominator = self.dominator_tree.common_dominator(common_dominator, *block_id); + } + + common_dominator + } +} + +pub(crate) fn is_constant_value(id: ValueId, dfg: &DataFlowGraph) -> bool { + matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. } | Value::Array { .. }) +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs index 52eded81919..73e88cee676 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -1,5 +1,6 @@ //! This module analyzes the liveness of variables (non-constant values) throughout a function. //! It uses the approach detailed in the section 4.2 of this paper https://inria.hal.science/inria-00558509v2/document + use crate::ssa::ir::{ basic_block::{BasicBlock, BasicBlockId}, cfg::ControlFlowGraph, @@ -13,6 +14,8 @@ use crate::ssa::ir::{ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use super::constant_allocation::ConstantAllocation; + /// A back edge is an edge from a node to one of its ancestors. It denotes a loop in the CFG. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] struct BackEdge { @@ -42,7 +45,7 @@ fn find_back_edges( } /// Collects the underlying variables inside a value id. It might be more than one, for example in constant arrays that are constructed with multiple vars. -fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { +pub(crate) fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { let value_id = dfg.resolve(value_id); let value = &dfg[value_id]; @@ -52,7 +55,7 @@ fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { - let mut value_ids = Vec::new(); + let mut value_ids = vec![value_id]; array.iter().for_each(|item_id| { let underlying_ids = collect_variables_of_value(*item_id, dfg); @@ -61,19 +64,21 @@ fn collect_variables_of_value(value_id: ValueId, dfg: &DataFlowGraph) -> Vec { + vec![value_id] + } // Functions are not variables in a defunctionalized SSA. Only constant function values should appear. - Value::ForeignFunction(_) - | Value::Function(_) - | Value::Intrinsic(..) - // Constants are not treated as variables for the variable liveness analysis, since they are defined every time they are used. - | Value::NumericConstant { .. } => { + Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => { vec![] } } } -fn variables_used_in_instruction(instruction: &Instruction, dfg: &DataFlowGraph) -> Vec { - let mut used = Vec::new(); +pub(crate) fn variables_used_in_instruction( + instruction: &Instruction, + dfg: &DataFlowGraph, +) -> Variables { + let mut used = HashSet::default(); instruction.for_each_value(|value_id| { let underlying_ids = collect_variables_of_value(value_id, dfg); @@ -83,8 +88,8 @@ fn variables_used_in_instruction(instruction: &Instruction, dfg: &DataFlowGraph) used } -fn variables_used_in_block(block: &BasicBlock, dfg: &DataFlowGraph) -> Vec { - let mut used: Vec = block +fn variables_used_in_block(block: &BasicBlock, dfg: &DataFlowGraph) -> Variables { + let mut used: Variables = block .instructions() .iter() .flat_map(|instruction_id| { @@ -124,6 +129,7 @@ type LastUses = HashMap; pub(crate) struct VariableLiveness { cfg: ControlFlowGraph, post_order: PostOrder, + dominator_tree: DominatorTree, /// The variables that are alive before the block starts executing live_in: HashMap, /// The variables that stop being alive after each specific instruction @@ -134,13 +140,15 @@ pub(crate) struct VariableLiveness { impl VariableLiveness { /// Computes the liveness of variables throughout a function. - pub(crate) fn from_function(func: &Function) -> Self { + pub(crate) fn from_function(func: &Function, constants: &ConstantAllocation) -> Self { let cfg = ControlFlowGraph::with_function(func); let post_order = PostOrder::with_function(func); + let dominator_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); let mut instance = Self { cfg, post_order, + dominator_tree, live_in: HashMap::default(), last_uses: HashMap::default(), param_definitions: HashMap::default(), @@ -148,7 +156,7 @@ impl VariableLiveness { instance.compute_block_param_definitions(func); - instance.compute_live_in_of_blocks(func); + instance.compute_live_in_of_blocks(func, constants); instance.compute_last_uses(func); @@ -182,8 +190,6 @@ impl VariableLiveness { } fn compute_block_param_definitions(&mut self, func: &Function) { - let tree = DominatorTree::with_cfg_and_post_order(&self.cfg, &self.post_order); - // Going in reverse post order to process the entry block first let mut reverse_post_order = Vec::new(); reverse_post_order.extend_from_slice(self.post_order.as_slice()); @@ -191,18 +197,19 @@ impl VariableLiveness { for block in reverse_post_order { let params = func.dfg[block].parameters(); // If it has no dominator, it's the entry block - let dominator_block = tree.immediate_dominator(block).unwrap_or(func.entry_block()); + let dominator_block = + self.dominator_tree.immediate_dominator(block).unwrap_or(func.entry_block()); let definitions_for_the_dominator = self.param_definitions.entry(dominator_block).or_default(); definitions_for_the_dominator.extend(params.iter()); } } - fn compute_live_in_of_blocks(&mut self, func: &Function) { + fn compute_live_in_of_blocks(&mut self, func: &Function, constants: &ConstantAllocation) { let back_edges = find_back_edges(func, &self.cfg, &self.post_order); // First pass, propagate up the live_ins skipping back edges - self.compute_live_in_recursive(func, func.entry_block(), &back_edges); + self.compute_live_in_recursive(func, func.entry_block(), &back_edges, constants); // Second pass, propagate header live_ins to the loop bodies for back_edge in back_edges { @@ -215,8 +222,11 @@ impl VariableLiveness { func: &Function, block_id: BasicBlockId, back_edges: &HashSet, + constants: &ConstantAllocation, ) { - let defined = self.compute_defined_variables(block_id, &func.dfg); + let mut defined = self.compute_defined_variables(block_id, &func.dfg); + + defined.extend(constants.allocated_in_block(block_id)); let block: &BasicBlock = &func.dfg[block_id]; @@ -227,7 +237,7 @@ impl VariableLiveness { for successor_id in block.successors() { if !back_edges.contains(&BackEdge { start: block_id, header: successor_id }) { if !self.live_in.contains_key(&successor_id) { - self.compute_live_in_recursive(func, successor_id, back_edges); + self.compute_live_in_recursive(func, successor_id, back_edges, constants); } live_out.extend( self.live_in @@ -332,6 +342,7 @@ impl VariableLiveness { mod test { use fxhash::FxHashSet; + use crate::brillig::brillig_gen::constant_allocation::ConstantAllocation; use crate::brillig::brillig_gen::variable_liveness::VariableLiveness; use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::function::RuntimeType; @@ -403,11 +414,18 @@ mod test { let ssa = builder.finish(); let func = ssa.main(); - let liveness = VariableLiveness::from_function(func); + let constants = ConstantAllocation::from_function(func); + let liveness = VariableLiveness::from_function(func, &constants); assert!(liveness.get_live_in(&func.entry_block()).is_empty()); - assert_eq!(liveness.get_live_in(&b2), &FxHashSet::from_iter([v3, v0].into_iter())); - assert_eq!(liveness.get_live_in(&b1), &FxHashSet::from_iter([v3, v1].into_iter())); + assert_eq!( + liveness.get_live_in(&b2), + &FxHashSet::from_iter([v3, v0, twenty_seven].into_iter()) + ); + assert_eq!( + liveness.get_live_in(&b1), + &FxHashSet::from_iter([v3, v1, twenty_seven].into_iter()) + ); assert_eq!(liveness.get_live_in(&b3), &FxHashSet::from_iter([v3].into_iter())); let block_1 = &func.dfg[b1]; @@ -415,11 +433,11 @@ mod test { let block_3 = &func.dfg[b3]; assert_eq!( liveness.get_last_uses(&b1).get(&block_1.instructions()[0]), - Some(&FxHashSet::from_iter([v1].into_iter())) + Some(&FxHashSet::from_iter([v1, twenty_seven].into_iter())) ); assert_eq!( liveness.get_last_uses(&b2).get(&block_2.instructions()[0]), - Some(&FxHashSet::from_iter([v0].into_iter())) + Some(&FxHashSet::from_iter([v0, twenty_seven].into_iter())) ); assert_eq!( liveness.get_last_uses(&b3).get(&block_3.instructions()[0]), @@ -548,7 +566,8 @@ mod test { let ssa = builder.finish(); let func = ssa.main(); - let liveness = VariableLiveness::from_function(func); + let constants = ConstantAllocation::from_function(func); + let liveness = VariableLiveness::from_function(func, &constants); assert!(liveness.get_live_in(&func.entry_block()).is_empty()); assert_eq!(liveness.get_live_in(&b1), &FxHashSet::from_iter([v0, v1, v3, v4].into_iter())); @@ -558,18 +577,21 @@ mod test { liveness.get_live_in(&b4), &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) ); - assert_eq!(liveness.get_live_in(&b6), &FxHashSet::from_iter([v0, v1, v3, v4].into_iter())); + assert_eq!( + liveness.get_live_in(&b6), + &FxHashSet::from_iter([v0, v1, v3, v4, one].into_iter()) + ); assert_eq!( liveness.get_live_in(&b5), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b7), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b8), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) ); let block_3 = &func.dfg[b3]; @@ -621,7 +643,9 @@ mod test { let ssa = builder.finish(); let func = ssa.main(); - let liveness = VariableLiveness::from_function(func); + + let constants = ConstantAllocation::from_function(func); + let liveness = VariableLiveness::from_function(func, &constants); // Entry point defines its own params and also b3's params. assert_eq!(liveness.defined_block_params(&func.entry_block()), vec![v0, v1, v2]); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 05117b96c5d..0c5a58e9282 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -148,6 +148,10 @@ impl BrilligContext< pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) { self.obj.set_call_stack(call_stack); } + + pub(crate) fn get_current_opcode_count(&self) -> usize { + self.obj.index_of_next_opcode() + } } #[cfg(test)] diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs index 15fa3bad38d..94f7a405c05 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -214,7 +214,7 @@ impl DominatorTree { /// Compute the common dominator of two basic blocks. /// /// Both basic blocks are assumed to be reachable. - fn common_dominator( + pub(crate) fn common_dominator( &self, mut block_a_id: BasicBlockId, mut block_b_id: BasicBlockId, From f750f93ac28dde4240beadb8bd64d80b787f98ef Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 30 Aug 2024 14:46:33 +0000 Subject: [PATCH 2/5] clippy --- .../src/brillig/brillig_gen/constant_allocation.rs | 6 +++++- .../compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs index 13f3a2a6681..7ebac76f20f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -47,7 +47,11 @@ impl ConstantAllocation { }) } - pub(crate) fn allocated_at_location(&self, block_id: BasicBlockId, location: InstructionLocation) -> Vec { + pub(crate) fn allocated_at_location( + &self, + block_id: BasicBlockId, + location: InstructionLocation, + ) -> Vec { self.allocation_points.get(&block_id).map_or(Vec::default(), |allocations| { allocations.get(&location).map_or(Vec::default(), |constants| constants.clone()) }) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 0c5a58e9282..18276d79415 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -149,9 +149,9 @@ impl BrilligContext< self.obj.set_call_stack(call_stack); } - pub(crate) fn get_current_opcode_count(&self) -> usize { - self.obj.index_of_next_opcode() - } + // pub(crate) fn get_current_opcode_count(&self) -> usize { + // self.obj.index_of_next_opcode() + // } } #[cfg(test)] From cb2d91b0cd495722c82dfdf540ae9a8466e880d0 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 2 Sep 2024 15:29:18 +0000 Subject: [PATCH 3/5] remove commented out code --- .../compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 18276d79415..05117b96c5d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -148,10 +148,6 @@ impl BrilligContext< pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) { self.obj.set_call_stack(call_stack); } - - // pub(crate) fn get_current_opcode_count(&self) -> usize { - // self.obj.index_of_next_opcode() - // } } #[cfg(test)] From 862ef46f8a84c4096f6801a5af3c7ae7a37f56db Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 2 Sep 2024 15:30:59 +0000 Subject: [PATCH 4/5] fix: initalize with the proper call stack --- .../src/brillig/brillig_gen/brillig_block.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index a166dbef4f5..bebe7a0ae7b 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -249,6 +249,9 @@ impl<'block> BrilligBlock<'block> { /// Converts an SSA instruction into a sequence of Brillig opcodes. fn convert_ssa_instruction(&mut self, instruction_id: InstructionId, dfg: &DataFlowGraph) { + let instruction = &dfg[instruction_id]; + self.brillig_context.set_call_stack(dfg.get_call_stack(instruction_id)); + self.initialize_constants( &self.function_context.constant_allocation.allocated_at_location( self.block_id, @@ -256,10 +259,6 @@ impl<'block> BrilligBlock<'block> { ), dfg, ); - - let instruction = &dfg[instruction_id]; - self.brillig_context.set_call_stack(dfg.get_call_stack(instruction_id)); - match instruction { Instruction::Binary(binary) => { let result_var = self.variables.define_single_addr_variable( From 6e3c17626e2d6710fdb869265f10a7bac33d22ed Mon Sep 17 00:00:00 2001 From: sirasistant Date: Mon, 2 Sep 2024 15:33:18 +0000 Subject: [PATCH 5/5] docs --- .../src/brillig/brillig_gen/constant_allocation.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs index 7ebac76f20f..cf484fa5038 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -1,3 +1,6 @@ +//! This module analyzes the usage of constants in a given function and decides an allocation point for them. +//! The allocation point will be the common dominator of all the places where the constant is used. +//! By allocating in the common dominator, we can cache the constants for all subsequent uses. use fxhash::FxHashMap as HashMap; use crate::ssa::ir::{