From 768aa7ce9ed809fa2c9d368f2b2f625d9689a63f Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 3 Dec 2024 09:33:51 -0600 Subject: [PATCH 1/3] feat: Add `BoundedVec::from_parts` and `BoundedVec::from_parts_unchecked` (#6691) Co-authored-by: Maxim Vezenov --- .../standard_library/containers/boundedvec.md | 36 ++++++++ noir_stdlib/src/collections/bounded_vec.nr | 92 ++++++++++++++++++- 2 files changed, 127 insertions(+), 1 deletion(-) diff --git a/docs/docs/noir/standard_library/containers/boundedvec.md b/docs/docs/noir/standard_library/containers/boundedvec.md index 98b7d584033..4efb1e4ea0f 100644 --- a/docs/docs/noir/standard_library/containers/boundedvec.md +++ b/docs/docs/noir/standard_library/containers/boundedvec.md @@ -246,6 +246,42 @@ Example: let bounded_vec: BoundedVec = BoundedVec::from_array([1, 2, 3]) ``` +### from_parts + +```rust +pub fn from_parts(mut array: [T; MaxLen], len: u32) -> Self +``` + +Creates a new BoundedVec from the given array and length. +The given length must be less than or equal to the length of the array. + +This function will zero out any elements at or past index `len` of `array`. +This incurs an extra runtime cost of O(MaxLen). If you are sure your array is +zeroed after that index, you can use `from_parts_unchecked` to remove the extra loop. + +Example: + +#include_code from-parts noir_stdlib/src/collections/bounded_vec.nr rust + +### from_parts_unchecked + +```rust +pub fn from_parts_unchecked(array: [T; MaxLen], len: u32) -> Self +``` + +Creates a new BoundedVec from the given array and length. +The given length must be less than or equal to the length of the array. + +This function is unsafe because it expects all elements past the `len` index +of `array` to be zeroed, but does not check for this internally. Use `from_parts` +for a safe version of this function which does zero out any indices past the +given length. Invalidating this assumption can notably cause `BoundedVec::eq` +to give incorrect results since it will check even elements past `len`. + +Example: + +#include_code from-parts-unchecked noir_stdlib/src/collections/bounded_vec.nr rust + ### map ```rust diff --git a/noir_stdlib/src/collections/bounded_vec.nr b/noir_stdlib/src/collections/bounded_vec.nr index f33890f197e..0ad39c518c4 100644 --- a/noir_stdlib/src/collections/bounded_vec.nr +++ b/noir_stdlib/src/collections/bounded_vec.nr @@ -420,6 +420,58 @@ impl BoundedVec { } ret } + + /// Creates a new BoundedVec from the given array and length. + /// The given length must be less than or equal to the length of the array. + /// + /// This function will zero out any elements at or past index `len` of `array`. + /// This incurs an extra runtime cost of O(MaxLen). If you are sure your array is + /// zeroed after that index, you can use `from_parts_unchecked` to remove the extra loop. + /// + /// Example: + /// + /// ```noir + /// let vec: BoundedVec = BoundedVec::from_parts([1, 2, 3, 0], 3); + /// assert_eq(vec.len(), 3); + /// ``` + pub fn from_parts(mut array: [T; MaxLen], len: u32) -> Self { + assert(len <= MaxLen); + let zeroed = crate::mem::zeroed(); + for i in 0..MaxLen { + if i >= len { + array[i] = zeroed; + } + } + BoundedVec { storage: array, len } + } + + /// Creates a new BoundedVec from the given array and length. + /// The given length must be less than or equal to the length of the array. + /// + /// This function is unsafe because it expects all elements past the `len` index + /// of `array` to be zeroed, but does not check for this internally. Use `from_parts` + /// for a safe version of this function which does zero out any indices past the + /// given length. Invalidating this assumption can notably cause `BoundedVec::eq` + /// to give incorrect results since it will check even elements past `len`. + /// + /// Example: + /// + /// ```noir + /// let vec: BoundedVec = BoundedVec::from_parts_unchecked([1, 2, 3, 0], 3); + /// assert_eq(vec.len(), 3); + /// + /// // invalid use! + /// let vec1: BoundedVec = BoundedVec::from_parts_unchecked([1, 2, 3, 1], 3); + /// let vec2: BoundedVec = BoundedVec::from_parts_unchecked([1, 2, 3, 2], 3); + /// + /// // both vecs have length 3 so we'd expect them to be equal, but this + /// // fails because elements past the length are still checked in eq + /// assert_eq(vec1, vec2); // fails + /// ``` + pub fn from_parts_unchecked(array: [T; MaxLen], len: u32) -> Self { + assert(len <= MaxLen); + BoundedVec { storage: array, len } + } } impl Eq for BoundedVec @@ -431,7 +483,11 @@ where // // We make the assumption that the user has used the proper interface for working with `BoundedVec`s // rather than directly manipulating the internal fields as this can result in an inconsistent internal state. - (self.len == other.len) & (self.storage == other.storage) + if self.len == other.len { + self.storage == other.storage + } else { + false + } } } @@ -598,4 +654,38 @@ mod bounded_vec_tests { assert(bounded_vec1 != bounded_vec2); } } + + mod from_parts { + use crate::collections::bounded_vec::BoundedVec; + + #[test] + fn from_parts() { + // docs:start:from-parts + let vec: BoundedVec = BoundedVec::from_parts([1, 2, 3, 0], 3); + assert_eq(vec.len(), 3); + + // Any elements past the given length are zeroed out, so these + // two BoundedVecs will be completely equal + let vec1: BoundedVec = BoundedVec::from_parts([1, 2, 3, 1], 3); + let vec2: BoundedVec = BoundedVec::from_parts([1, 2, 3, 2], 3); + assert_eq(vec1, vec2); + // docs:end:from-parts + } + + #[test] + fn from_parts_unchecked() { + // docs:start:from-parts-unchecked + let vec: BoundedVec = BoundedVec::from_parts_unchecked([1, 2, 3, 0], 3); + assert_eq(vec.len(), 3); + + // invalid use! + let vec1: BoundedVec = BoundedVec::from_parts_unchecked([1, 2, 3, 1], 3); + let vec2: BoundedVec = BoundedVec::from_parts_unchecked([1, 2, 3, 2], 3); + + // both vecs have length 3 so we'd expect them to be equal, but this + // fails because elements past the length are still checked in eq + assert(vec1 != vec2); + // docs:end:from-parts-unchecked + } + } } From 440d94d8149ede5f211437e9405f65b460cfcbf8 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 3 Dec 2024 12:29:28 -0600 Subject: [PATCH 2/3] fix: Don't remove necessary RC instructions in DIE pass (#6585) Co-authored-by: Ary Borenszweig Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: Maxim Vezenov --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 91 ++++++++----------- .../reference_counts/src/main.nr | 6 +- 2 files changed, 43 insertions(+), 54 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 8d3fa9cc615..9817d0a6738 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -127,8 +127,9 @@ impl Context { .push(instructions_len - instruction_index - 1); } } else { - use Instruction::*; - if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { + // We can't remove rc instructions if they're loaded from a reference + // since we'd have no way of knowing whether the reference is still used. + if Self::is_inc_dec_instruction_on_known_array(instruction, &function.dfg) { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { @@ -140,7 +141,6 @@ impl Context { rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } - self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, @@ -337,6 +337,28 @@ impl Context { inserted_check } + + /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` + /// operating on an array directly from a `Instruction::MakeArray` or an + /// intrinsic known to return a fresh array. + fn is_inc_dec_instruction_on_known_array( + instruction: &Instruction, + dfg: &DataFlowGraph, + ) -> bool { + use Instruction::*; + if let IncrementRc { value } | DecrementRc { value } = instruction { + if let Value::Instruction { instruction, .. } = &dfg[*value] { + return match &dfg[*instruction] { + MakeArray { .. } => true, + Call { func, .. } => { + matches!(&dfg[*func], Value::Intrinsic(_) | Value::ForeignFunction(_)) + } + _ => false, + }; + } + } + false + } } fn instruction_might_result_in_out_of_bounds( @@ -510,10 +532,11 @@ struct RcTracker { // If we see an inc/dec RC pair within a block we can safely remove both instructions. rcs_with_possible_pairs: HashMap>, rc_pairs_to_remove: HashSet, + // We also separately track all IncrementRc instructions and all arrays which have been mutably borrowed. // If an array has not been mutably borrowed we can then safely remove all IncrementRc instructions on that array. inc_rcs: HashMap>, - mut_borrowed_arrays: HashSet, + // The SSA often creates patterns where after simplifications we end up with repeat // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc, // and if the current instruction is also an IncrementRc on the same value we remove the current instruction. @@ -566,35 +589,10 @@ impl RcTracker { dec_rc.possibly_mutated = true; } } - - self.mut_borrowed_arrays.insert(*array); - } - Instruction::Store { value, .. } => { - // We are very conservative and say that any store of an array value means it has the potential - // to be mutated. This is done due to the tracking of mutable borrows still being per block. - let typ = function.dfg.type_of_value(*value); - if matches!(&typ, Type::Array(..) | Type::Slice(..)) { - self.mut_borrowed_arrays.insert(*value); - } } _ => {} } } - - fn get_non_mutated_arrays(&self) -> HashSet { - self.inc_rcs - .keys() - .filter_map(|value| { - if !self.mut_borrowed_arrays.contains(value) { - Some(&self.inc_rcs[value]) - } else { - None - } - }) - .flatten() - .copied() - .collect() - } } #[cfg(test)] mod test { @@ -830,32 +828,23 @@ mod test { } #[test] - fn remove_inc_rcs_that_are_never_mutably_borrowed() { + fn does_not_remove_inc_or_dec_rc_of_if_they_are_loaded_from_a_reference() { let src = " - acir(inline) fn main f0 { - b0(v0: [Field; 2]): - inc_rc v0 - inc_rc v0 - inc_rc v0 - v2 = array_get v0, index u32 0 -> Field - inc_rc v0 - return v2 + brillig(inline) fn borrow_mut f0 { + b0(v0: &mut [Field; 3]): + v1 = load v0 -> [Field; 3] + inc_rc v1 // this one shouldn't be removed + v2 = load v0 -> [Field; 3] + inc_rc v2 // this one shouldn't be removed + v3 = load v0 -> [Field; 3] + v6 = array_set v3, index u32 0, value Field 5 + store v6 at v0 + dec_rc v6 + return } "; let ssa = Ssa::from_str(src).unwrap(); - let main = ssa.main(); - - // The instruction count never includes the terminator instruction - assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); - - let expected = " - acir(inline) fn main f0 { - b0(v0: [Field; 2]): - v2 = array_get v0, index u32 0 -> Field - return v2 - } - "; let ssa = ssa.dead_instruction_elimination(); - assert_normalized_ssa_equals(ssa, expected); + assert_normalized_ssa_equals(ssa, src); } } diff --git a/test_programs/execution_success/reference_counts/src/main.nr b/test_programs/execution_success/reference_counts/src/main.nr index 7ab7de893fa..86b5c812472 100644 --- a/test_programs/execution_success/reference_counts/src/main.nr +++ b/test_programs/execution_success/reference_counts/src/main.nr @@ -1,6 +1,6 @@ fn main() { let mut array = [0, 1, 2]; - assert_refcount(array, 1); + assert_refcount(array, 2); borrow(array, std::mem::array_refcount(array)); borrow_mut(&mut array, std::mem::array_refcount(array)); @@ -13,13 +13,13 @@ fn borrow(array: [Field; 3], rc_before_call: u32) { } 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 + assert_refcount(*array, 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 + assert_refcount(array, rc_before_call + 1); array[0] = 6; println(array[0]); } From dc96de7c7a3d3c325b955edb277534653204cc5a Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 3 Dec 2024 15:02:10 -0600 Subject: [PATCH 3/3] chore: Revert "fix: Don't remove necessary RC instructions in DIE pass (#6585)" (#6693) --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 91 +++++++++++-------- .../reference_counts/src/main.nr | 6 +- 2 files changed, 54 insertions(+), 43 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 9817d0a6738..8d3fa9cc615 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -127,9 +127,8 @@ impl Context { .push(instructions_len - instruction_index - 1); } } else { - // We can't remove rc instructions if they're loaded from a reference - // since we'd have no way of knowing whether the reference is still used. - if Self::is_inc_dec_instruction_on_known_array(instruction, &function.dfg) { + use Instruction::*; + if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { @@ -141,6 +140,7 @@ impl Context { rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } + self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, @@ -337,28 +337,6 @@ impl Context { inserted_check } - - /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` - /// operating on an array directly from a `Instruction::MakeArray` or an - /// intrinsic known to return a fresh array. - fn is_inc_dec_instruction_on_known_array( - instruction: &Instruction, - dfg: &DataFlowGraph, - ) -> bool { - use Instruction::*; - if let IncrementRc { value } | DecrementRc { value } = instruction { - if let Value::Instruction { instruction, .. } = &dfg[*value] { - return match &dfg[*instruction] { - MakeArray { .. } => true, - Call { func, .. } => { - matches!(&dfg[*func], Value::Intrinsic(_) | Value::ForeignFunction(_)) - } - _ => false, - }; - } - } - false - } } fn instruction_might_result_in_out_of_bounds( @@ -532,11 +510,10 @@ struct RcTracker { // If we see an inc/dec RC pair within a block we can safely remove both instructions. rcs_with_possible_pairs: HashMap>, rc_pairs_to_remove: HashSet, - // We also separately track all IncrementRc instructions and all arrays which have been mutably borrowed. // If an array has not been mutably borrowed we can then safely remove all IncrementRc instructions on that array. inc_rcs: HashMap>, - + mut_borrowed_arrays: HashSet, // The SSA often creates patterns where after simplifications we end up with repeat // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc, // and if the current instruction is also an IncrementRc on the same value we remove the current instruction. @@ -589,10 +566,35 @@ impl RcTracker { dec_rc.possibly_mutated = true; } } + + self.mut_borrowed_arrays.insert(*array); + } + Instruction::Store { value, .. } => { + // We are very conservative and say that any store of an array value means it has the potential + // to be mutated. This is done due to the tracking of mutable borrows still being per block. + let typ = function.dfg.type_of_value(*value); + if matches!(&typ, Type::Array(..) | Type::Slice(..)) { + self.mut_borrowed_arrays.insert(*value); + } } _ => {} } } + + fn get_non_mutated_arrays(&self) -> HashSet { + self.inc_rcs + .keys() + .filter_map(|value| { + if !self.mut_borrowed_arrays.contains(value) { + Some(&self.inc_rcs[value]) + } else { + None + } + }) + .flatten() + .copied() + .collect() + } } #[cfg(test)] mod test { @@ -828,23 +830,32 @@ mod test { } #[test] - fn does_not_remove_inc_or_dec_rc_of_if_they_are_loaded_from_a_reference() { + fn remove_inc_rcs_that_are_never_mutably_borrowed() { let src = " - brillig(inline) fn borrow_mut f0 { - b0(v0: &mut [Field; 3]): - v1 = load v0 -> [Field; 3] - inc_rc v1 // this one shouldn't be removed - v2 = load v0 -> [Field; 3] - inc_rc v2 // this one shouldn't be removed - v3 = load v0 -> [Field; 3] - v6 = array_set v3, index u32 0, value Field 5 - store v6 at v0 - dec_rc v6 - return + acir(inline) fn main f0 { + b0(v0: [Field; 2]): + inc_rc v0 + inc_rc v0 + inc_rc v0 + v2 = array_get v0, index u32 0 -> Field + inc_rc v0 + return v2 } "; let ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main(); + + // The instruction count never includes the terminator instruction + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); + + let expected = " + acir(inline) fn main f0 { + b0(v0: [Field; 2]): + v2 = array_get v0, index u32 0 -> Field + return v2 + } + "; let ssa = ssa.dead_instruction_elimination(); - assert_normalized_ssa_equals(ssa, src); + assert_normalized_ssa_equals(ssa, expected); } } diff --git a/test_programs/execution_success/reference_counts/src/main.nr b/test_programs/execution_success/reference_counts/src/main.nr index 86b5c812472..7ab7de893fa 100644 --- a/test_programs/execution_success/reference_counts/src/main.nr +++ b/test_programs/execution_success/reference_counts/src/main.nr @@ -1,6 +1,6 @@ fn main() { let mut array = [0, 1, 2]; - assert_refcount(array, 2); + assert_refcount(array, 1); borrow(array, std::mem::array_refcount(array)); borrow_mut(&mut array, std::mem::array_refcount(array)); @@ -13,13 +13,13 @@ fn borrow(array: [Field; 3], rc_before_call: u32) { } fn borrow_mut(array: &mut [Field; 3], rc_before_call: u32) { - assert_refcount(*array, rc_before_call + 1); + 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 + 1); + assert_refcount(array, rc_before_call + 0); // Issue! This should be rc_before_call + 1 array[0] = 6; println(array[0]); }