From 2d19468bd88761a07c87c1a4f978e67b46a58888 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 26 Nov 2024 12:08:43 -0300 Subject: [PATCH 01/18] Add a test that fails --- .../src/ssa/opt/constant_folding.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 019bace33a..78f1b06e82 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1341,4 +1341,26 @@ mod test { let ssa = ssa.fold_constants_with_brillig(&brillig); assert_normalized_ssa_equals(ssa, expected); } + + #[test] + fn does_not_use_cached_constrain_in_block_that_is_not_dominated() { + let src = " + brillig(inline) fn main f0 { + b0(v0: Field, v1: Field): + v3 = eq v0, Field 0 + jmpif v3 then: b1, else: b2 + b1(): + v5 = eq v1, Field 1 + constrain v1 == Field 1 + jmp b2() + b2(): + v6 = eq v1, Field 0 + constrain v1 == Field 0 + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.fold_constants_using_constraints(); + assert_normalized_ssa_equals(ssa, src); + } } From 98d246b520bb1bcca8065b0f2477f78b3156d57f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 26 Nov 2024 15:51:54 +0000 Subject: [PATCH 02/18] Add simplify function --- .../src/ssa/opt/constant_folding.rs | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 78f1b06e82..a669cfb55a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -377,26 +377,8 @@ impl<'brillig> Context<'brillig> { // to map from the more complex to the simpler value. if let Instruction::Constrain(lhs, rhs, _) = instruction { // These `ValueId`s should be fully resolved now. - match (&dfg[lhs], &dfg[rhs]) { - // Ignore trivial constraints - (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => (), - - // Prefer replacing with constants where possible. - (Value::NumericConstant { .. }, _) => { - self.get_constraint_map(side_effects_enabled_var).insert(rhs, lhs); - } - (_, Value::NumericConstant { .. }) => { - self.get_constraint_map(side_effects_enabled_var).insert(lhs, rhs); - } - // Otherwise prefer block parameters over instruction results. - // This is as block parameters are more likely to be a single witness rather than a full expression. - (Value::Param { .. }, Value::Instruction { .. }) => { - self.get_constraint_map(side_effects_enabled_var).insert(rhs, lhs); - } - (Value::Instruction { .. }, Value::Param { .. }) => { - self.get_constraint_map(side_effects_enabled_var).insert(lhs, rhs); - } - (_, _) => (), + if let Some((complex, simple)) = simplify(dfg, lhs, rhs) { + self.get_constraint_map(side_effects_enabled_var).insert(complex, simple); } } } @@ -687,6 +669,25 @@ fn value_id_to_calldata(value_id: ValueId, dfg: &DataFlowGraph, calldata: &mut V panic!("Expected ValueId to be numeric constant or array constant"); } +/// Check if one expression is simpler than the other. +/// Returns `Some((complex, simple))` if a simplification was found, otherwise `None`. +/// Expects the `ValueId`s to be fully resolved. +fn simplify(dfg: &DataFlowGraph, lhs: ValueId, rhs: ValueId) -> Option<(ValueId, ValueId)> { + match (&dfg[lhs], &dfg[rhs]) { + // Ignore trivial constraints + (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => None, + + // Prefer replacing with constants where possible. + (Value::NumericConstant { .. }, _) => Some((rhs, lhs)), + (_, Value::NumericConstant { .. }) => Some((lhs, rhs)), + // Otherwise prefer block parameters over instruction results. + // This is as block parameters are more likely to be a single witness rather than a full expression. + (Value::Param { .. }, Value::Instruction { .. }) => Some((rhs, lhs)), + (Value::Instruction { .. }, Value::Param { .. }) => Some((lhs, rhs)), + (_, _) => None, + } +} + #[cfg(test)] mod test { use std::sync::Arc; From 2dec436da780da209e5f304401847fd2a4fbb608 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 26 Nov 2024 16:04:44 +0000 Subject: [PATCH 03/18] Add SimplificationCache --- .../src/ssa/opt/constant_folding.rs | 47 ++++++++++++++++--- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index a669cfb55a..a6b47cda07 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -160,6 +160,9 @@ impl Function { } } +/// `ValueId` in an `EnableConstraintsIf` instruction. +type Predicate = ValueId; + struct Context<'a> { use_constraint_info: bool, brillig_info: Option>, @@ -174,7 +177,7 @@ struct Context<'a> { /// We partition the maps of constrained values according to the side-effects flag at the point /// at which the values are constrained. This prevents constraints which are only sometimes enforced /// being used to modify the rest of the program. - constraint_simplification_mappings: HashMap>, + constraint_simplification_mappings: ConstraitSimplificationCache, // Cache of instructions without any side-effects along with their outputs. cached_instruction_results: InstructionResultCache, @@ -188,12 +191,39 @@ pub(crate) struct BrilligInfo<'a> { brillig_functions: &'a BTreeMap, } +struct SimplificationCache { + simplified: ValueId, + blocks: HashSet, +} + +impl SimplificationCache { + fn new(simplified: ValueId) -> Self { + Self { simplified, blocks: Default::default() } + } + fn merge(&mut self, dfg: &DataFlowGraph, other: ValueId, block: BasicBlockId) { + if self.simplified == other { + self.blocks.insert(block); + } else { + match simplify(dfg, self.simplified, other) { + Some((complex, simple)) if self.simplified == complex => { + self.simplified = simple; + self.blocks.clear(); + self.blocks.insert(block); + } + _ => {} + } + } + } +} + +type ConstraitSimplificationCache = HashMap>; + /// HashMap from (Instruction, side_effects_enabled_var) to the results of the instruction. /// Stored as a two-level map to avoid cloning Instructions during the `.get` call. /// /// In addition to each result, the original BasicBlockId is stored as well. This allows us /// to deduplicate instructions across blocks as long as the new block dominates the original. -type InstructionResultCache = HashMap, ResultCache>>; +type InstructionResultCache = HashMap, ResultCache>>; /// Records the results of all duplicate [`Instruction`]s along with the blocks in which they sit. /// @@ -308,7 +338,7 @@ impl<'brillig> Context<'brillig> { fn resolve_instruction( instruction_id: InstructionId, dfg: &DataFlowGraph, - constraint_simplification_mapping: &HashMap, + constraint_simplification_mapping: &HashMap, ) -> Instruction { let instruction = dfg[instruction_id].clone(); @@ -319,12 +349,12 @@ impl<'brillig> Context<'brillig> { // constraints to the cache. fn resolve_cache( dfg: &DataFlowGraph, - cache: &HashMap, + cache: &HashMap, value_id: ValueId, ) -> ValueId { let resolved_id = dfg.resolve(value_id); match cache.get(&resolved_id) { - Some(cached_value) => resolve_cache(dfg, cache, *cached_value), + Some(cached_value) => resolve_cache(dfg, cache, cached_value.simplified), None => resolved_id, } } @@ -378,7 +408,10 @@ impl<'brillig> Context<'brillig> { if let Instruction::Constrain(lhs, rhs, _) = instruction { // These `ValueId`s should be fully resolved now. if let Some((complex, simple)) = simplify(dfg, lhs, rhs) { - self.get_constraint_map(side_effects_enabled_var).insert(complex, simple); + self.get_constraint_map(side_effects_enabled_var) + .entry(complex) + .or_insert_with(|| SimplificationCache::new(simple)) + .merge(dfg, simple, block); } } } @@ -402,7 +435,7 @@ impl<'brillig> Context<'brillig> { fn get_constraint_map( &mut self, side_effects_enabled_var: ValueId, - ) -> &mut HashMap { + ) -> &mut HashMap { self.constraint_simplification_mappings.entry(side_effects_enabled_var).or_default() } From f27b1d251db4ee60453c3fe6cdffd56dbec8cafd Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 26 Nov 2024 16:18:22 +0000 Subject: [PATCH 04/18] Add docs --- .../src/ssa/opt/constant_folding.rs | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index a6b47cda07..227ad06fab 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -177,7 +177,7 @@ struct Context<'a> { /// We partition the maps of constrained values according to the side-effects flag at the point /// at which the values are constrained. This prevents constraints which are only sometimes enforced /// being used to modify the rest of the program. - constraint_simplification_mappings: ConstraitSimplificationCache, + constraint_simplification_mappings: ConstraintSimplificationCache, // Cache of instructions without any side-effects along with their outputs. cached_instruction_results: InstructionResultCache, @@ -191,15 +191,26 @@ pub(crate) struct BrilligInfo<'a> { brillig_functions: &'a BTreeMap, } +/// Records a simplified equivalent of an [`Instruction`]s along with the blocks in which the +/// constraint that advised the simplification has been encountered. +/// +/// For more information see [`ConstraintSimplificationCache`]. struct SimplificationCache { simplified: ValueId, blocks: HashSet, } impl SimplificationCache { + /// Create a new simplification record. + /// + /// Will immediately be followed on by a call to `merge` to add the block. fn new(simplified: ValueId) -> Self { Self { simplified, blocks: Default::default() } } + + /// Called with a newly encountered simplification of the original expression: + /// if it's the same simplified value then we record the new block we see it in, + /// otherwise we keep the simpler of the new and the existing value. fn merge(&mut self, dfg: &DataFlowGraph, other: ValueId, block: BasicBlockId) { if self.simplified == other { self.blocks.insert(block); @@ -216,7 +227,13 @@ impl SimplificationCache { } } -type ConstraitSimplificationCache = HashMap>; +/// HashMap from Instruction to a simplified expression that it can be replaced with based on +/// constraints that testify to their equivalence, stored together with the set of blocks at which +/// this constraint has been observed. Only blocks dominated by one in the cache should have +/// access to this information, otherwise we create a sort of time paradox where we replace +/// an instruction with a constant we believe _should_ be true about it, without ever actually +/// producing and asserting the value. +type ConstraintSimplificationCache = HashMap>; /// HashMap from (Instruction, side_effects_enabled_var) to the results of the instruction. /// Stored as a two-level map to avoid cloning Instructions during the `.get` call. From 4e9c1403512f1ba7fac82ec38158c04443049ad4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 26 Nov 2024 17:19:20 +0000 Subject: [PATCH 05/18] Fix bug by restriting view --- .../src/ssa/opt/constant_folding.rs | 52 +++++++++++++------ 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 227ad06fab..76da5ec826 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -146,7 +146,7 @@ impl Function { use_constraint_info: bool, brillig_info: Option, ) { - let mut context = Context::new(self, use_constraint_info, brillig_info); + let mut context = Context::new(use_constraint_info, brillig_info); context.block_queue.push_back(self.entry_block()); while let Some(block) = context.block_queue.pop_front() { @@ -181,8 +181,6 @@ struct Context<'a> { // Cache of instructions without any side-effects along with their outputs. cached_instruction_results: InstructionResultCache, - - dom: DominatorTree, } #[derive(Copy, Clone)] @@ -225,6 +223,14 @@ impl SimplificationCache { } } } + + fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option { + if self.blocks.iter().any(|b| dom.dominates(*b, block)) { + Some(self.simplified) + } else { + None + } + } } /// HashMap from Instruction to a simplified expression that it can be replaced with based on @@ -251,11 +257,7 @@ struct ResultCache { } impl<'brillig> Context<'brillig> { - fn new( - function: &Function, - use_constraint_info: bool, - brillig_info: Option>, - ) -> Self { + fn new(use_constraint_info: bool, brillig_info: Option>) -> Self { Self { use_constraint_info, brillig_info, @@ -263,7 +265,6 @@ impl<'brillig> Context<'brillig> { block_queue: Default::default(), constraint_simplification_mappings: Default::default(), cached_instruction_results: Default::default(), - dom: DominatorTree::with_function(function), } } @@ -273,9 +274,12 @@ impl<'brillig> Context<'brillig> { let mut side_effects_enabled_var = function.dfg.make_constant(FieldElement::one(), Type::bool()); + let mut dom = DominatorTree::with_function(function); + for instruction_id in instructions { self.fold_constants_into_instruction( &mut function.dfg, + &mut dom, block, instruction_id, &mut side_effects_enabled_var, @@ -287,17 +291,21 @@ impl<'brillig> Context<'brillig> { fn fold_constants_into_instruction( &mut self, dfg: &mut DataFlowGraph, + dom: &mut DominatorTree, mut block: BasicBlockId, id: InstructionId, side_effects_enabled_var: &mut ValueId, ) { let constraint_simplification_mapping = self.get_constraint_map(*side_effects_enabled_var); - let instruction = Self::resolve_instruction(id, dfg, constraint_simplification_mapping); + + let instruction = + Self::resolve_instruction(id, block, dfg, dom, constraint_simplification_mapping); + let old_results = dfg.instruction_results(id).to_vec(); // If a copy of this instruction exists earlier in the block, then reuse the previous results. if let Some(cache_result) = - self.get_cached(dfg, &instruction, *side_effects_enabled_var, block) + self.get_cached(dfg, dom, &instruction, *side_effects_enabled_var, block) { match cache_result { CacheResult::Cached(cached) => { @@ -354,7 +362,9 @@ impl<'brillig> Context<'brillig> { /// Fetches an [`Instruction`] by its [`InstructionId`] and fully resolves its inputs. fn resolve_instruction( instruction_id: InstructionId, + block: BasicBlockId, dfg: &DataFlowGraph, + dom: &mut DominatorTree, constraint_simplification_mapping: &HashMap, ) -> Instruction { let instruction = dfg[instruction_id].clone(); @@ -365,20 +375,29 @@ impl<'brillig> Context<'brillig> { // This allows us to reach a stable final `ValueId` for each instruction input as we add more // constraints to the cache. fn resolve_cache( + block: BasicBlockId, dfg: &DataFlowGraph, + dom: &mut DominatorTree, cache: &HashMap, value_id: ValueId, ) -> ValueId { let resolved_id = dfg.resolve(value_id); match cache.get(&resolved_id) { - Some(cached_value) => resolve_cache(dfg, cache, cached_value.simplified), + Some(simplification_cache) => { + if let Some(simplified) = simplification_cache.get(block, dom) { + resolve_cache(block, dfg, dom, cache, simplified) + } else { + resolved_id + } + } None => resolved_id, } } // Resolve any inputs to ensure that we're comparing like-for-like instructions. - instruction - .map_values(|value_id| resolve_cache(dfg, constraint_simplification_mapping, value_id)) + instruction.map_values(|value_id| { + resolve_cache(block, dfg, dom, constraint_simplification_mapping, value_id) + }) } /// Pushes a new [`Instruction`] into the [`DataFlowGraph`] which applies any optimizations @@ -468,8 +487,9 @@ impl<'brillig> Context<'brillig> { } fn get_cached( - &mut self, + &self, dfg: &DataFlowGraph, + dom: &mut DominatorTree, instruction: &Instruction, side_effects_enabled_var: ValueId, block: BasicBlockId, @@ -479,7 +499,7 @@ impl<'brillig> Context<'brillig> { let predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); let predicate = predicate.then_some(side_effects_enabled_var); - results_for_instruction.get(&predicate)?.get(block, &mut self.dom) + results_for_instruction.get(&predicate)?.get(block, dom) } /// Checks if the given instruction is a call to a brillig function with all constant arguments. From 2c9513024b371ce042117cffd41c4edbc795d1a2 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 26 Nov 2024 17:31:51 +0000 Subject: [PATCH 06/18] Allow multiple blocks --- .../src/ssa/opt/constant_folding.rs | 50 +++++++++---------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 76da5ec826..da49f0e94c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -193,43 +193,39 @@ pub(crate) struct BrilligInfo<'a> { /// constraint that advised the simplification has been encountered. /// /// For more information see [`ConstraintSimplificationCache`]. +#[derive(Default)] struct SimplificationCache { - simplified: ValueId, - blocks: HashSet, + simplifications: HashMap, } impl SimplificationCache { - /// Create a new simplification record. - /// - /// Will immediately be followed on by a call to `merge` to add the block. - fn new(simplified: ValueId) -> Self { - Self { simplified, blocks: Default::default() } - } - - /// Called with a newly encountered simplification of the original expression: - /// if it's the same simplified value then we record the new block we see it in, - /// otherwise we keep the simpler of the new and the existing value. - fn merge(&mut self, dfg: &DataFlowGraph, other: ValueId, block: BasicBlockId) { - if self.simplified == other { - self.blocks.insert(block); - } else { - match simplify(dfg, self.simplified, other) { - Some((complex, simple)) if self.simplified == complex => { - self.simplified = simple; - self.blocks.clear(); - self.blocks.insert(block); + /// Called with a newly encountered simplification. + fn add(&mut self, dfg: &DataFlowGraph, simple: ValueId, block: BasicBlockId) { + let existing = self.simplifications.entry(block).or_insert(simple); + // Keep the simpler expression in this block. + if *existing != simple { + match simplify(dfg, *existing, simple) { + Some((complex, simple)) if *existing == complex => { + *existing = simple; } _ => {} } } } + /// Try to find a simplification in a visible block. fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option { - if self.blocks.iter().any(|b| dom.dominates(*b, block)) { - Some(self.simplified) - } else { - None + // See if we have a direct simplification in this block. + if let Some(value) = self.simplifications.get(&block) { + return Some(*value); + } + // Check if there is a dominating block we can take a simplification from. + for (constraining_block, value) in self.simplifications.iter() { + if dom.dominates(*constraining_block, block) { + return Some(*value); + } } + None } } @@ -446,8 +442,8 @@ impl<'brillig> Context<'brillig> { if let Some((complex, simple)) = simplify(dfg, lhs, rhs) { self.get_constraint_map(side_effects_enabled_var) .entry(complex) - .or_insert_with(|| SimplificationCache::new(simple)) - .merge(dfg, simple, block); + .or_default() + .add(dfg, simple, block); } } } From fd19fb0d6bd19f6dc03605e4bda99b471cf458a6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 26 Nov 2024 14:27:35 -0300 Subject: [PATCH 07/18] Add a regression test for instruction hoisting --- .../src/ssa/opt/constant_folding.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index da49f0e94c..ae97f39ac9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1430,4 +1430,28 @@ mod test { let ssa = ssa.fold_constants_using_constraints(); assert_normalized_ssa_equals(ssa, src); } + + #[test] + fn does_not_hoist_constrain_to_common_ancestor() { + let src = " + brillig(inline) fn main f0 { + b0(v0: Field, v1: Field): + v3 = eq v0, Field 0 + jmpif v3 then: b1, else: b2 + b1(): + constrain v1 == Field 1 + jmp b2() + b2(): + jmpif v0 then: b3, else: b4 + b3(): + constrain v1 == Field 1 // This was incorrectly hoisted to b0 but this condition is not valid when going b0 -> b2 -> b4 + jmp b4() + b4(): + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.fold_constants_using_constraints(); + assert_normalized_ssa_equals(ssa, src); + } } From cd26a1b108429662f31babd0dd7cce1bb048a21a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 26 Nov 2024 15:00:27 -0300 Subject: [PATCH 08/18] Don't hoist instructions that have side effects --- .../src/ssa/opt/constant_folding.rs | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index ae97f39ac9..4aa1f3cef5 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -309,11 +309,13 @@ impl<'brillig> Context<'brillig> { return; } CacheResult::NeedToHoistToCommonBlock(dominator, _cached) => { - // Just change the block to insert in the common dominator instead. - // This will only move the current instance of the instruction right now. - // When constant folding is run a second time later on, it'll catch - // that the previous instance can be deduplicated to this instance. - block = dominator; + if instruction_can_be_hoisted(&instruction, dfg, self.use_constraint_info) { + // Just change the block to insert in the common dominator instead. + // This will only move the current instance of the instruction right now. + // When constant folding is run a second time later on, it'll catch + // that the previous instance can be deduplicated to this instance. + block = dominator; + } } } } @@ -659,6 +661,20 @@ impl<'brillig> Context<'brillig> { } } +fn instruction_can_be_hoisted( + instruction: &Instruction, + dfg: &mut DataFlowGraph, + deduplicate_with_predicate: bool, +) -> bool { + // These two can never be hoisted as they have a side-effect + // (though it's fine to de-duplicate them, just not fine to hoist them) + if matches!(instruction, Instruction::Constrain(..) | Instruction::RangeCheck { .. }) { + return false; + } + + instruction.can_be_deduplicated(dfg, deduplicate_with_predicate) +} + impl ResultCache { /// Records that an `Instruction` in block `block` produced the result values `results`. fn cache(&mut self, block: BasicBlockId, results: Vec) { From 85fbebd17558603bdc4f2484af2e85a4a22a2ea9 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 26 Nov 2024 19:37:00 +0000 Subject: [PATCH 09/18] Move Dom up --- .../src/ssa/opt/constant_folding.rs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index ae97f39ac9..8c700e123c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -147,6 +147,7 @@ impl Function { brillig_info: Option, ) { let mut context = Context::new(use_constraint_info, brillig_info); + let mut dom = DominatorTree::with_function(self); context.block_queue.push_back(self.entry_block()); while let Some(block) = context.block_queue.pop_front() { @@ -155,7 +156,7 @@ impl Function { } context.visited_blocks.insert(block); - context.fold_constants_in_block(self, block); + context.fold_constants_in_block(&mut self.dfg, &mut dom, block); } } } @@ -264,24 +265,26 @@ impl<'brillig> Context<'brillig> { } } - fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) { - let instructions = function.dfg[block].take_instructions(); - - let mut side_effects_enabled_var = - function.dfg.make_constant(FieldElement::one(), Type::bool()); + fn fold_constants_in_block( + &mut self, + dfg: &mut DataFlowGraph, + dom: &mut DominatorTree, + block: BasicBlockId, + ) { + let instructions = dfg[block].take_instructions(); - let mut dom = DominatorTree::with_function(function); + let mut side_effects_enabled_var = dfg.make_constant(FieldElement::one(), Type::bool()); for instruction_id in instructions { self.fold_constants_into_instruction( - &mut function.dfg, - &mut dom, + dfg, + dom, block, instruction_id, &mut side_effects_enabled_var, ); } - self.block_queue.extend(function.dfg[block].successors()); + self.block_queue.extend(dfg[block].successors()); } fn fold_constants_into_instruction( From e17a570e0384a5f6f4bfc246b3b4c896c111c3ba Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 08:11:19 +0000 Subject: [PATCH 10/18] Use BTreeMap to store block IDs --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index a21e93b177..b53cbab858 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -196,7 +196,9 @@ pub(crate) struct BrilligInfo<'a> { /// For more information see [`ConstraintSimplificationCache`]. #[derive(Default)] struct SimplificationCache { - simplifications: HashMap, + /// Simplified expressions where we found them. + /// Using a `BTreeMap` for deterministic enumeration. + simplifications: BTreeMap, } impl SimplificationCache { @@ -221,7 +223,9 @@ impl SimplificationCache { return Some(*value); } // Check if there is a dominating block we can take a simplification from. - for (constraining_block, value) in self.simplifications.iter() { + // Going backwards so that we find a constraint closest to what we have already processed + // (assuming block IDs of blocks further down in the SSA are larger). + for (constraining_block, value) in self.simplifications.iter().rev() { if dom.dominates(*constraining_block, block) { return Some(*value); } From 069f260f70bb01635ac4eceaffea8ce8c9679901 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 10:23:30 +0000 Subject: [PATCH 11/18] Walk up the dominator tree to find the first simplification --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 20 +++++++++++++++++++ .../src/ssa/opt/constant_folding.rs | 14 +++---------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index c1a7f14e0d..085789fb67 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -119,6 +119,26 @@ impl DominatorTree { } } + /// Walk up the dominator tree until we find one that to which `f` returns `Some` value. + /// Otherwise return `None` when we reach the top. + /// + /// Similar to `Iterator::filter_map` but only returns the first hit. + pub(crate) fn find_map_dominator( + &self, + mut block_id: BasicBlockId, + f: impl Fn(BasicBlockId) -> Option, + ) -> Option { + loop { + if let Some(value) = f(block_id) { + return Some(value); + } + block_id = match self.immediate_dominator(block_id) { + Some(immediate_dominator) => immediate_dominator, + None => return None, + } + } + } + /// Allocate and compute a dominator tree from a pre-computed control flow graph and /// post-order counterpart. pub(crate) fn with_cfg_and_post_order(cfg: &ControlFlowGraph, post_order: &PostOrder) -> Self { diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index b53cbab858..f46afd35cb 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -218,19 +218,11 @@ impl SimplificationCache { /// Try to find a simplification in a visible block. fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option { - // See if we have a direct simplification in this block. - if let Some(value) = self.simplifications.get(&block) { - return Some(*value); + if self.simplifications.is_empty() { + return None; } // Check if there is a dominating block we can take a simplification from. - // Going backwards so that we find a constraint closest to what we have already processed - // (assuming block IDs of blocks further down in the SSA are larger). - for (constraining_block, value) in self.simplifications.iter().rev() { - if dom.dominates(*constraining_block, block) { - return Some(*value); - } - } - None + dom.find_map_dominator(block, |b| self.simplifications.get(&b).cloned()) } } From 3db0696606ebb294863b8db84ddcab0323a71695 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 11:35:32 +0000 Subject: [PATCH 12/18] Do not find anything for unreachable nodes --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index 085789fb67..97d402c5ae 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -128,6 +128,9 @@ impl DominatorTree { mut block_id: BasicBlockId, f: impl Fn(BasicBlockId) -> Option, ) -> Option { + if !self.is_reachable(block_id) { + return None; + } loop { if let Some(value) = f(block_id) { return Some(value); @@ -468,4 +471,22 @@ mod tests { assert!(dt.dominates(block2_id, block1_id)); assert!(dt.dominates(block2_id, block2_id)); } + + #[test] + fn test_find_map_dominator() { + let (dt, b0, b1, b2, _b3) = unreachable_node_setup(); + + assert_eq!( + dt.find_map_dominator(b2, |b| if b == b0 { Some("root") } else { None }), + Some("root") + ); + assert_eq!( + dt.find_map_dominator(b1, |b| if b == b0 { Some("unreachable") } else { None }), + None + ); + assert_eq!( + dt.find_map_dominator(b1, |b| if b == b1 { Some("not part of tree") } else { None }), + None + ); + } } From 5b196d5408e72b0ba0a712ce7d1506b273607835 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 11:59:37 +0000 Subject: [PATCH 13/18] Add Instruction::has_side_effect and use it to disable hoisting --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 41 +++++++++++++++ .../src/ssa/opt/constant_folding.rs | 50 ++++++++----------- 2 files changed, 62 insertions(+), 29 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 9958aadd44..1eabcd4ff1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -327,6 +327,47 @@ impl Instruction { matches!(self.result_type(), InstructionResultType::Unknown) } + /// Indicates if the instruction has a side effect, ie. it can fail, or it interacts with memory. + /// + /// This is similar to `can_be_deduplicated`, but it doesn't depend on whether the caller takes + /// constraints into account, because it might not use it to isolate the side effects across branches. + pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool { + use Instruction::*; + + match self { + // These either have side-effects or interact with memory + EnableSideEffectsIf { .. } + | Allocate + | Load { .. } + | Store { .. } + | IncrementRc { .. } + | DecrementRc { .. } => true, + + Call { func, .. } => match dfg[*func] { + Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(), + _ => true, // Be conservative and assume other functions can have side effects. + }, + + // These can fail. + Constrain(..) | RangeCheck { .. } => true, + + // This should never be side-effectful + MakeArray { .. } => false, + + // These can have different behavior depending on the EnableSideEffectsIf context. + // Replacing them with a similar instruction potentially enables replacing an instruction + // with one that was disabled. See + // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328. + Binary(_) + | Cast(_, _) + | Not(_) + | Truncate { .. } + | IfElse { .. } + | ArrayGet { .. } + | ArraySet { .. } => true, + } + } + /// Indicates if the instruction can be safely replaced with the results of another instruction with the same inputs. /// If `deduplicate_with_predicate` is set, we assume we're deduplicating with the instruction /// and its predicate, rather than just the instruction. Setting this means instructions that diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index f46afd35cb..b49a85f598 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -307,14 +307,12 @@ impl<'brillig> Context<'brillig> { Self::replace_result_ids(dfg, &old_results, cached); return; } - CacheResult::NeedToHoistToCommonBlock(dominator, _cached) => { - if instruction_can_be_hoisted(&instruction, dfg, self.use_constraint_info) { - // Just change the block to insert in the common dominator instead. - // This will only move the current instance of the instruction right now. - // When constant folding is run a second time later on, it'll catch - // that the previous instance can be deduplicated to this instance. - block = dominator; - } + CacheResult::NeedToHoistToCommonBlock(dominator) => { + // Just change the block to insert in the common dominator instead. + // This will only move the current instance of the instruction right now. + // When constant folding is run a second time later on, it'll catch + // that the previous instance can be deduplicated to this instance. + block = dominator; } } } @@ -483,6 +481,7 @@ impl<'brillig> Context<'brillig> { } } + /// Get a cached result if it can be used in this context. fn get_cached( &self, dfg: &DataFlowGraph, @@ -496,7 +495,7 @@ impl<'brillig> Context<'brillig> { let predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); let predicate = predicate.then_some(side_effects_enabled_var); - results_for_instruction.get(&predicate)?.get(block, dom) + results_for_instruction.get(&predicate)?.get(block, dom, instruction.has_side_effects(dfg)) } /// Checks if the given instruction is a call to a brillig function with all constant arguments. @@ -660,20 +659,6 @@ impl<'brillig> Context<'brillig> { } } -fn instruction_can_be_hoisted( - instruction: &Instruction, - dfg: &mut DataFlowGraph, - deduplicate_with_predicate: bool, -) -> bool { - // These two can never be hoisted as they have a side-effect - // (though it's fine to de-duplicate them, just not fine to hoist them) - if matches!(instruction, Instruction::Constrain(..) | Instruction::RangeCheck { .. }) { - return false; - } - - instruction.can_be_deduplicated(dfg, deduplicate_with_predicate) -} - impl ResultCache { /// Records that an `Instruction` in block `block` produced the result values `results`. fn cache(&mut self, block: BasicBlockId, results: Vec) { @@ -688,14 +673,21 @@ impl ResultCache { /// We require that the cached instruction's block dominates `block` in order to avoid /// cycles causing issues (e.g. two instructions being replaced with the results of each other /// such that neither instruction exists anymore.) - fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option { - self.result.as_ref().map(|(origin_block, results)| { + fn get( + &self, + block: BasicBlockId, + dom: &mut DominatorTree, + has_side_effects: bool, + ) -> Option { + self.result.as_ref().and_then(|(origin_block, results)| { if dom.dominates(*origin_block, block) { - CacheResult::Cached(results) - } else { + Some(CacheResult::Cached(results)) + } else if !has_side_effects { // Insert a copy of this instruction in the common dominator let dominator = dom.common_dominator(*origin_block, block); - CacheResult::NeedToHoistToCommonBlock(dominator, results) + Some(CacheResult::NeedToHoistToCommonBlock(dominator)) + } else { + None } }) } @@ -703,7 +695,7 @@ impl ResultCache { enum CacheResult<'a> { Cached(&'a [ValueId]), - NeedToHoistToCommonBlock(BasicBlockId, &'a [ValueId]), + NeedToHoistToCommonBlock(BasicBlockId), } /// Result of trying to evaluate an instruction (any instruction) in this pass. From 911aedbcea424142cf4dbdcf321113ec06f29a72 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 12:07:26 +0000 Subject: [PATCH 14/18] Update compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/ssa/opt/constant_folding.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index b49a85f598..0371666f5e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -204,16 +204,16 @@ struct SimplificationCache { impl SimplificationCache { /// Called with a newly encountered simplification. fn add(&mut self, dfg: &DataFlowGraph, simple: ValueId, block: BasicBlockId) { - let existing = self.simplifications.entry(block).or_insert(simple); - // Keep the simpler expression in this block. - if *existing != simple { - match simplify(dfg, *existing, simple) { - Some((complex, simple)) if *existing == complex => { - *existing = simple; - } - _ => {} - } - } + self.simplifications + .entry(block) + .and_modify(|existing| { + // `SimplificationCache` may already hold a simplification in this block + // so we check whether `simple` is a better simplification than the current one. + if let Some((_, simpler)) = simplify(dfg, *existing, simple) { + *existing = simpler; + }; + }) + .or_insert(simple); } /// Try to find a simplification in a visible block. From c07d760886d5f5201b6acb4c01c92e72d9a0a07f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 12:10:30 +0000 Subject: [PATCH 15/18] Remove Predicate --- .../src/ssa/opt/constant_folding.rs | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 0371666f5e..2fbd7e6643 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -161,9 +161,6 @@ impl Function { } } -/// `ValueId` in an `EnableConstraintsIf` instruction. -type Predicate = ValueId; - struct Context<'a> { use_constraint_info: bool, brillig_info: Option>, @@ -204,7 +201,7 @@ struct SimplificationCache { impl SimplificationCache { /// Called with a newly encountered simplification. fn add(&mut self, dfg: &DataFlowGraph, simple: ValueId, block: BasicBlockId) { - self.simplifications + self.simplifications .entry(block) .and_modify(|existing| { // `SimplificationCache` may already hold a simplification in this block @@ -226,20 +223,21 @@ impl SimplificationCache { } } -/// HashMap from Instruction to a simplified expression that it can be replaced with based on -/// constraints that testify to their equivalence, stored together with the set of blocks at which -/// this constraint has been observed. Only blocks dominated by one in the cache should have -/// access to this information, otherwise we create a sort of time paradox where we replace -/// an instruction with a constant we believe _should_ be true about it, without ever actually -/// producing and asserting the value. -type ConstraintSimplificationCache = HashMap>; +/// HashMap from (side_effects_enabled_var, Instruction) to a simplified expression that it can +/// be replaced with based on constraints that testify to their equivalence, stored together +/// with the set of blocks at which this constraint has been observed. +/// +/// Only blocks dominated by one in the cache should have access to this information, otherwise +/// we create a sort of time paradox where we replace an instruction with a constant we believe +/// it _should_ equal to, without ever actually producing and asserting the value. +type ConstraintSimplificationCache = HashMap>; /// HashMap from (Instruction, side_effects_enabled_var) to the results of the instruction. /// Stored as a two-level map to avoid cloning Instructions during the `.get` call. /// /// In addition to each result, the original BasicBlockId is stored as well. This allows us /// to deduplicate instructions across blocks as long as the new block dominates the original. -type InstructionResultCache = HashMap, ResultCache>>; +type InstructionResultCache = HashMap, ResultCache>>; /// Records the results of all duplicate [`Instruction`]s along with the blocks in which they sit. /// From 41502f607cd1247c9f38b4038f46e99028ac8ed3 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 12:16:40 +0000 Subject: [PATCH 16/18] No need for mutable any more. No need to check empty either --- .../src/ssa/opt/constant_folding.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 2fbd7e6643..22eab51254 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -187,15 +187,17 @@ pub(crate) struct BrilligInfo<'a> { brillig_functions: &'a BTreeMap, } -/// Records a simplified equivalent of an [`Instruction`]s along with the blocks in which the -/// constraint that advised the simplification has been encountered. +/// Records a simplified equivalents of an [`Instruction`] in the blocks +/// where the constraint that advised the simplification has been encountered. /// /// For more information see [`ConstraintSimplificationCache`]. #[derive(Default)] struct SimplificationCache { /// Simplified expressions where we found them. - /// Using a `BTreeMap` for deterministic enumeration. - simplifications: BTreeMap, + /// + /// It will always have at least one value because `add` is called + /// after the default is constructed. + simplifications: HashMap, } impl SimplificationCache { @@ -214,11 +216,8 @@ impl SimplificationCache { } /// Try to find a simplification in a visible block. - fn get(&self, block: BasicBlockId, dom: &mut DominatorTree) -> Option { - if self.simplifications.is_empty() { - return None; - } - // Check if there is a dominating block we can take a simplification from. + fn get(&self, block: BasicBlockId, dom: &DominatorTree) -> Option { + // Deterministically walk up the dominator chain until we encounter a block that contains a simplification. dom.find_map_dominator(block, |b| self.simplifications.get(&b).cloned()) } } From 05cf0a934e908996f53be27f8aef3692468b7720 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 12:20:51 +0000 Subject: [PATCH 17/18] Fix comment --- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index 97d402c5ae..504eecf480 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -119,7 +119,7 @@ impl DominatorTree { } } - /// Walk up the dominator tree until we find one that to which `f` returns `Some` value. + /// Walk up the dominator tree until we find a block for which `f` returns `Some` value. /// Otherwise return `None` when we reach the top. /// /// Similar to `Iterator::filter_map` but only returns the first hit. From 1e835b5c54059ec18b726c7f7ca761dbdfb91b58 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 28 Nov 2024 00:29:21 +0100 Subject: [PATCH 18/18] Use requires_acir_gen_predicate in has_side_effects --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index e02b72e058..6737b335b7 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -390,16 +390,13 @@ impl Instruction { MakeArray { .. } => false, // These can have different behavior depending on the EnableSideEffectsIf context. - // Replacing them with a similar instruction potentially enables replacing an instruction - // with one that was disabled. See - // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328. Binary(_) | Cast(_, _) | Not(_) | Truncate { .. } | IfElse { .. } | ArrayGet { .. } - | ArraySet { .. } => true, + | ArraySet { .. } => self.requires_acir_gen_predicate(dfg), } }