From f7f219a3e591c7e0807227a757f5cc1b9ca18553 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 26 Jun 2024 23:13:08 +0000 Subject: [PATCH 1/2] Revert "chore: revert removal of flattening optimisation" This reverts commit fb2f6a801877419347b5af96d5c159868f990755. --- .../src/ssa/opt/flatten_cfg.rs | 27 +------------------ 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index e07f947db8d..58f70ba9192 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -375,7 +375,6 @@ impl<'f> Context<'f> { let old_condition = *condition; let then_condition = self.inserter.resolve(old_condition); - let one = FieldElement::one(); let old_stores = std::mem::take(&mut self.store_values); let old_allocations = std::mem::take(&mut self.local_allocations); let branch = ConditionalBranch { @@ -393,15 +392,6 @@ impl<'f> Context<'f> { }; self.condition_stack.push(cond_context); self.insert_current_side_effects_enabled(); - // Optimization: within the then branch we know the condition to be true, so replace - // any references of it within this branch with true. Likewise, do the same with false - // with the else branch. We must be careful not to replace the condition if it is a - // known constant, otherwise we can end up setting 1 = 0 or vice-versa. - if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { - let known_value = self.inserter.function.dfg.make_constant(one, Type::bool()); - - self.inserter.map_value(old_condition, known_value); - } vec![self.branch_ends[if_entry], *else_destination, *then_destination] } @@ -414,7 +404,6 @@ impl<'f> Context<'f> { self.insert_instruction(Instruction::Not(cond_context.condition), CallStack::new()); let else_condition = self.link_condition(else_condition); - let zero = FieldElement::zero(); // Make sure the else branch sees the previous values of each store // rather than any values created in the 'then' branch. let old_stores = std::mem::take(&mut cond_context.then_branch.store_values); @@ -429,21 +418,12 @@ impl<'f> Context<'f> { local_allocations: old_allocations, last_block: *block, }; - let old_condition = else_branch.old_condition; cond_context.then_branch.local_allocations.clear(); cond_context.else_branch = Some(else_branch); self.condition_stack.push(cond_context); self.insert_current_side_effects_enabled(); - // Optimization: within the then branch we know the condition to be true, so replace - // any references of it within this branch with true. Likewise, do the same with false - // with the else branch. We must be careful not to replace the condition if it is a - // known constant, otherwise we can end up setting 1 = 0 or vice-versa. - if self.inserter.function.dfg.get_numeric_constant(old_condition).is_none() { - let known_value = self.inserter.function.dfg.make_constant(zero, Type::bool()); - - self.inserter.map_value(old_condition, known_value); - } + assert_eq!(self.cfg.successors(*block).len(), 1); vec![self.cfg.successors(*block).next().unwrap()] } @@ -471,11 +451,6 @@ impl<'f> Context<'f> { // known to be true/false within the then/else branch respectively. self.insert_current_side_effects_enabled(); - // We must map back to `then_condition` here. Mapping `old_condition` to itself would - // lose any previous mappings. - self.inserter - .map_value(cond_context.then_branch.old_condition, cond_context.then_branch.condition); - // While there is a condition on the stack we don't compile outside the condition // until it is popped. This ensures we inline the full then and else branches // before continuing from the end of the conditional here where they can be merged properly. From 1c95d18dc55299e6bb1cf1173d13064d7b753789 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 26 Jun 2024 23:10:12 +0000 Subject: [PATCH 2/2] Revert "test: revert constant arrays optimization" This reverts commit a6ba5c6a2feb545168960c2ef02f6b2048587d0a. --- .../src/ssa/ir/function_inserter.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs index 68ece87c7c7..a063a7ff268 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -16,11 +16,12 @@ pub(crate) struct FunctionInserter<'f> { pub(crate) function: &'f mut Function, values: HashMap, + const_arrays: HashMap, ValueId>, } impl<'f> FunctionInserter<'f> { pub(crate) fn new(function: &'f mut Function) -> FunctionInserter<'f> { - Self { function, values: HashMap::default() } + Self { function, values: HashMap::default(), const_arrays: HashMap::default() } } /// Resolves a ValueId to its new, updated value. @@ -34,10 +35,17 @@ impl<'f> FunctionInserter<'f> { super::value::Value::Array { array, typ } => { let array = array.clone(); let typ = typ.clone(); - let new_array = array.iter().map(|id| self.resolve(*id)).collect(); - let new_id = self.function.dfg.make_array(new_array, typ); - self.values.insert(value, new_id); - new_id + let new_array: im::Vector = + array.iter().map(|id| self.resolve(*id)).collect(); + if self.const_arrays.get(&new_array) == Some(&value) { + value + } else { + let new_array_clone = new_array.clone(); + let new_id = self.function.dfg.make_array(new_array, typ); + self.values.insert(value, new_id); + self.const_arrays.insert(new_array_clone, new_id); + new_id + } } _ => value, },