From a4f3b7887306212c91ba306f0c3239552e72ed90 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 11 Sep 2023 22:31:08 +0000 Subject: [PATCH 01/12] do not simplify ArrayLen for slices in SSA --- compiler/noirc_errors/src/debug_info.rs | 2 +- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 8 +++++++- compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs | 5 +++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index f62ab08dd77..946841c279b 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -33,7 +33,7 @@ impl DebugInfo { let old_locations = mem::take(&mut self.locations); for (old_opcode_location, source_locations) in old_locations { - let _ = update_map.new_locations(old_opcode_location).map(|new_opcode_location| { + update_map.new_locations(old_opcode_location).for_each(|new_opcode_location| { self.locations.insert(new_opcode_location, source_locations.clone()); }); } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 81c79eda064..e91c9a6079f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1166,7 +1166,13 @@ impl Context { } Intrinsic::ArrayLen => { let len = match self.convert_value(arguments[0], dfg) { - AcirValue::Var(_, _) => unreachable!("Non-array passed to array.len() method"), + AcirValue::Var(var, _) => { + if matches!(dfg.type_of_value(arguments[1]), Type::Slice(_)) { + return Ok(vec![AcirValue::Var(var, AcirType::field())]); + } else { + unreachable!("Non-array passed to array.len() method"); + } + } AcirValue::Array(values) => (values.len() as u128).into(), AcirValue::DynamicArray(array) => (array.len as u128).into(), }; diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 42d0aa0a4e4..1883d866f80 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -79,11 +79,12 @@ pub(super) fn simplify_call( } } Intrinsic::ArrayLen => { + // We never simplify slice lengths here as we reuse the same value to represent a slice length + // throughout the SSA. This can lead to bugs due to that some other SSA optimization rely + // on SSA having unique values, such as replacing previously constrained values during constant folding. if let Some(length) = dfg.try_get_array_length(arguments[0]) { let length = FieldElement::from(length as u128); SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field())) - } else if matches!(dfg.type_of_value(arguments[1]), Type::Slice(_)) { - SimplifyResult::SimplifiedTo(arguments[0]) } else { SimplifyResult::None } From 0fc0075abcf23f8a53b54cf6ebb109f21427fc25 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 12 Sep 2023 15:03:15 +0000 Subject: [PATCH 02/12] do not replace constrained values with side effects enabled --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 8 +- .../src/ssa/ir/instruction/call.rs | 2 + .../src/ssa/opt/constant_folding.rs | 91 +++++++++++----- .../execution_success/slices/src/main.nr | 102 +++++++++--------- 4 files changed, 121 insertions(+), 82 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index e91c9a6079f..81c79eda064 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1166,13 +1166,7 @@ impl Context { } Intrinsic::ArrayLen => { let len = match self.convert_value(arguments[0], dfg) { - AcirValue::Var(var, _) => { - if matches!(dfg.type_of_value(arguments[1]), Type::Slice(_)) { - return Ok(vec![AcirValue::Var(var, AcirType::field())]); - } else { - unreachable!("Non-array passed to array.len() method"); - } - } + AcirValue::Var(_, _) => unreachable!("Non-array passed to array.len() method"), AcirValue::Array(values) => (values.len() as u128).into(), AcirValue::DynamicArray(array) => (array.len as u128).into(), }; diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 1883d866f80..742fa624c7b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -85,6 +85,8 @@ pub(super) fn simplify_call( if let Some(length) = dfg.try_get_array_length(arguments[0]) { let length = FieldElement::from(length as u128); SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field())) + } else if matches!(dfg.type_of_value(arguments[1]), Type::Slice(_)) { + SimplifyResult::SimplifiedTo(arguments[0]) } else { SimplifyResult::None } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 9eda52e0475..0fe2fa5da5b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -78,6 +78,7 @@ impl Context { // Cache of instructions without any side-effects along with their outputs. let mut cached_instruction_results: HashMap> = HashMap::default(); let mut constrained_values: HashMap = HashMap::default(); + let mut side_effects_enabled: bool = false; for instruction_id in instructions { Self::fold_constants_into_instruction( @@ -86,6 +87,7 @@ impl Context { instruction_id, &mut cached_instruction_results, &mut constrained_values, + &mut side_effects_enabled, ); } self.block_queue.extend(function.dfg[block].successors()); @@ -97,8 +99,10 @@ impl Context { id: InstructionId, instruction_result_cache: &mut HashMap>, constrained_values: &mut HashMap, + side_effects_enabled: &mut bool, ) { - let instruction = Self::resolve_instruction(id, dfg, constrained_values); + let instruction = + Self::resolve_instruction(id, dfg, constrained_values, side_effects_enabled); 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. @@ -118,6 +122,7 @@ impl Context { dfg, instruction_result_cache, constrained_values, + side_effects_enabled, ); } @@ -126,6 +131,7 @@ impl Context { instruction_id: InstructionId, dfg: &DataFlowGraph, constrained_values: &mut HashMap, + side_effects_enabled: &bool, ) -> Instruction { let instruction = dfg[instruction_id].clone(); @@ -134,20 +140,34 @@ impl Context { // // This allows us to reach a stable final `ValueId` for each instruction input as we add more // constraints to the cache. + // + // We also must check whether side effects are currently enabled. If they are enabled we do not want + // to replace constrained values as they could be reliant upon runtime values. The runtime values + // would then be replaced later in the SSA, possibly leading to a mismatched constant constraint failure + // when we should have a runtime constraint failure earlier in the SSA. fn resolve_cache( dfg: &DataFlowGraph, cache: &HashMap, value_id: ValueId, + side_effects_enabled: &bool, ) -> ValueId { let resolved_id = dfg.resolve(value_id); - match cache.get(&resolved_id) { - Some(cached_value) => resolve_cache(dfg, cache, *cached_value), - None => resolved_id, + if *side_effects_enabled { + return resolved_id; } + let resolved_id = match cache.get(&resolved_id) { + Some(cached_value) => { + resolve_cache(dfg, cache, *cached_value, side_effects_enabled) + } + None => resolved_id, + }; + resolved_id } // Resolve any inputs to ensure that we're comparing like-for-like instructions. - instruction.map_values(|value_id| resolve_cache(dfg, constrained_values, value_id)) + instruction.map_values(|value_id| { + resolve_cache(dfg, constrained_values, value_id, side_effects_enabled) + }) } /// Pushes a new [`Instruction`] into the [`DataFlowGraph`] which applies any optimizations @@ -186,31 +206,48 @@ impl Context { dfg: &DataFlowGraph, instruction_result_cache: &mut HashMap>, constraint_cache: &mut HashMap, + side_effects_enabled: &mut bool, ) { - // If the instruction was a constraint, then create a link between the two `ValueId`s - // 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 { .. }, _) => { - constraint_cache.insert(rhs, lhs); + match instruction { + Instruction::EnableSideEffects { condition } => { + if let Value::NumericConstant { constant, .. } = &dfg[condition] { + if constant.is_one() { + *side_effects_enabled = true; + } else { + *side_effects_enabled = false; + } + } else { + // If the `enable_side_effects` condition is not a constant we cannot know for sure whether + // side effects are not enabled. Thus, we mark the side effects as being enabled to make sure + // that we do not mistakenly replace constrained values dependent upon runtime side effects. + *side_effects_enabled = true; } - (_, Value::NumericConstant { .. }) => { - constraint_cache.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 { .. }) => { - constraint_cache.insert(rhs, lhs); - } - (Value::Instruction { .. }, Value::Param { .. }) => { - constraint_cache.insert(lhs, rhs); + } + Instruction::Constrain(lhs, rhs, _) => { + match (&dfg[lhs], &dfg[rhs]) { + // Ignore trivial constraints + (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => (), + + // Prefer replacing with constants where possible. + (Value::NumericConstant { .. }, _) => { + constraint_cache.insert(rhs, lhs); + } + (_, Value::NumericConstant { .. }) => { + constraint_cache.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 { .. }) => { + constraint_cache.insert(rhs, lhs); + } + (Value::Instruction { .. }, Value::Param { .. }) => { + constraint_cache.insert(lhs, rhs); + } + (_, _) => (), } - (_, _) => (), + } + _ => { + // The remaining instructions are not cached, so do nothing } } diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 8fbe14bfea3..dce6a6d6967 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -1,56 +1,62 @@ use dep::std::slice; use dep::std; -fn main(x : Field, y : pub Field) { - let mut slice = [0; 2]; - assert(slice[0] == 0); - assert(slice[0] != 1); - slice[0] = x; - assert(slice[0] == x); - - let slice_plus_10 = slice.push_back(y); - assert(slice_plus_10[2] == 10); - assert(slice_plus_10[2] != 8); - assert(slice_plus_10.len() == 3); - - let mut new_slice = []; - for i in 0..5 { - new_slice = new_slice.push_back(i); - } - assert(new_slice.len() == 5); - - new_slice = new_slice.push_front(20); - assert(new_slice[0] == 20); - assert(new_slice.len() == 6); - - let (popped_slice, last_elem) = new_slice.pop_back(); - assert(last_elem == 4); - assert(popped_slice.len() == 5); - - let (first_elem, rest_of_slice) = popped_slice.pop_front(); - assert(first_elem == 20); - assert(rest_of_slice.len() == 4); - - new_slice = rest_of_slice.insert(2, 100); - assert(new_slice[2] == 100); - assert(new_slice[4] == 3); - assert(new_slice.len() == 5); - - let (remove_slice, removed_elem) = new_slice.remove(3); - assert(removed_elem == 2); - assert(remove_slice[3] == 3); - assert(remove_slice.len() == 4); - - let append = [1, 2].append([3, 4, 5]); - assert(append.len() == 5); - assert(append[0] == 1); - assert(append[4] == 5); - - regression_2083(); - // The parameters to this function must come from witness values (inputs to main) - regression_merge_slices(x, y); +fn main(x: Field, y: Field) { + let slice = merge_slices_mutate(x, y); + assert(slice.len() == 3); + assert(slice[3] == 5); } +// fn main(x : Field, y : pub Field) { +// let mut slice = [0; 2]; +// assert(slice[0] == 0); +// assert(slice[0] != 1); +// slice[0] = x; +// assert(slice[0] == x); + +// let slice_plus_10 = slice.push_back(y); +// assert(slice_plus_10[2] == 10); +// assert(slice_plus_10[2] != 8); +// assert(slice_plus_10.len() == 3); + +// let mut new_slice = []; +// for i in 0..5 { +// new_slice = new_slice.push_back(i); +// } +// assert(new_slice.len() == 5); + +// new_slice = new_slice.push_front(20); +// assert(new_slice[0] == 20); +// assert(new_slice.len() == 6); + +// let (popped_slice, last_elem) = new_slice.pop_back(); +// assert(last_elem == 4); +// assert(popped_slice.len() == 5); + +// let (first_elem, rest_of_slice) = popped_slice.pop_front(); +// assert(first_elem == 20); +// assert(rest_of_slice.len() == 4); + +// new_slice = rest_of_slice.insert(2, 100); +// assert(new_slice[2] == 100); +// assert(new_slice[4] == 3); +// assert(new_slice.len() == 5); + +// let (remove_slice, removed_elem) = new_slice.remove(3); +// assert(removed_elem == 2); +// assert(remove_slice[3] == 3); +// assert(remove_slice.len() == 4); + +// let append = [1, 2].append([3, 4, 5]); +// assert(append.len() == 5); +// assert(append[0] == 1); +// assert(append[4] == 5); + +// regression_2083(); +// // The parameters to this function must come from witness values (inputs to main) +// regression_merge_slices(x, y); +// } + // Ensure that slices of struct/tuple values work. fn regression_2083() { let y = [(1, 2)]; From 6566b2798c6dcfa99389350f9e51d7e4c9f134c9 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 12 Sep 2023 15:49:14 +0000 Subject: [PATCH 03/12] add test to constant_folding and fix up slices integration test --- .../src/ssa/opt/constant_folding.rs | 76 +++++++++++-- .../execution_success/slices/src/main.nr | 102 +++++++++--------- 2 files changed, 114 insertions(+), 64 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 0fe2fa5da5b..0180519495c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -445,16 +445,7 @@ mod test { // Compiling main let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); - let v0 = builder.add_parameter(Type::field()); - - let field_10 = builder.field_constant(10u128); - builder.insert_constrain(v0, field_10, None); - - let field_1 = builder.field_constant(1u128); - let v1 = builder.insert_binary(v0, BinaryOp::Add, field_1); - - let field_11 = builder.field_constant(11u128); - builder.insert_constrain(v1, field_11, None); + constrained_value_replacement_setup(&mut builder, false); let mut ssa = builder.finish(); let main = ssa.main_mut(); @@ -479,4 +470,69 @@ mod test { &Instruction::Constrain(ValueId::test_new(0), ValueId::test_new(1), None) ); } + + #[test] + fn constrained_value_replacement_with_side_effects() { + // fn main f0 { + // b0(v0: Field): + // enable_side_effects u1 1 + // constrain v0 == Field 10 + // v1 = add v0, Field 1 + // constrain v1 == Field 11 + // } + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); + constrained_value_replacement_setup(&mut builder, true); + + let mut ssa = builder.finish(); + let main = ssa.main_mut(); + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 4); + + // Expected output: + // + // fn main f0 { + // b0(v0: Field): + // enable_side_effects u1 1 + // constrain v0 == Field 10 + // v6 = add v0, Field 1 + // constrain v6 == Field 11 + // } + let ssa = ssa.fold_constants(); + println!("ssa: {ssa}"); + let main = ssa.main(); + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 4); + + assert_eq!( + &main.dfg[instructions[0]], + &Instruction::EnableSideEffects { condition: ValueId::test_new(1) } + ); + + assert_eq!( + &main.dfg[instructions[3]], + &Instruction::Constrain(ValueId::test_new(6), ValueId::test_new(5), None) + ); + } + + fn constrained_value_replacement_setup(builder: &mut FunctionBuilder, with_side_effects: bool) { + let v0 = builder.add_parameter(Type::field()); + + if with_side_effects { + let field_one = builder.numeric_constant(1u128, Type::unsigned(1)); + let enable_side_effects = Instruction::EnableSideEffects { condition: field_one }; + builder.insert_instruction(enable_side_effects, None); + } + + let field_10 = builder.field_constant(10u128); + builder.insert_constrain(v0, field_10, None); + + let field_1 = builder.field_constant(1u128); + let v1 = builder.insert_binary(v0, BinaryOp::Add, field_1); + + let field_11 = builder.field_constant(11u128); + builder.insert_constrain(v1, field_11, None); + } } diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index dce6a6d6967..8fbe14bfea3 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -1,62 +1,56 @@ use dep::std::slice; use dep::std; -fn main(x: Field, y: Field) { - let slice = merge_slices_mutate(x, y); - assert(slice.len() == 3); - assert(slice[3] == 5); +fn main(x : Field, y : pub Field) { + let mut slice = [0; 2]; + assert(slice[0] == 0); + assert(slice[0] != 1); + slice[0] = x; + assert(slice[0] == x); + + let slice_plus_10 = slice.push_back(y); + assert(slice_plus_10[2] == 10); + assert(slice_plus_10[2] != 8); + assert(slice_plus_10.len() == 3); + + let mut new_slice = []; + for i in 0..5 { + new_slice = new_slice.push_back(i); + } + assert(new_slice.len() == 5); + + new_slice = new_slice.push_front(20); + assert(new_slice[0] == 20); + assert(new_slice.len() == 6); + + let (popped_slice, last_elem) = new_slice.pop_back(); + assert(last_elem == 4); + assert(popped_slice.len() == 5); + + let (first_elem, rest_of_slice) = popped_slice.pop_front(); + assert(first_elem == 20); + assert(rest_of_slice.len() == 4); + + new_slice = rest_of_slice.insert(2, 100); + assert(new_slice[2] == 100); + assert(new_slice[4] == 3); + assert(new_slice.len() == 5); + + let (remove_slice, removed_elem) = new_slice.remove(3); + assert(removed_elem == 2); + assert(remove_slice[3] == 3); + assert(remove_slice.len() == 4); + + let append = [1, 2].append([3, 4, 5]); + assert(append.len() == 5); + assert(append[0] == 1); + assert(append[4] == 5); + + regression_2083(); + // The parameters to this function must come from witness values (inputs to main) + regression_merge_slices(x, y); } -// fn main(x : Field, y : pub Field) { -// let mut slice = [0; 2]; -// assert(slice[0] == 0); -// assert(slice[0] != 1); -// slice[0] = x; -// assert(slice[0] == x); - -// let slice_plus_10 = slice.push_back(y); -// assert(slice_plus_10[2] == 10); -// assert(slice_plus_10[2] != 8); -// assert(slice_plus_10.len() == 3); - -// let mut new_slice = []; -// for i in 0..5 { -// new_slice = new_slice.push_back(i); -// } -// assert(new_slice.len() == 5); - -// new_slice = new_slice.push_front(20); -// assert(new_slice[0] == 20); -// assert(new_slice.len() == 6); - -// let (popped_slice, last_elem) = new_slice.pop_back(); -// assert(last_elem == 4); -// assert(popped_slice.len() == 5); - -// let (first_elem, rest_of_slice) = popped_slice.pop_front(); -// assert(first_elem == 20); -// assert(rest_of_slice.len() == 4); - -// new_slice = rest_of_slice.insert(2, 100); -// assert(new_slice[2] == 100); -// assert(new_slice[4] == 3); -// assert(new_slice.len() == 5); - -// let (remove_slice, removed_elem) = new_slice.remove(3); -// assert(removed_elem == 2); -// assert(remove_slice[3] == 3); -// assert(remove_slice.len() == 4); - -// let append = [1, 2].append([3, 4, 5]); -// assert(append.len() == 5); -// assert(append[0] == 1); -// assert(append[4] == 5); - -// regression_2083(); -// // The parameters to this function must come from witness values (inputs to main) -// regression_merge_slices(x, y); -// } - // Ensure that slices of struct/tuple values work. fn regression_2083() { let y = [(1, 2)]; From d871f8b2680ab563ee8d35448e1b345eeb16385e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 12 Sep 2023 15:49:57 +0000 Subject: [PATCH 04/12] remove old comment --- compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 742fa624c7b..42d0aa0a4e4 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -79,9 +79,6 @@ pub(super) fn simplify_call( } } Intrinsic::ArrayLen => { - // We never simplify slice lengths here as we reuse the same value to represent a slice length - // throughout the SSA. This can lead to bugs due to that some other SSA optimization rely - // on SSA having unique values, such as replacing previously constrained values during constant folding. if let Some(length) = dfg.try_get_array_length(arguments[0]) { let length = FieldElement::from(length as u128); SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field())) From 70e87449f981a51761513431b4636001aaef9cb1 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 12 Sep 2023 15:51:29 +0000 Subject: [PATCH 05/12] return match directly --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 0180519495c..e4e33aedf1a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -155,13 +155,12 @@ impl Context { if *side_effects_enabled { return resolved_id; } - let resolved_id = match cache.get(&resolved_id) { + match cache.get(&resolved_id) { Some(cached_value) => { resolve_cache(dfg, cache, *cached_value, side_effects_enabled) } None => resolved_id, - }; - resolved_id + } } // Resolve any inputs to ensure that we're comparing like-for-like instructions. From 0b0395a9ad9ca7acf19de8feb544b1ac42fab798 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 12 Sep 2023 17:00:17 +0100 Subject: [PATCH 06/12] Update compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index e4e33aedf1a..6d4cedbdaee 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -131,7 +131,7 @@ impl Context { instruction_id: InstructionId, dfg: &DataFlowGraph, constrained_values: &mut HashMap, - side_effects_enabled: &bool, + side_effects_enabled: bool, ) -> Instruction { let instruction = dfg[instruction_id].clone(); From 779d20e8c5d9f89732481a64c55b1c9bc8e933f0 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 12 Sep 2023 16:01:10 +0000 Subject: [PATCH 07/12] deref bool earlier --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 6d4cedbdaee..9c337a7f47a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -102,7 +102,7 @@ impl Context { side_effects_enabled: &mut bool, ) { let instruction = - Self::resolve_instruction(id, dfg, constrained_values, side_effects_enabled); + Self::resolve_instruction(id, dfg, constrained_values, *side_effects_enabled); 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. @@ -149,10 +149,10 @@ impl Context { dfg: &DataFlowGraph, cache: &HashMap, value_id: ValueId, - side_effects_enabled: &bool, + side_effects_enabled: bool, ) -> ValueId { let resolved_id = dfg.resolve(value_id); - if *side_effects_enabled { + if side_effects_enabled { return resolved_id; } match cache.get(&resolved_id) { From 682c5bb58b97fc8e4ec8681cf937e6962bd04f54 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Fri, 15 Sep 2023 18:23:06 +0100 Subject: [PATCH 08/12] Update compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 9c337a7f47a..07cd2178221 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -500,7 +500,6 @@ mod test { // constrain v6 == Field 11 // } let ssa = ssa.fold_constants(); - println!("ssa: {ssa}"); let main = ssa.main(); let instructions = main.dfg[main.entry_block()].instructions(); assert_eq!(instructions.len(), 4); From 097a275b33d807b011ef2005f6e939707fcc6995 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 21 Sep 2023 15:37:14 +0100 Subject: [PATCH 09/12] Update compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs Co-authored-by: jfecher --- compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 6c61ba570b7..2f49a59e51a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -78,7 +78,7 @@ impl Context { // Cache of instructions without any side-effects along with their outputs. let mut cached_instruction_results: HashMap> = HashMap::default(); let mut constrained_values: HashMap = HashMap::default(); - let mut side_effects_enabled: bool = false; + let mut side_effects_enabled = false; for instruction_id in instructions { Self::fold_constants_into_instruction( From deec30c8442b3bc7738cfa4066b792dba09c2a18 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 21 Sep 2023 17:33:21 +0000 Subject: [PATCH 10/12] remove constrain_cache in constant_folding --- .../src/ssa/opt/constant_folding.rs | 210 +----------------- .../execution_success/slices/src/main.nr | 6 +- 2 files changed, 8 insertions(+), 208 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 2f49a59e51a..a96a8d70e04 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -30,7 +30,7 @@ use crate::ssa::{ dfg::{DataFlowGraph, InsertInstructionResult}, function::Function, instruction::{Instruction, InstructionId}, - value::{Value, ValueId}, + value::ValueId, }, ssa_gen::Ssa, }; @@ -77,8 +77,6 @@ impl Context { // Cache of instructions without any side-effects along with their outputs. let mut cached_instruction_results: HashMap> = HashMap::default(); - let mut constrained_values: HashMap = HashMap::default(); - let mut side_effects_enabled = false; for instruction_id in instructions { Self::fold_constants_into_instruction( @@ -86,8 +84,6 @@ impl Context { block, instruction_id, &mut cached_instruction_results, - &mut constrained_values, - &mut side_effects_enabled, ); } self.block_queue.extend(function.dfg[block].successors()); @@ -98,11 +94,8 @@ impl Context { block: BasicBlockId, id: InstructionId, instruction_result_cache: &mut HashMap>, - constrained_values: &mut HashMap, - side_effects_enabled: &mut bool, ) { - let instruction = - Self::resolve_instruction(id, dfg, constrained_values, *side_effects_enabled); + let instruction = Self::resolve_instruction(id, dfg); 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. @@ -116,57 +109,15 @@ impl Context { Self::replace_result_ids(dfg, &old_results, &new_results); - Self::cache_instruction( - instruction, - new_results, - dfg, - instruction_result_cache, - constrained_values, - side_effects_enabled, - ); + Self::cache_instruction(instruction, new_results, dfg, instruction_result_cache); } /// Fetches an [`Instruction`] by its [`InstructionId`] and fully resolves its inputs. - fn resolve_instruction( - instruction_id: InstructionId, - dfg: &DataFlowGraph, - constrained_values: &HashMap, - side_effects_enabled: bool, - ) -> Instruction { + fn resolve_instruction(instruction_id: InstructionId, dfg: &DataFlowGraph) -> Instruction { let instruction = dfg[instruction_id].clone(); - // Alternate between resolving `value_id` in the `dfg` and checking to see if the resolved value - // has been constrained to be equal to some simpler value in the current block. - // - // This allows us to reach a stable final `ValueId` for each instruction input as we add more - // constraints to the cache. - // - // We also must check whether side effects are currently enabled. If they are enabled we do not want - // to replace constrained values as they could be reliant upon runtime values. The runtime values - // would then be replaced later in the SSA, possibly leading to a mismatched constant constraint failure - // when we should have a runtime constraint failure earlier in the SSA. - fn resolve_cache( - dfg: &DataFlowGraph, - cache: &HashMap, - value_id: ValueId, - side_effects_enabled: bool, - ) -> ValueId { - let resolved_id = dfg.resolve(value_id); - if side_effects_enabled { - return resolved_id; - } - match cache.get(&resolved_id) { - Some(cached_value) => { - resolve_cache(dfg, cache, *cached_value, side_effects_enabled) - } - None => resolved_id, - } - } - // Resolve any inputs to ensure that we're comparing like-for-like instructions. - instruction.map_values(|value_id| { - resolve_cache(dfg, constrained_values, value_id, side_effects_enabled) - }) + instruction.map_values(|value_id| dfg.resolve(value_id)) } /// Pushes a new [`Instruction`] into the [`DataFlowGraph`] which applies any optimizations @@ -204,52 +155,7 @@ impl Context { instruction_results: Vec, dfg: &DataFlowGraph, instruction_result_cache: &mut HashMap>, - constraint_cache: &mut HashMap, - side_effects_enabled: &mut bool, ) { - match instruction { - Instruction::EnableSideEffects { condition } => { - if let Value::NumericConstant { constant, .. } = &dfg[condition] { - if constant.is_one() { - *side_effects_enabled = true; - } else { - *side_effects_enabled = false; - } - } else { - // If the `enable_side_effects` condition is not a constant we cannot know for sure whether - // side effects are not enabled. Thus, we mark the side effects as being enabled to make sure - // that we do not mistakenly replace constrained values dependent upon runtime side effects. - *side_effects_enabled = true; - } - } - Instruction::Constrain(lhs, rhs, _) => { - match (&dfg[lhs], &dfg[rhs]) { - // Ignore trivial constraints - (Value::NumericConstant { .. }, Value::NumericConstant { .. }) => (), - - // Prefer replacing with constants where possible. - (Value::NumericConstant { .. }, _) => { - constraint_cache.insert(rhs, lhs); - } - (_, Value::NumericConstant { .. }) => { - constraint_cache.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 { .. }) => { - constraint_cache.insert(rhs, lhs); - } - (Value::Instruction { .. }, Value::Param { .. }) => { - constraint_cache.insert(lhs, rhs); - } - (_, _) => (), - } - } - _ => { - // The remaining instructions are not cached, so do nothing - } - } - // If the instruction doesn't have side-effects, cache the results so we can reuse them if // the same instruction appears again later in the block. if instruction.is_pure(dfg) { @@ -427,110 +333,4 @@ mod test { assert_eq!(instruction, &Instruction::Cast(ValueId::test_new(0), Type::unsigned(32))); } - - #[test] - fn constrained_value_replacement() { - // fn main f0 { - // b0(v0: Field): - // constrain v0 == Field 10 - // v1 = add v0, Field 1 - // constrain v1 == Field 11 - // } - // - // After constructing this IR, we run constant folding which should replace references to `v0` - // with the constant `10`. This then allows us to optimize away the rest of the circuit. - - let main_id = Id::test_new(0); - - // Compiling main - let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); - constrained_value_replacement_setup(&mut builder, false); - - let mut ssa = builder.finish(); - let main = ssa.main_mut(); - let instructions = main.dfg[main.entry_block()].instructions(); - assert_eq!(instructions.len(), 3); - - // Expected output: - // - // fn main f0 { - // b0(v0: Field): - // constrain v0 == Field 10 - // } - let ssa = ssa.fold_constants(); - let main = ssa.main(); - let instructions = main.dfg[main.entry_block()].instructions(); - - assert_eq!(instructions.len(), 1); - let instruction = &main.dfg[instructions[0]]; - - assert_eq!( - instruction, - &Instruction::Constrain(ValueId::test_new(0), ValueId::test_new(1), None) - ); - } - - #[test] - fn constrained_value_replacement_with_side_effects() { - // fn main f0 { - // b0(v0: Field): - // enable_side_effects u1 1 - // constrain v0 == Field 10 - // v1 = add v0, Field 1 - // constrain v1 == Field 11 - // } - let main_id = Id::test_new(0); - - // Compiling main - let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); - constrained_value_replacement_setup(&mut builder, true); - - let mut ssa = builder.finish(); - let main = ssa.main_mut(); - let instructions = main.dfg[main.entry_block()].instructions(); - assert_eq!(instructions.len(), 4); - - // Expected output: - // - // fn main f0 { - // b0(v0: Field): - // enable_side_effects u1 1 - // constrain v0 == Field 10 - // v6 = add v0, Field 1 - // constrain v6 == Field 11 - // } - let ssa = ssa.fold_constants(); - let main = ssa.main(); - let instructions = main.dfg[main.entry_block()].instructions(); - assert_eq!(instructions.len(), 4); - - assert_eq!( - &main.dfg[instructions[0]], - &Instruction::EnableSideEffects { condition: ValueId::test_new(1) } - ); - - assert_eq!( - &main.dfg[instructions[3]], - &Instruction::Constrain(ValueId::test_new(6), ValueId::test_new(5), None) - ); - } - - fn constrained_value_replacement_setup(builder: &mut FunctionBuilder, with_side_effects: bool) { - let v0 = builder.add_parameter(Type::field()); - - if with_side_effects { - let field_one = builder.numeric_constant(1u128, Type::unsigned(1)); - let enable_side_effects = Instruction::EnableSideEffects { condition: field_one }; - builder.insert_instruction(enable_side_effects, None); - } - - let field_10 = builder.field_constant(10u128); - builder.insert_constrain(v0, field_10, None); - - let field_1 = builder.field_constant(1u128); - let v1 = builder.insert_binary(v0, BinaryOp::Add, field_1); - - let field_11 = builder.field_constant(11u128); - builder.insert_constrain(v1, field_11, None); - } } diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 8fbe14bfea3..1e5a7a605aa 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -92,16 +92,16 @@ fn regression_merge_slices(x: Field, y: Field) { fn merge_slices_if(x: Field, y: Field) { let slice = merge_slices_return(x, y); - assert(slice[2] == 10); assert(slice.len() == 3); + assert(slice[2] == 10); let slice = merge_slices_mutate(x, y); - assert(slice[3] == 5); assert(slice.len() == 4); + assert(slice[3] == 5); let slice = merge_slices_mutate_in_loop(x, y); - assert(slice[6] == 4); assert(slice.len() == 7); + assert(slice[6] == 4); let slice = merge_slices_mutate_two_ifs(x, y); assert(slice.len() == 6); From a47ce03ce4dd934dd3ddb48015bbaf6dd936c5d8 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 21 Sep 2023 17:45:34 +0000 Subject: [PATCH 11/12] move len checks to be first one --- tooling/nargo_cli/tests/execution_success/slices/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 1e5a7a605aa..2f90188859e 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -96,7 +96,7 @@ fn merge_slices_if(x: Field, y: Field) { assert(slice[2] == 10); let slice = merge_slices_mutate(x, y); - assert(slice.len() == 4); + assert(slice.len() == 3); assert(slice[3] == 5); let slice = merge_slices_mutate_in_loop(x, y); From 20a1b7e4e3f4e4cf40a01e3fb70b697c9b45d702 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 21 Sep 2023 17:59:09 +0000 Subject: [PATCH 12/12] correct slice len assertion --- tooling/nargo_cli/tests/execution_success/slices/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 2f90188859e..1e5a7a605aa 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -96,7 +96,7 @@ fn merge_slices_if(x: Field, y: Field) { assert(slice[2] == 10); let slice = merge_slices_mutate(x, y); - assert(slice.len() == 3); + assert(slice.len() == 4); assert(slice[3] == 5); let slice = merge_slices_mutate_in_loop(x, y);