From 3a2c75bc1acaf06af7670fbd2228cd3148378576 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 28 Nov 2024 19:22:16 +0000 Subject: [PATCH 1/5] chore: pull out sync changes --- .../noirc_evaluator/src/acir/acir_variable.rs | 17 +- .../compiler/noirc_evaluator/src/acir/mod.rs | 8 + .../src/brillig/brillig_gen/brillig_block.rs | 427 ++++++++++-------- .../check_for_underconstrained_values.rs | 4 +- .../noirc_evaluator/src/ssa/ir/dom.rs | 41 ++ .../noirc_evaluator/src/ssa/ir/instruction.rs | 119 ++++- .../src/ssa/ir/instruction/call.rs | 2 + .../src/ssa/opt/mem2reg/alias_set.rs | 4 + .../src/ssa/opt/mem2reg/block.rs | 13 + .../src/ssa/opt/remove_enable_side_effects.rs | 2 + .../src/ssa/opt/remove_if_else.rs | 2 + .../noirc_evaluator/src/ssa/opt/unrolling.rs | 10 +- .../src/ssa/ssa_gen/context.rs | 11 +- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 +- .../docs/noir/concepts/data_types/integers.md | 11 + .../loop_invariant_regression/Nargo.toml | 7 + .../loop_invariant_regression/Prover.toml | 2 + .../loop_invariant_regression/src/main.nr | 13 + .../reference_counts/Nargo.toml | 7 + .../reference_counts/Prover.toml | 2 + .../reference_counts/src/main.nr | 40 ++ .../execution_success/regression/Prover.toml | 2 + .../execution_success/regression/src/main.nr | 10 +- 23 files changed, 539 insertions(+), 220 deletions(-) create mode 100644 noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Nargo.toml create mode 100644 noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Prover.toml create mode 100644 noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr create mode 100644 noir/noir-repo/test_programs/execution_success/reference_counts/Nargo.toml create mode 100644 noir/noir-repo/test_programs/execution_success/reference_counts/Prover.toml create mode 100644 noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs index 6ba072f01a47..a42426e6c04d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs @@ -578,7 +578,7 @@ impl> AcirContext { let numeric_type = match typ { AcirType::NumericType(numeric_type) => numeric_type, AcirType::Array(_, _) => { - todo!("cannot divide arrays. This should have been caught by the frontend") + unreachable!("cannot divide arrays. This should have been caught by the frontend") } }; match numeric_type { @@ -1084,11 +1084,22 @@ impl> AcirContext { &mut self, lhs: AcirVar, rhs: AcirVar, + typ: AcirType, bit_size: u32, predicate: AcirVar, ) -> Result { - let (_, remainder) = self.euclidean_division_var(lhs, rhs, bit_size, predicate)?; - Ok(remainder) + let numeric_type = match typ { + AcirType::NumericType(numeric_type) => numeric_type, + AcirType::Array(_, _) => { + unreachable!("cannot modulo arrays. This should have been caught by the frontend") + } + }; + + let (_, remainder_var) = match numeric_type { + NumericType::Signed { bit_size } => self.signed_division_var(lhs, rhs, bit_size)?, + _ => self.euclidean_division_var(lhs, rhs, bit_size, predicate)?, + }; + Ok(remainder_var) } /// Constrains the `AcirVar` variable to be of type `NumericType`. diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs index 46d0924b322d..69679495b926 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs @@ -1968,6 +1968,7 @@ impl<'a> Context<'a> { BinaryOp::Mod => self.acir_context.modulo_var( lhs, rhs, + binary_type.clone(), bit_count, self.current_side_effects_enabled_var, ), @@ -2762,6 +2763,13 @@ impl<'a> Context<'a> { Intrinsic::FieldLessThan => { unreachable!("FieldLessThan can only be called in unconstrained") } + Intrinsic::ArrayRefCount | Intrinsic::SliceRefCount => { + let zero = self.acir_context.add_constant(FieldElement::zero()); + Ok(vec![AcirValue::Var( + zero, + AcirType::NumericType(NumericType::Unsigned { bit_size: 32 }), + )]) + } } } 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 36e1ee90e11a..1fa4985295a9 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 @@ -402,210 +402,251 @@ impl<'block> BrilligBlock<'block> { let result_ids = dfg.instruction_results(instruction_id); self.convert_ssa_function_call(*func_id, arguments, dfg, result_ids); } - Value::Intrinsic(Intrinsic::BlackBox(bb_func)) => { - // Slices are represented as a tuple of (length, slice contents). - // We must check the inputs to determine if there are slices - // and make sure that we pass the correct inputs to the black box function call. - // The loop below only keeps the slice contents, so that - // setting up a black box function with slice inputs matches the expected - // number of arguments specified in the function signature. - let mut arguments_no_slice_len = Vec::new(); - for (i, arg) in arguments.iter().enumerate() { - if matches!(dfg.type_of_value(*arg), Type::Numeric(_)) { - if i < arguments.len() - 1 { - if !matches!(dfg.type_of_value(arguments[i + 1]), Type::Slice(_)) { - arguments_no_slice_len.push(*arg); - } + Value::Intrinsic(intrinsic) => { + // This match could be combined with the above but without it rust analyzer + // can't automatically insert any missing cases + match intrinsic { + Intrinsic::ArrayLen => { + let result_variable = self.variables.define_single_addr_variable( + self.function_context, + self.brillig_context, + dfg.instruction_results(instruction_id)[0], + dfg, + ); + let param_id = arguments[0]; + // Slices are represented as a tuple in the form: (length, slice contents). + // Thus, we can expect the first argument to a field in the case of a slice + // or an array in the case of an array. + if let Type::Numeric(_) = dfg.type_of_value(param_id) { + let len_variable = self.convert_ssa_value(arguments[0], dfg); + let length = len_variable.extract_single_addr(); + self.brillig_context + .mov_instruction(result_variable.address, length.address); } else { - arguments_no_slice_len.push(*arg); + self.convert_ssa_array_len( + arguments[0], + result_variable.address, + dfg, + ); } - } else { - arguments_no_slice_len.push(*arg); } - } - - let function_arguments = - vecmap(&arguments_no_slice_len, |arg| self.convert_ssa_value(*arg, dfg)); - let function_results = dfg.instruction_results(instruction_id); - let function_results = vecmap(function_results, |result| { - self.allocate_external_call_result(*result, dfg) - }); - convert_black_box_call( - self.brillig_context, - bb_func, - &function_arguments, - &function_results, - ); - } - Value::Intrinsic(Intrinsic::ArrayLen) => { - let result_variable = self.variables.define_single_addr_variable( - self.function_context, - self.brillig_context, - dfg.instruction_results(instruction_id)[0], - dfg, - ); - let param_id = arguments[0]; - // Slices are represented as a tuple in the form: (length, slice contents). - // Thus, we can expect the first argument to a field in the case of a slice - // or an array in the case of an array. - if let Type::Numeric(_) = dfg.type_of_value(param_id) { - let len_variable = self.convert_ssa_value(arguments[0], dfg); - let length = len_variable.extract_single_addr(); - self.brillig_context - .mov_instruction(result_variable.address, length.address); - } else { - self.convert_ssa_array_len(arguments[0], result_variable.address, dfg); - } - } - Value::Intrinsic(Intrinsic::AsSlice) => { - let source_variable = self.convert_ssa_value(arguments[0], dfg); - let result_ids = dfg.instruction_results(instruction_id); - let destination_len_variable = self.variables.define_single_addr_variable( - self.function_context, - self.brillig_context, - result_ids[0], - dfg, - ); - let destination_variable = self.variables.define_variable( - self.function_context, - self.brillig_context, - result_ids[1], - dfg, - ); - let destination_vector = destination_variable.extract_vector(); - let source_array = source_variable.extract_array(); - let element_size = dfg.type_of_value(arguments[0]).element_size(); - - let source_size_register = self - .brillig_context - .make_usize_constant_instruction(source_array.size.into()); - - // we need to explicitly set the destination_len_variable - self.brillig_context.codegen_usize_op( - source_size_register.address, - destination_len_variable.address, - BrilligBinaryOp::UnsignedDiv, - element_size, - ); - - self.brillig_context.codegen_initialize_vector( - destination_vector, - source_size_register, - None, - ); - - // Items - let vector_items_pointer = - self.brillig_context.codegen_make_vector_items_pointer(destination_vector); - let array_items_pointer = - self.brillig_context.codegen_make_array_items_pointer(source_array); - - self.brillig_context.codegen_mem_copy( - array_items_pointer, - vector_items_pointer, - source_size_register, - ); - - self.brillig_context.deallocate_single_addr(source_size_register); - self.brillig_context.deallocate_register(vector_items_pointer); - self.brillig_context.deallocate_register(array_items_pointer); - } - Value::Intrinsic( - Intrinsic::SlicePushBack - | Intrinsic::SlicePopBack - | Intrinsic::SlicePushFront - | Intrinsic::SlicePopFront - | Intrinsic::SliceInsert - | Intrinsic::SliceRemove, - ) => { - self.convert_ssa_slice_intrinsic_call( - dfg, - &dfg[dfg.resolve(*func)], - instruction_id, - arguments, - ); - } - Value::Intrinsic(Intrinsic::ToRadix(endianness)) => { - let results = dfg.instruction_results(instruction_id); - - let source = self.convert_ssa_single_addr_value(arguments[0], dfg); - let radix = self.convert_ssa_single_addr_value(arguments[1], dfg); - - let target_array = self - .variables - .define_variable( - self.function_context, - self.brillig_context, - results[0], - dfg, - ) - .extract_array(); - - self.brillig_context.codegen_to_radix( - source, - target_array, - radix, - matches!(endianness, Endian::Little), - false, - ); - } - Value::Intrinsic(Intrinsic::ToBits(endianness)) => { - let results = dfg.instruction_results(instruction_id); + Intrinsic::AsSlice => { + let source_variable = self.convert_ssa_value(arguments[0], dfg); + let result_ids = dfg.instruction_results(instruction_id); + let destination_len_variable = + self.variables.define_single_addr_variable( + self.function_context, + self.brillig_context, + result_ids[0], + dfg, + ); + let destination_variable = self.variables.define_variable( + self.function_context, + self.brillig_context, + result_ids[1], + dfg, + ); + let destination_vector = destination_variable.extract_vector(); + let source_array = source_variable.extract_array(); + let element_size = dfg.type_of_value(arguments[0]).element_size(); - let source = self.convert_ssa_single_addr_value(arguments[0], dfg); + let source_size_register = self + .brillig_context + .make_usize_constant_instruction(source_array.size.into()); + + // we need to explicitly set the destination_len_variable + self.brillig_context.codegen_usize_op( + source_size_register.address, + destination_len_variable.address, + BrilligBinaryOp::UnsignedDiv, + element_size, + ); - let target_array = self - .variables - .define_variable( - self.function_context, - self.brillig_context, - results[0], - dfg, - ) - .extract_array(); + self.brillig_context.codegen_initialize_vector( + destination_vector, + source_size_register, + None, + ); - let two = self.brillig_context.make_usize_constant_instruction(2_usize.into()); + // Items + let vector_items_pointer = self + .brillig_context + .codegen_make_vector_items_pointer(destination_vector); + let array_items_pointer = + self.brillig_context.codegen_make_array_items_pointer(source_array); + + self.brillig_context.codegen_mem_copy( + array_items_pointer, + vector_items_pointer, + source_size_register, + ); - self.brillig_context.codegen_to_radix( - source, - target_array, - two, - matches!(endianness, Endian::Little), - true, - ); + self.brillig_context.deallocate_single_addr(source_size_register); + self.brillig_context.deallocate_register(vector_items_pointer); + self.brillig_context.deallocate_register(array_items_pointer); + } + Intrinsic::SlicePushBack + | Intrinsic::SlicePopBack + | Intrinsic::SlicePushFront + | Intrinsic::SlicePopFront + | Intrinsic::SliceInsert + | Intrinsic::SliceRemove => { + self.convert_ssa_slice_intrinsic_call( + dfg, + &dfg[dfg.resolve(*func)], + instruction_id, + arguments, + ); + } + Intrinsic::ToBits(endianness) => { + let results = dfg.instruction_results(instruction_id); + + let source = self.convert_ssa_single_addr_value(arguments[0], dfg); + + let target_array = self + .variables + .define_variable( + self.function_context, + self.brillig_context, + results[0], + dfg, + ) + .extract_array(); + + let two = self + .brillig_context + .make_usize_constant_instruction(2_usize.into()); + + self.brillig_context.codegen_to_radix( + source, + target_array, + two, + matches!(endianness, Endian::Little), + true, + ); - self.brillig_context.deallocate_single_addr(two); - } + self.brillig_context.deallocate_single_addr(two); + } - // `Intrinsic::AsWitness` is used to provide hints to acir-gen on optimal expression splitting. - // It is then useless in the brillig runtime and so we can ignore it - Value::Intrinsic(Intrinsic::AsWitness) => (), - Value::Intrinsic(Intrinsic::FieldLessThan) => { - let lhs = self.convert_ssa_single_addr_value(arguments[0], dfg); - assert!(lhs.bit_size == FieldElement::max_num_bits()); - let rhs = self.convert_ssa_single_addr_value(arguments[1], dfg); - assert!(rhs.bit_size == FieldElement::max_num_bits()); - - let results = dfg.instruction_results(instruction_id); - let destination = self - .variables - .define_variable( - self.function_context, - self.brillig_context, - results[0], - dfg, - ) - .extract_single_addr(); - assert!(destination.bit_size == 1); + Intrinsic::ToRadix(endianness) => { + let results = dfg.instruction_results(instruction_id); + + let source = self.convert_ssa_single_addr_value(arguments[0], dfg); + let radix = self.convert_ssa_single_addr_value(arguments[1], dfg); + + let target_array = self + .variables + .define_variable( + self.function_context, + self.brillig_context, + results[0], + dfg, + ) + .extract_array(); + + self.brillig_context.codegen_to_radix( + source, + target_array, + radix, + matches!(endianness, Endian::Little), + false, + ); + } + Intrinsic::BlackBox(bb_func) => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the inputs to determine if there are slices + // and make sure that we pass the correct inputs to the black box function call. + // The loop below only keeps the slice contents, so that + // setting up a black box function with slice inputs matches the expected + // number of arguments specified in the function signature. + let mut arguments_no_slice_len = Vec::new(); + for (i, arg) in arguments.iter().enumerate() { + if matches!(dfg.type_of_value(*arg), Type::Numeric(_)) { + if i < arguments.len() - 1 { + if !matches!( + dfg.type_of_value(arguments[i + 1]), + Type::Slice(_) + ) { + arguments_no_slice_len.push(*arg); + } + } else { + arguments_no_slice_len.push(*arg); + } + } else { + arguments_no_slice_len.push(*arg); + } + } - self.brillig_context.binary_instruction( - lhs, - rhs, - destination, - BrilligBinaryOp::LessThan, - ); + let function_arguments = vecmap(&arguments_no_slice_len, |arg| { + self.convert_ssa_value(*arg, dfg) + }); + let function_results = dfg.instruction_results(instruction_id); + let function_results = vecmap(function_results, |result| { + self.allocate_external_call_result(*result, dfg) + }); + convert_black_box_call( + self.brillig_context, + bb_func, + &function_arguments, + &function_results, + ); + } + // `Intrinsic::AsWitness` is used to provide hints to acir-gen on optimal expression splitting. + // It is then useless in the brillig runtime and so we can ignore it + Intrinsic::AsWitness => (), + Intrinsic::FieldLessThan => { + let lhs = self.convert_ssa_single_addr_value(arguments[0], dfg); + assert!(lhs.bit_size == FieldElement::max_num_bits()); + let rhs = self.convert_ssa_single_addr_value(arguments[1], dfg); + assert!(rhs.bit_size == FieldElement::max_num_bits()); + + let results = dfg.instruction_results(instruction_id); + let destination = self + .variables + .define_variable( + self.function_context, + self.brillig_context, + results[0], + dfg, + ) + .extract_single_addr(); + assert!(destination.bit_size == 1); + + self.brillig_context.binary_instruction( + lhs, + rhs, + destination, + BrilligBinaryOp::LessThan, + ); + } + Intrinsic::ArrayRefCount | Intrinsic::SliceRefCount => { + let array = self.convert_ssa_value(arguments[0], dfg); + let result = dfg.instruction_results(instruction_id)[0]; + + let destination = self.variables.define_variable( + self.function_context, + self.brillig_context, + result, + dfg, + ); + let destination = destination.extract_register(); + let array = array.extract_register(); + self.brillig_context.load_instruction(destination, array); + } + Intrinsic::FromField + | Intrinsic::AsField + | Intrinsic::IsUnconstrained + | Intrinsic::DerivePedersenGenerators + | Intrinsic::ApplyRangeConstraint + | Intrinsic::StrAsBytes + | Intrinsic::AssertConstant + | Intrinsic::StaticAssert + | Intrinsic::ArrayAsStrUnchecked => { + unreachable!("unsupported function call type {:?}", dfg[*func]) + } + } } - _ => { + Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } => { unreachable!("unsupported function call type {:?}", dfg[*func]) } }, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index cf884c98be97..7a4e336c33ec 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -205,16 +205,18 @@ impl Context { | Intrinsic::IsUnconstrained => {} Intrinsic::ArrayLen | Intrinsic::ArrayAsStrUnchecked + | Intrinsic::ArrayRefCount | Intrinsic::AsField | Intrinsic::AsSlice | Intrinsic::BlackBox(..) | Intrinsic::DerivePedersenGenerators | Intrinsic::FromField + | Intrinsic::SliceInsert | Intrinsic::SlicePushBack | Intrinsic::SlicePushFront | Intrinsic::SlicePopBack | Intrinsic::SlicePopFront - | Intrinsic::SliceInsert + | Intrinsic::SliceRefCount | Intrinsic::SliceRemove | Intrinsic::StaticAssert | Intrinsic::StrAsBytes 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 c1a7f14e0d14..504eecf48018 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 @@ -119,6 +119,29 @@ impl DominatorTree { } } + /// 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. + pub(crate) fn find_map_dominator( + &self, + 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); + } + 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 { @@ -448,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 + ); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index ba854d6a3c11..6737b335b7da 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -11,7 +11,7 @@ use fxhash::FxHasher64; use iter_extended::vecmap; use noirc_frontend::hir_def::types::Type as HirType; -use crate::ssa::opt::flatten_cfg::value_merger::ValueMerger; +use crate::ssa::{ir::function::RuntimeType, opt::flatten_cfg::value_merger::ValueMerger}; use super::{ basic_block::BasicBlockId, @@ -45,8 +45,7 @@ pub(crate) type InstructionId = Id; /// - Opcodes which the IR knows the target machine has /// special support for. (LowLevel) /// - Opcodes which have no function definition in the -/// source code and must be processed by the IR. An example -/// of this is println. +/// source code and must be processed by the IR. #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub(crate) enum Intrinsic { ArrayLen, @@ -71,6 +70,8 @@ pub(crate) enum Intrinsic { IsUnconstrained, DerivePedersenGenerators, FieldLessThan, + ArrayRefCount, + SliceRefCount, } impl std::fmt::Display for Intrinsic { @@ -100,6 +101,8 @@ impl std::fmt::Display for Intrinsic { Intrinsic::IsUnconstrained => write!(f, "is_unconstrained"), Intrinsic::DerivePedersenGenerators => write!(f, "derive_pedersen_generators"), Intrinsic::FieldLessThan => write!(f, "field_less_than"), + Intrinsic::ArrayRefCount => write!(f, "array_refcount"), + Intrinsic::SliceRefCount => write!(f, "slice_refcount"), } } } @@ -108,11 +111,18 @@ impl Intrinsic { /// Returns whether the `Intrinsic` has side effects. /// /// If there are no side effects then the `Intrinsic` can be removed if the result is unused. + /// + /// An example of a side effect is increasing the reference count of an array, but functions + /// which can fail due to implicit constraints are also considered to have a side effect. pub(crate) fn has_side_effects(&self) -> bool { match self { Intrinsic::AssertConstant | Intrinsic::StaticAssert | Intrinsic::ApplyRangeConstraint + // Array & slice ref counts are treated as having side effects since they operate + // on hidden variables on otherwise identical array values. + | Intrinsic::ArrayRefCount + | Intrinsic::SliceRefCount | Intrinsic::AsWitness => true, // These apply a constraint that the input must fit into a specified number of limbs. @@ -144,6 +154,39 @@ impl Intrinsic { } } + /// Intrinsics which only have a side effect due to the chance that + /// they can fail a constraint can be deduplicated. + pub(crate) fn can_be_deduplicated(&self, deduplicate_with_predicate: bool) -> bool { + match self { + // These apply a constraint in the form of ACIR opcodes, but they can be deduplicated + // if the inputs are the same. If they depend on a side effect variable (e.g. because + // they were in an if-then-else) then `handle_instruction_side_effects` in `flatten_cfg` + // will have attached the condition variable to their inputs directly, so they don't + // directly depend on the corresponding `enable_side_effect` instruction any more. + // However, to conform with the expectations of `Instruction::can_be_deduplicated` and + // `constant_folding` we only use this information if the caller shows interest in it. + Intrinsic::ToBits(_) + | Intrinsic::ToRadix(_) + | Intrinsic::BlackBox( + BlackBoxFunc::MultiScalarMul + | BlackBoxFunc::EmbeddedCurveAdd + | BlackBoxFunc::RecursiveAggregation, + ) => deduplicate_with_predicate, + + // Operations that remove items from a slice don't modify the slice, they just assert it's non-empty. + Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceRemove => { + deduplicate_with_predicate + } + + Intrinsic::AssertConstant + | Intrinsic::StaticAssert + | Intrinsic::ApplyRangeConstraint + | Intrinsic::AsWitness => deduplicate_with_predicate, + + _ => !self.has_side_effects(), + } + } + /// Lookup an Intrinsic by name and return it if found. /// If there is no such intrinsic by that name, None is returned. pub(crate) fn lookup(name: &str) -> Option { @@ -171,6 +214,8 @@ impl Intrinsic { "is_unconstrained" => Some(Intrinsic::IsUnconstrained), "derive_pedersen_generators" => Some(Intrinsic::DerivePedersenGenerators), "field_less_than" => Some(Intrinsic::FieldLessThan), + "array_refcount" => Some(Intrinsic::ArrayRefCount), + "slice_refcount" => Some(Intrinsic::SliceRefCount), other => BlackBoxFunc::lookup(other).map(Intrinsic::BlackBox), } @@ -235,7 +280,7 @@ pub(crate) enum Instruction { /// - `code1` will have side effects iff `condition1` evaluates to `true` /// /// This instruction is only emitted after the cfg flattening pass, and is used to annotate - /// instruction regions with an condition that corresponds to their position in the CFG's + /// instruction regions with a condition that corresponds to their position in the CFG's /// if-branching structure. EnableSideEffectsIf { condition: ValueId }, @@ -270,9 +315,6 @@ pub(crate) enum Instruction { /// else_value /// } /// ``` - /// - /// Where we save the result of !then_condition so that we have the same - /// ValueId for it each time. IfElse { then_condition: ValueId, then_value: ValueId, else_value: ValueId }, /// Creates a new array or slice. @@ -320,10 +362,53 @@ 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. + Binary(_) + | Cast(_, _) + | Not(_) + | Truncate { .. } + | IfElse { .. } + | ArrayGet { .. } + | ArraySet { .. } => self.requires_acir_gen_predicate(dfg), + } + } + /// 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 /// rely on predicates can be deduplicated as well. + /// + /// Some instructions get the predicate attached to their inputs by `handle_instruction_side_effects` in `flatten_cfg`. + /// These can be deduplicated because they implicitly depend on the predicate, not only when the caller uses the + /// predicate variable as a key to cache results. However, to avoid tight coupling between passes, we make the deduplication + /// conditional on whether the caller wants the predicate to be taken into account or not. pub(crate) fn can_be_deduplicated( &self, dfg: &DataFlowGraph, @@ -341,7 +426,9 @@ impl Instruction { | DecrementRc { .. } => false, Call { func, .. } => match dfg[*func] { - Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), + Value::Intrinsic(intrinsic) => { + intrinsic.can_be_deduplicated(deduplicate_with_predicate) + } _ => false, }, @@ -391,8 +478,19 @@ impl Instruction { | ArraySet { .. } | MakeArray { .. } => true, + // Store instructions must be removed by DIE in acir code, any load + // instructions should already be unused by that point. + // + // Note that this check assumes that it is being performed after the flattening + // pass and after the last mem2reg pass. This is currently the case for the DIE + // pass where this check is done, but does mean that we cannot perform mem2reg + // after the DIE pass. + Store { .. } => { + matches!(function.runtime(), RuntimeType::Acir(_)) + && function.reachable_blocks().len() == 1 + } + Constrain(..) - | Store { .. } | EnableSideEffectsIf { .. } | IncrementRc { .. } | DecrementRc { .. } @@ -403,6 +501,7 @@ impl Instruction { // Explicitly allows removal of unused ec operations, even if they can fail Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) | Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => true, + Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), // All foreign functions are treated as having side effects. @@ -418,7 +517,7 @@ impl Instruction { } } - /// If true the instruction will depends on enable_side_effects context during acir-gen + /// If true the instruction will depend on `enable_side_effects` context during acir-gen. pub(crate) fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool { match self { Instruction::Binary(binary) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 7e41512fd8f3..4be37b3c626b 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -368,6 +368,8 @@ pub(super) fn simplify_call( SimplifyResult::None } } + Intrinsic::ArrayRefCount => SimplifyResult::None, + Intrinsic::SliceRefCount => SimplifyResult::None, } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs index 4d768caa36b4..e32eaa701861 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs @@ -24,6 +24,10 @@ impl AliasSet { Self { aliases: Some(aliases) } } + pub(super) fn known_multiple(values: BTreeSet) -> AliasSet { + Self { aliases: Some(values) } + } + /// In rare cases, such as when creating an empty array of references, the set of aliases for a /// particular value will be known to be zero, which is distinct from being unknown and /// possibly referring to any alias. diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs index 532785d29283..f4265b2466de 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mem2reg/block.rs @@ -34,6 +34,9 @@ pub(super) struct Block { /// The last instance of a `Store` instruction to each address in this block pub(super) last_stores: im::OrdMap, + + // The last instance of a `Load` instruction to each address in this block + pub(super) last_loads: im::OrdMap, } /// An `Expression` here is used to represent a canonical key @@ -237,4 +240,14 @@ impl Block { Cow::Owned(AliasSet::unknown()) } + + pub(super) fn set_last_load(&mut self, address: ValueId, instruction: InstructionId) { + self.last_loads.insert(address, instruction); + } + + pub(super) fn keep_last_load_for(&mut self, address: ValueId, function: &Function) { + let address = function.dfg.resolve(address); + self.last_loads.remove(&address); + self.for_each_alias_of(address, |block, alias| block.last_loads.remove(&alias)); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 0517f9ef89fc..f735d9300ced 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -180,6 +180,8 @@ impl Context { | Intrinsic::AsWitness | Intrinsic::IsUnconstrained | Intrinsic::DerivePedersenGenerators + | Intrinsic::ArrayRefCount + | Intrinsic::SliceRefCount | Intrinsic::FieldLessThan => false, }, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs index 8076bc3cc997..8e25c3f0a35a 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_if_else.rs @@ -232,6 +232,8 @@ fn slice_capacity_change( | Intrinsic::DerivePedersenGenerators | Intrinsic::ToBits(_) | Intrinsic::ToRadix(_) + | Intrinsic::ArrayRefCount + | Intrinsic::SliceRefCount | Intrinsic::FieldLessThan => SizeChange::None, } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 44e25f9d4a13..777c16dacd17 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -18,8 +18,6 @@ //! //! When unrolling ACIR code, we remove reference count instructions because they are //! only used by Brillig bytecode. -use std::collections::HashSet; - use acvm::{acir::AcirField, FieldElement}; use crate::{ @@ -39,7 +37,7 @@ use crate::{ ssa_gen::Ssa, }, }; -use fxhash::FxHashMap as HashMap; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; impl Ssa { /// Loop unrolling can return errors, since ACIR functions need to be fully unrolled. @@ -163,9 +161,9 @@ impl Loops { loops.sort_by_key(|loop_| loop_.blocks.len()); Self { - failed_to_unroll: HashSet::new(), + failed_to_unroll: HashSet::default(), yet_to_unroll: loops, - modified_blocks: HashSet::new(), + modified_blocks: HashSet::default(), cfg, } } @@ -209,7 +207,7 @@ impl Loop { back_edge_start: BasicBlockId, cfg: &ControlFlowGraph, ) -> Self { - let mut blocks = HashSet::new(); + let mut blocks = HashSet::default(); blocks.insert(header); let mut insert = |block, stack: &mut Vec| { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 0c6041029dab..ddc3365b5519 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -172,6 +172,7 @@ impl<'a> FunctionContext<'a> { /// Always returns a Value::Mutable wrapping the allocate instruction. pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value { let element_type = self.builder.current_function.dfg.type_of_value(value_to_store); + self.builder.increment_array_reference_count(value_to_store); let alloc = self.builder.insert_allocate(element_type); self.builder.insert_store(alloc, value_to_store); let typ = self.builder.type_of_value(value_to_store); @@ -735,7 +736,6 @@ impl<'a> FunctionContext<'a> { // Reference counting in brillig relies on us incrementing reference // counts when arrays/slices are constructed or indexed. // Thus, if we dereference an lvalue which happens to be array/slice we should increment its reference counter. - self.builder.increment_array_reference_count(reference); self.builder.insert_load(reference, element_type).into() }) } @@ -916,7 +916,10 @@ impl<'a> FunctionContext<'a> { let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec(); for parameter in parameters { - self.builder.increment_array_reference_count(parameter); + // Avoid reference counts for immutable arrays that aren't behind references. + if self.builder.current_function.dfg.value_is_reference(parameter) { + self.builder.increment_array_reference_count(parameter); + } } entry @@ -933,7 +936,9 @@ impl<'a> FunctionContext<'a> { dropped_parameters.retain(|parameter| !terminator_args.contains(parameter)); for parameter in dropped_parameters { - self.builder.decrement_array_reference_count(parameter); + if self.builder.current_function.dfg.value_is_reference(parameter) { + self.builder.decrement_array_reference_count(parameter); + } } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index c50f0a7f45c5..d28236bd3608 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -665,12 +665,11 @@ impl<'a> FunctionContext<'a> { values = values.map(|value| { let value = value.eval(self); - // Make sure to increment array reference counts on each let binding - self.builder.increment_array_reference_count(value); - Tree::Leaf(if let_expr.mutable { self.new_mutable_variable(value) } else { + // `new_mutable_variable` already increments rcs internally + self.builder.increment_array_reference_count(value); value::Value::Normal(value) }) }); diff --git a/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md b/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md index a1d59bf31662..f3badde62bed 100644 --- a/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md +++ b/noir/noir-repo/docs/docs/noir/concepts/data_types/integers.md @@ -47,6 +47,17 @@ fn main() { The bit size determines the maximum and minimum range of value the integer type can store. For example, an `i8` variable can store a value in the range of -128 to 127 (i.e. $\\-2^{7}\\$ to $\\2^{7}-1\\$). + +```rust +fn main(x: i16, y: i16) { + // modulo + let c = x % y; + let c = x % -13; +} +``` + +Modulo operation is defined for negative integers thanks to integer division, so that the equality `x = (x/y)*y + (x%y)` holds. + ## 128 bits Unsigned Integers The built-in structure `U128` allows you to use 128-bit unsigned integers almost like a native integer type. However, there are some differences to keep in mind: diff --git a/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Nargo.toml b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Nargo.toml new file mode 100644 index 000000000000..9590789f52ed --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "loop_invariant_regression" +type = "bin" +authors = [""] +compiler_version = ">=0.38.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Prover.toml b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Prover.toml new file mode 100644 index 000000000000..18680c805a73 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/Prover.toml @@ -0,0 +1,2 @@ +x = "2" +y = "3" diff --git a/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr new file mode 100644 index 000000000000..25f6e92f8680 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr @@ -0,0 +1,13 @@ +// Tests a simple loop where we expect loop invariant instructions +// to be hoisted to the loop's pre-header block. +fn main(x: u32, y: u32) { + loop(4, x, y); +} + +fn loop(upper_bound: u32, x: u32, y: u32) { + for _ in 0..upper_bound { + let mut z = x * y; + z = z * x; + assert_eq(z, 12); + } +} diff --git a/noir/noir-repo/test_programs/execution_success/reference_counts/Nargo.toml b/noir/noir-repo/test_programs/execution_success/reference_counts/Nargo.toml new file mode 100644 index 000000000000..ae787e0ccb9d --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/reference_counts/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "reference_counts" +type = "bin" +authors = [""] +compiler_version = ">=0.35.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/reference_counts/Prover.toml b/noir/noir-repo/test_programs/execution_success/reference_counts/Prover.toml new file mode 100644 index 000000000000..c01dd9462d8a --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/reference_counts/Prover.toml @@ -0,0 +1,2 @@ +x = 5 +b = true diff --git a/noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr b/noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr new file mode 100644 index 000000000000..7ab7de893fa5 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/reference_counts/src/main.nr @@ -0,0 +1,40 @@ +fn main() { + let mut array = [0, 1, 2]; + assert_refcount(array, 1); + + borrow(array, std::mem::array_refcount(array)); + borrow_mut(&mut array, std::mem::array_refcount(array)); + copy_mut(array, std::mem::array_refcount(array)); +} + +fn borrow(array: [Field; 3], rc_before_call: u32) { + assert_refcount(array, rc_before_call); + println(array[0]); +} + +fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { + assert_refcount(*array, rc_before_call + 0); // Issue! This should be rc_before_call + 1 + array[0] = 5; + println(array[0]); +} + +fn copy_mut(mut array: [Field; 3], rc_before_call: u32) { + assert_refcount(array, rc_before_call + 0); // Issue! This should be rc_before_call + 1 + array[0] = 6; + println(array[0]); +} + +fn assert_refcount(array: [Field; 3], expected: u32) { + let count = std::mem::array_refcount(array); + + // All refcounts are zero when running this as a constrained program + if std::runtime::is_unconstrained() { + if count != expected { + // Brillig doesn't print the actual & expected arguments on assertion failure + println(f"actual = {count}, expected = {expected}"); + } + assert_eq(count, expected); + } else { + assert_eq(count, 0); + } +} diff --git a/noir/noir-repo/test_programs/execution_success/regression/Prover.toml b/noir/noir-repo/test_programs/execution_success/regression/Prover.toml index 2875190982f8..c81cbf10fbb3 100644 --- a/noir/noir-repo/test_programs/execution_success/regression/Prover.toml +++ b/noir/noir-repo/test_programs/execution_success/regression/Prover.toml @@ -1,2 +1,4 @@ x = [0x3f, 0x1c, 0xb8, 0x99, 0xab] z = 3 +u = "169" +v = "-13" \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/regression/src/main.nr b/noir/noir-repo/test_programs/execution_success/regression/src/main.nr index 1c2f557d2cda..809fdbe4b28c 100644 --- a/noir/noir-repo/test_programs/execution_success/regression/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/regression/src/main.nr @@ -94,7 +94,7 @@ fn bitshift_variable(idx: u8) -> u64 { bits } -fn main(x: [u8; 5], z: Field) { +fn main(x: [u8; 5], z: Field, u: i16, v: i16) { //Issue 1144 let (nib, len) = compact_decode(x, z); assert(len == 5); @@ -130,4 +130,12 @@ fn main(x: [u8; 5], z: Field) { assert(result_0 == 1); let result_4 = bitshift_variable(4); assert(result_4 == 16); + + // Issue 6609 + assert(u % -13 == 0); + assert(u % v == 0); + assert(u % -11 == 4); + assert(-u % -11 == -4); + assert(u % -11 == u % (v + 2)); + assert(-u % -11 == -u % (v + 2)); } From 6bacff6b6241be4c1c22755346b6d98f6e127e58 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 28 Nov 2024 22:08:58 +0000 Subject: [PATCH 2/5] . --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 12 +----------- .../noirc_evaluator/src/ssa/ssa_gen/context.rs | 11 +++-------- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 6737b335b7da..b5799d2e37a2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -478,19 +478,9 @@ impl Instruction { | ArraySet { .. } | MakeArray { .. } => true, - // Store instructions must be removed by DIE in acir code, any load - // instructions should already be unused by that point. - // - // Note that this check assumes that it is being performed after the flattening - // pass and after the last mem2reg pass. This is currently the case for the DIE - // pass where this check is done, but does mean that we cannot perform mem2reg - // after the DIE pass. - Store { .. } => { - matches!(function.runtime(), RuntimeType::Acir(_)) - && function.reachable_blocks().len() == 1 - } Constrain(..) + | Store { .. } | EnableSideEffectsIf { .. } | IncrementRc { .. } | DecrementRc { .. } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index ddc3365b5519..538863e6a5db 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -172,7 +172,6 @@ impl<'a> FunctionContext<'a> { /// Always returns a Value::Mutable wrapping the allocate instruction. pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value { let element_type = self.builder.current_function.dfg.type_of_value(value_to_store); - self.builder.increment_array_reference_count(value_to_store); let alloc = self.builder.insert_allocate(element_type); self.builder.insert_store(alloc, value_to_store); let typ = self.builder.type_of_value(value_to_store); @@ -736,6 +735,7 @@ impl<'a> FunctionContext<'a> { // Reference counting in brillig relies on us incrementing reference // counts when arrays/slices are constructed or indexed. // Thus, if we dereference an lvalue which happens to be array/slice we should increment its reference counter. + self.builder.increment_array_reference_count(value_to_store); self.builder.insert_load(reference, element_type).into() }) } @@ -916,10 +916,7 @@ impl<'a> FunctionContext<'a> { let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec(); for parameter in parameters { - // Avoid reference counts for immutable arrays that aren't behind references. - if self.builder.current_function.dfg.value_is_reference(parameter) { - self.builder.increment_array_reference_count(parameter); - } + self.builder.increment_array_reference_count(parameter); } entry @@ -936,9 +933,7 @@ impl<'a> FunctionContext<'a> { dropped_parameters.retain(|parameter| !terminator_args.contains(parameter)); for parameter in dropped_parameters { - if self.builder.current_function.dfg.value_is_reference(parameter) { - self.builder.decrement_array_reference_count(parameter); - } + self.builder.decrement_array_reference_count(parameter); } } From b561e6296b4afedafca09ec08db24db200cfb3c7 Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Thu, 28 Nov 2024 22:10:32 +0000 Subject: [PATCH 3/5] . --- .../compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- .../compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 538863e6a5db..0c6041029dab 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -735,7 +735,7 @@ impl<'a> FunctionContext<'a> { // Reference counting in brillig relies on us incrementing reference // counts when arrays/slices are constructed or indexed. // Thus, if we dereference an lvalue which happens to be array/slice we should increment its reference counter. - self.builder.increment_array_reference_count(value_to_store); + self.builder.increment_array_reference_count(reference); self.builder.insert_load(reference, element_type).into() }) } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d28236bd3608..c50f0a7f45c5 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -665,11 +665,12 @@ impl<'a> FunctionContext<'a> { values = values.map(|value| { let value = value.eval(self); + // Make sure to increment array reference counts on each let binding + self.builder.increment_array_reference_count(value); + Tree::Leaf(if let_expr.mutable { self.new_mutable_variable(value) } else { - // `new_mutable_variable` already increments rcs internally - self.builder.increment_array_reference_count(value); value::Value::Normal(value) }) }); From 0610d7fef2f8a31584b0013b0eb4e29bce3d268b Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 28 Nov 2024 23:02:34 +0000 Subject: [PATCH 4/5] Update noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs --- .../noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index b5799d2e37a2..6c1b51f83d68 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -478,7 +478,6 @@ impl Instruction { | ArraySet { .. } | MakeArray { .. } => true, - Constrain(..) | Store { .. } | EnableSideEffectsIf { .. } From 5c1b4e65304a8740fb89e35d7ccfd2267f098dbb Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 28 Nov 2024 23:16:42 +0000 Subject: [PATCH 5/5] Update noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs --- .../compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 6c1b51f83d68..f606fffbf91e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -11,7 +11,7 @@ use fxhash::FxHasher64; use iter_extended::vecmap; use noirc_frontend::hir_def::types::Type as HirType; -use crate::ssa::{ir::function::RuntimeType, opt::flatten_cfg::value_merger::ValueMerger}; +use crate::ssa::opt::flatten_cfg::value_merger::ValueMerger; use super::{ basic_block::BasicBlockId,