From 143e75b1cbe358da1bfc652e0be2b40b6419146d Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 6 Mar 2024 20:28:46 +0000 Subject: [PATCH 1/4] handle intrinsics that return slices --- .../ssa/opt/flatten_cfg/capacity_tracker.rs | 31 +++++- .../execution_success/slices/src/main.nr | 103 ++++++++++-------- 2 files changed, 80 insertions(+), 54 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs index 7cd0fe3084e..5241517adf8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs @@ -5,6 +5,7 @@ use crate::ssa::ir::{ value::{Value, ValueId}, }; +use acvm::FieldElement; use fxhash::FxHashMap as HashMap; pub(crate) struct SliceCapacityTracker<'a> { @@ -62,21 +63,26 @@ impl<'a> SliceCapacityTracker<'a> { | Intrinsic::SlicePushFront | Intrinsic::SlicePopBack | Intrinsic::SliceInsert - | Intrinsic::SliceRemove => (1, 1), + | Intrinsic::SliceRemove => (Some(1), 1), // `pop_front` returns the popped element, and then the respective slice. // This means in the case of a slice with structs, the result index of the popped slice // will change depending on the number of elements in the struct. // For example, a slice with four elements will look as such in SSA: // v3, v4, v5, v6, v7, v8 = call slice_pop_front(v1, v2) // where v7 is the slice length and v8 is the popped slice itself. - Intrinsic::SlicePopFront => (1, results.len() - 1), + Intrinsic::SlicePopFront => (Some(1), results.len() - 1), + // The slice capacity of these intrinsics is not determined by the arguments of the function. + Intrinsic::ToBits(_) | Intrinsic::ToRadix(_) => (None, 1), _ => return, }; - let slice_contents = arguments[argument_index]; + let result_slice = results[result_index]; match intrinsic { Intrinsic::SlicePushBack | Intrinsic::SlicePushFront | Intrinsic::SliceInsert => { + let argument_index = argument_index.expect("ICE: Should have an argument index for slice intrinsics"); + let slice_contents = arguments[argument_index]; + for arg in &arguments[(argument_index + 1)..] { let element_typ = self.dfg.type_of_value(*arg); if element_typ.contains_slice_element() { @@ -85,23 +91,36 @@ impl<'a> SliceCapacityTracker<'a> { } if let Some(contents_capacity) = slice_sizes.get(&slice_contents) { let new_capacity = *contents_capacity + 1; - slice_sizes.insert(results[result_index], new_capacity); + slice_sizes.insert(result_slice, new_capacity); } } Intrinsic::SlicePopBack | Intrinsic::SliceRemove | Intrinsic::SlicePopFront => { + let argument_index = argument_index.expect("ICE: Should have an argument index for slice intrinsics"); + let slice_contents = arguments[argument_index]; + // We do not decrement the size on intrinsics that could remove values from a slice. // This is because we could potentially go back to the smaller slice and not fill in dummies. // This pass should be tracking the potential max that a slice ***could be*** if let Some(contents_capacity) = slice_sizes.get(&slice_contents) { let new_capacity = *contents_capacity - 1; - slice_sizes.insert(results[result_index], new_capacity); + slice_sizes.insert(result_slice, new_capacity); } } + Intrinsic::ToBits(_) => { + // Compiler sanity check + assert!(matches!(self.dfg.type_of_value(result_slice), Type::Slice(_))); + slice_sizes.insert(result_slice, FieldElement::max_num_bits() as usize); + } + Intrinsic::ToRadix(_) => { + // Compiler sanity check + assert!(matches!(self.dfg.type_of_value(result_slice), Type::Slice(_))); + slice_sizes.insert(result_slice, FieldElement::max_num_bytes() as usize); + } _ => {} } - } + } } Instruction::Store { address, value } => { let value_typ = self.dfg.type_of_value(*value); diff --git a/test_programs/execution_success/slices/src/main.nr b/test_programs/execution_success/slices/src/main.nr index eca42a660c4..e0973c66ee9 100644 --- a/test_programs/execution_success/slices/src/main.nr +++ b/test_programs/execution_success/slices/src/main.nr @@ -2,54 +2,55 @@ 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); - regression_2370(); + // 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); + // regression_2370(); + regression_4418(x); } // Ensure that slices of struct/tuple values work. fn regression_2083() { @@ -297,3 +298,9 @@ fn regression_2370() { let mut slice = []; slice = [1, 2, 3]; } + +fn regression_4418(x: Field) { + let mut crash = x.to_be_bytes(32); + + if (x != 0) {} +} From 31f3ec1bae98de5a5944c19b6b56274ee4ad116d Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 6 Mar 2024 21:36:06 +0000 Subject: [PATCH 2/4] handle radix intrinsics for slice mergers and returning from calls --- .../src/ssa/opt/flatten_cfg.rs | 18 ++- .../execution_success/slices/src/main.nr | 118 ++++++++++-------- 2 files changed, 84 insertions(+), 52 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 943a57c1bc0..fbe95be6dc8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -477,7 +477,7 @@ impl<'f> Context<'f> { let else_args = self.inserter.function.dfg[else_branch.last_block].terminator_arguments().to_vec(); - let params = self.inserter.function.dfg.block_parameters(destination); + let params = self.inserter.function.dfg.block_parameters(destination).to_vec(); assert_eq!(params.len(), then_args.len()); assert_eq!(params.len(), else_args.len()); @@ -487,8 +487,8 @@ impl<'f> Context<'f> { let block = self.inserter.function.entry_block(); - // Make sure we have tracked the slice capacities of any block arguments let capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); + // Make sure we have tracked the slice capacities of any block arguments for (then_arg, else_arg) in args.iter() { capacity_tracker.compute_slice_capacity(*then_arg, &mut self.slice_sizes); capacity_tracker.compute_slice_capacity(*else_arg, &mut self.slice_sizes); @@ -507,6 +507,18 @@ impl<'f> Context<'f> { ) }); + // We need a fresh `SliceCapacityTracker` as we borrow a mutable `DataFlowGraph` to perform a value merger + let capacity_tracker = SliceCapacityTracker::new(&self.inserter.function.dfg); + // For not yet inlined blocks instructions will still use the original `ValueId`. + // If we do not update the old parameter we will hit a panic when trying to process an instruction + // that uses the old parameter. + for (old_param, new_param) in params.iter().zip(args.iter()) { + capacity_tracker.compute_slice_capacity(*new_param, &mut self.slice_sizes); + if let Some(new_capacity) = self.slice_sizes.get(new_param) { + self.slice_sizes.insert(*old_param, *new_capacity); + } + } + self.merge_stores(then_branch, else_branch); // insert merge instruction @@ -615,7 +627,7 @@ impl<'f> Context<'f> { /// Expects that the `arguments` given are already translated via self.inserter.resolve. /// If they are not, it is possible some values which no longer exist, such as block /// parameters, will be kept in the program. - fn inline_block(&mut self, destination: BasicBlockId, arguments: &[ValueId]) -> BasicBlockId { + fn inline_block(&mut self, destination: BasicBlockId, arguments: &[ValueId]) -> BasicBlockId { self.inserter.remember_block_params(destination, arguments); // If this is not a separate variable, clippy gets confused and says the to_vec is diff --git a/test_programs/execution_success/slices/src/main.nr b/test_programs/execution_success/slices/src/main.nr index e0973c66ee9..65d2c502c48 100644 --- a/test_programs/execution_success/slices/src/main.nr +++ b/test_programs/execution_success/slices/src/main.nr @@ -2,55 +2,57 @@ 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); - // regression_2370(); + 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); + regression_2370(); + regression_4418(x); + regression_slice_call_result(x, y); } // Ensure that slices of struct/tuple values work. fn regression_2083() { @@ -302,5 +304,23 @@ fn regression_2370() { fn regression_4418(x: Field) { let mut crash = x.to_be_bytes(32); - if (x != 0) {} + if x != 0 { + crash[0] = 10; + } +} + +fn regression_slice_call_result(x: Field, y: Field) { + let mut slice = merge_slices_return(x, y); + if x != 0 { + slice = slice.push_back(5); + slice = slice.push_back(10); + } else { + slice = slice.push_back(5); + } + assert(slice.len() == 5); + assert(slice[0] == 0); + assert(slice[1] == 0); + assert(slice[2] == 10); + assert(slice[3] == 5); + assert(slice[4] == 10); } From 3b403e83f45df0cf460c03ef7a632214ab255600 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 6 Mar 2024 21:39:14 +0000 Subject: [PATCH 3/4] cargo fmt --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 2 +- .../src/ssa/opt/flatten_cfg/capacity_tracker.rs | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index fbe95be6dc8..b202a9f8e85 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -627,7 +627,7 @@ impl<'f> Context<'f> { /// Expects that the `arguments` given are already translated via self.inserter.resolve. /// If they are not, it is possible some values which no longer exist, such as block /// parameters, will be kept in the program. - fn inline_block(&mut self, destination: BasicBlockId, arguments: &[ValueId]) -> BasicBlockId { + fn inline_block(&mut self, destination: BasicBlockId, arguments: &[ValueId]) -> BasicBlockId { self.inserter.remember_block_params(destination, arguments); // If this is not a separate variable, clippy gets confused and says the to_vec is diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs index 5241517adf8..bdfc04f0bbe 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/capacity_tracker.rs @@ -80,7 +80,8 @@ impl<'a> SliceCapacityTracker<'a> { Intrinsic::SlicePushBack | Intrinsic::SlicePushFront | Intrinsic::SliceInsert => { - let argument_index = argument_index.expect("ICE: Should have an argument index for slice intrinsics"); + let argument_index = argument_index + .expect("ICE: Should have an argument index for slice intrinsics"); let slice_contents = arguments[argument_index]; for arg in &arguments[(argument_index + 1)..] { @@ -97,7 +98,8 @@ impl<'a> SliceCapacityTracker<'a> { Intrinsic::SlicePopBack | Intrinsic::SliceRemove | Intrinsic::SlicePopFront => { - let argument_index = argument_index.expect("ICE: Should have an argument index for slice intrinsics"); + let argument_index = argument_index + .expect("ICE: Should have an argument index for slice intrinsics"); let slice_contents = arguments[argument_index]; // We do not decrement the size on intrinsics that could remove values from a slice. @@ -116,11 +118,12 @@ impl<'a> SliceCapacityTracker<'a> { Intrinsic::ToRadix(_) => { // Compiler sanity check assert!(matches!(self.dfg.type_of_value(result_slice), Type::Slice(_))); - slice_sizes.insert(result_slice, FieldElement::max_num_bytes() as usize); + slice_sizes + .insert(result_slice, FieldElement::max_num_bytes() as usize); } _ => {} } - } + } } Instruction::Store { address, value } => { let value_typ = self.dfg.type_of_value(*value); From fbafab4ab5f0d4c252d2ed9392ad8627de5b421c Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 7 Mar 2024 04:23:11 +0000 Subject: [PATCH 4/4] nargo fmt --- test_programs/execution_success/slices/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_programs/execution_success/slices/src/main.nr b/test_programs/execution_success/slices/src/main.nr index 65d2c502c48..3f9b48ce407 100644 --- a/test_programs/execution_success/slices/src/main.nr +++ b/test_programs/execution_success/slices/src/main.nr @@ -304,7 +304,7 @@ fn regression_2370() { fn regression_4418(x: Field) { let mut crash = x.to_be_bytes(32); - if x != 0 { + if x != 0 { crash[0] = 10; } }