From f988d020e43cdf36a38613f2052d4518de39193a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 8 Mar 2024 20:03:50 +0000 Subject: [PATCH] fix(ssa): Handle mergers of slices returned from calls (#4496) # Description ## Problem\* Resolves #4418 ## Summary\* We need to add handling for slices returned from `ToRadix` and `ToBits`. ~~This PR does two fixes.~~ EDIT: After moving to the iterative flattening appraoch w are now accurately tracking slice capacities when they are the parameters of blocks. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../ssa/opt/flatten_cfg/capacity_tracker.rs | 32 ++++++++++++++++--- .../execution_success/slices/src/main.nr | 26 +++++++++++++++ 2 files changed, 53 insertions(+), 5 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..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 @@ -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,27 @@ 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,20 +92,35 @@ 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); + } _ => {} } } diff --git a/test_programs/execution_success/slices/src/main.nr b/test_programs/execution_success/slices/src/main.nr index 6823bf05d96..e44edf872a4 100644 --- a/test_programs/execution_success/slices/src/main.nr +++ b/test_programs/execution_success/slices/src/main.nr @@ -51,6 +51,8 @@ fn main(x: Field, y: pub Field) { regression_merge_slices(x, y); regression_2370(); + regression_4418(x); + regression_slice_call_result(x, y); regression_4506(); } // Ensure that slices of struct/tuple values work. @@ -300,6 +302,30 @@ fn regression_2370() { slice = [1, 2, 3]; } +fn regression_4418(x: Field) { + let mut crash = x.to_be_bytes(32); + + 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); +} + fn regression_4506() { let slice: [Field] = [1, 2, 3]; assert(slice == slice);