From 5567d4678da6f354ee5c46a93e0dd2141e32cc22 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Fri, 8 Sep 2023 19:11:00 +0000 Subject: [PATCH 01/16] working merge_slices_mutate_two_ifs, push back between mergers is broken though --- .../execution_success/slices/src/main.nr | 207 +++++++++----- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 17 +- .../src/ssa/ir/instruction/call.rs | 26 +- .../src/ssa/opt/flatten_cfg.rs | 262 +++++++++++++++++- crates/noirc_evaluator/src/ssa/opt/mod.rs | 2 +- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- 6 files changed, 431 insertions(+), 85 deletions(-) diff --git a/crates/nargo_cli/tests/execution_success/slices/src/main.nr b/crates/nargo_cli/tests/execution_success/slices/src/main.nr index 8fbe14bfea..773a1ae4b6 100644 --- a/crates/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/slices/src/main.nr @@ -1,52 +1,87 @@ 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); +// fn main(x : Field, y : pub Field) { +// let slice = merge_slices_mutate_two_ifs_test(x, y); +// let len = slice.len(); +// dep::std::println(f"len: {len}"); +// dep::std::println(slice[0]); +// dep::std::println(slice[1]); +// dep::std::println(slice[2]); +// dep::std::println(slice[3]); +// dep::std::println(slice[4]); +// dep::std::println(slice[5]); +// } + +// fn merge_slices_mutate_two_ifs_test(x: Field, y: Field) -> [Field] { +// let mut slice = [0; 2]; +// if x != y { +// slice = slice.push_back(y); +// slice = slice.push_back(x); +// } else { +// slice = slice.push_back(x); +// } +// if x == 20 { +// slice = slice.push_back(20); +// // slice = slice.push_back(40); +// } +// // else { +// // slice = slice.push_back(15); +// // } +// // TODO(#2599): Breaks if the push back happens without the else case +// // slice = slice.push_back(15); +// slice = slice.push_back(30); + +// slice +// } - regression_2083(); +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); + + // TODO: need to implement dynamic arrays for + // regression_2083(); // The parameters to this function must come from witness values (inputs to main) regression_merge_slices(x, y); } @@ -87,33 +122,42 @@ fn regression_2083() { // The parameters to this function must come from witness values (inputs to main) fn regression_merge_slices(x: Field, y: Field) { merge_slices_if(x, y); - merge_slices_else(x); + // merge_slices_else(x); } fn merge_slices_if(x: Field, y: Field) { - let slice = merge_slices_return(x, y); - assert(slice[2] == 10); - assert(slice.len() == 3); - - let slice = merge_slices_mutate(x, y); - assert(slice[3] == 5); - assert(slice.len() == 4); - - let slice = merge_slices_mutate_in_loop(x, y); - assert(slice[6] == 4); - assert(slice.len() == 7); - - let slice = merge_slices_mutate_two_ifs(x, y); - assert(slice.len() == 6); - assert(slice[3] == 5); - assert(slice[4] == 15); - assert(slice[5] == 30); - - let slice = merge_slices_mutate_between_ifs(x, y); - assert(slice.len() == 6); - assert(slice[3] == 5); - assert(slice[4] == 30); - assert(slice[5] == 15); + // let slice = merge_slices_return(x, y); + // assert(slice[2] == 10); + // assert(slice.len() == 3); + + // let slice = merge_slices_mutate(x, y); + // assert(slice[3] == 5); + // assert(slice.len() == 4); + + // let slice = merge_slices_mutate_in_loop(x, y); + // assert(slice[6] == 4); + // assert(slice.len() == 7); + + // let slice = merge_slices_mutate_two_ifs(x, y); + // let len = slice.len(); + // dep::std::println(f"len: {len}"); + // assert(slice.len() == 6); + // assert(slice[3] == 5); + // assert(slice[4] == 15); + // assert(slice[5] == 30); + + // let slice = merge_slices_mutate_between_ifs(x, y); + // let len = slice.len(); + // dep::std::println(f"len: {len}"); + // for i in 0..6 { + // let elem = slice[i]; + // dep::std::println(f"slice[{i}]: {elem}"); + // } + // assert(slice.len() == 6); + + // assert(slice[3] == 5); + // assert(slice[4] == 30); + // assert(slice[5] == 15); } fn merge_slices_else(x: Field) { @@ -178,18 +222,23 @@ fn merge_slices_mutate_two_ifs(x: Field, y: Field) -> [Field] { } else { slice = slice.push_back(x); } + let len = slice.len(); + dep::std::println(f"len: {len}"); if x == 20 { slice = slice.push_back(20); - } else { - slice = slice.push_back(15); - } + } + // else { + // slice = slice.push_back(15); + // } // TODO(#2599): Breaks if the push back happens without the else case - // slice = slice.push_back(15); + // Working now + slice = slice.push_back(15); slice = slice.push_back(30); slice } +// Still broken fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { let mut slice = [0; 2]; if x != y { @@ -200,12 +249,18 @@ fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { } slice = slice.push_back(30); - + let len = slice.len(); + dep::std::println(f"len: {len}"); if x == 20 { slice = slice.push_back(20); - } else { - slice = slice.push_back(15); } + // else { + // slice = slice.push_back(15); + // } + + let len = slice.len(); + dep::std::println(f"len: {len}"); + // TODO(#2599): Breaks if the push back happens without the else case // slice = slice.push_back(15); diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 092fb1e966..d9a7417711 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -659,6 +659,7 @@ impl Context { Type::Slice(typ) => { if typ.len() != 1 { // TODO(#2461) + dbg!(typ); unimplemented!( "Non-const array indices is not implemented for non-homogenous array" ); @@ -708,9 +709,19 @@ impl Context { .expect("ICE: array set on slice must have a length associated with the call"); let length_acir_var = self.convert_value(length, dfg).into_var()?; let len = self.acir_context.var_to_expression(length_acir_var)?.to_const(); - let len = len - .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); - len.to_u128() as usize + if let Some(len) = len { + len.to_u128() as usize + } else { + if let Some((array, _)) = dfg.get_array_constant(array) { + // dbg!(array.clone()); + array.len() + } else { + panic!("ICE: slice length should be fully tracked and constant by ACIR gen"); + } + } + // let len = len + // .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); + // len.to_u128() as usize } _ => unreachable!("ICE - expected an array"), }; diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs index 42d0aa0a4e..98ab900c79 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -12,6 +12,7 @@ use crate::ssa::ir::{ types::Type, value::{Value, ValueId}, }; +use crate::ssa::opt::flatten_cfg::merge_values_pub; use super::{Binary, BinaryOp, Endian, Instruction, SimplifyResult}; @@ -91,13 +92,34 @@ pub(super) fn simplify_call( Intrinsic::SlicePushBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((mut slice, element_type)) = slice { + let length = dfg.get_numeric_constant(arguments[0]); + dbg!(length); + dbg!(slice.len()); + + // The capacity must be an integer so that we can compare it against the slice length which is represented as a field + let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); + + let len_equals_capacity_instr = Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, rhs: capacity }); + let call_stack = dfg.get_value_call_stack(arguments[0]); + let len_equals_capacity = dfg.insert_instruction_and_results(len_equals_capacity_instr, block, None, call_stack.clone()).first(); + let len_not_equals_capacity_instr = Instruction::Not(len_equals_capacity); + let len_not_equals_capacity = dfg.insert_instruction_and_results(len_not_equals_capacity_instr, block, None, call_stack.clone()).first(); + + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + for elem in &arguments[2..] { slice.push_back(*elem); } + dbg!(slice.len()); + let new_slice = dfg.make_array(slice, element_type); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + // let slice_length_minus_one = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + let set_last_slice_value_instr = Instruction::ArraySet { array: new_slice, index: arguments[0], value: arguments[2], length: Some(new_slice_length) }; + let set_last_slice_value = dfg.insert_instruction_and_results(set_last_slice_value_instr, block, None, call_stack.clone()).first(); - let new_slice = dfg.make_array(slice, element_type); + let new_slice = merge_values_pub(len_not_equals_capacity, len_equals_capacity, set_last_slice_value, new_slice, block, dfg); + // let new_slice_val = dfg.get_array_constant(new_slice); + // dbg!(new_slice_val); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) } else { SimplifyResult::None diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index d57c2cc793..47bc80e52c 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -141,7 +141,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, - dfg::{CallStack, InsertInstructionResult}, + dfg::{CallStack, InsertInstructionResult, DataFlowGraph}, function::Function, function_inserter::FunctionInserter, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, @@ -418,7 +418,8 @@ impl<'f> Context<'f> { ) -> ValueId { match self.inserter.function.dfg.type_of_value(then_value) { Type::Numeric(_) => { - self.merge_numeric_values(then_condition, else_condition, then_value, else_value) + // self.merge_numeric_values(then_condition, else_condition, then_value, else_value) + merge_numeric_values_pub(then_condition, else_condition, then_value, else_value, self.inserter.function.entry_block(), &mut self.inserter.function.dfg) } typ @ Type::Array(_, _) => { self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) @@ -447,7 +448,14 @@ impl<'f> Context<'f> { }; let then_len = self.get_slice_length(then_value_id); + dbg!(then_len); let else_len = self.get_slice_length(else_value_id); + dbg!(else_len); + if else_len == 4 { + dbg!(&self.inserter.function.dfg.resolve(then_value_id)); + dbg!(&self.inserter.function.dfg.resolve(else_value_id)); + dbg!(&self.inserter.function.dfg[else_value_id]); + } let len = then_len.max(else_len); @@ -487,6 +495,7 @@ impl<'f> Context<'f> { fn get_slice_length(&mut self, value_id: ValueId) -> usize { let value = &self.inserter.function.dfg[value_id]; + // dbg!(value); match value { Value::Array { array, .. } => array.len(), Value::Instruction { instruction: instruction_id, .. } => { @@ -501,6 +510,10 @@ impl<'f> Context<'f> { .get(address) .expect("ICE: load in merger should have store from outer block") }; + // let context_store = + // self.outer_block_stores + // .get(address) + // .expect("ICE: load in merger should have store from outer block"); self.get_slice_length(context_store.new_value) } @@ -879,6 +892,251 @@ impl<'f> Context<'f> { } } + +pub(crate) fn merge_values_pub( + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + block: BasicBlockId, + dfg: &mut DataFlowGraph, +) -> ValueId { + // let block = self.inserter.function.entry_block(); + match dfg.type_of_value(then_value) { + Type::Numeric(_) => { + // self.merge_numeric_values(then_condition, else_condition, then_value, else_value) + merge_numeric_values_pub(then_condition, else_condition, then_value, else_value, block, dfg) + } + typ @ Type::Array(_, _) => { + // self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) + merge_array_values_pub(typ, then_condition, else_condition, then_value, else_value, block, dfg) + } + typ @ Type::Slice(_) => { + // self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) + // panic!("ahh"); + merge_slice_values_pub(typ, then_condition, else_condition, then_value, else_value, block, dfg) + } + Type::Reference => panic!("Cannot return references from an if expression"), + Type::Function => panic!("Cannot return functions from an if expression"), + } +} + +pub(crate) fn merge_slice_values_pub( + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value_id: ValueId, + else_value_id: ValueId, + block: BasicBlockId, + dfg: &mut DataFlowGraph, +) -> ValueId { + let mut merged = im::Vector::new(); + + let element_types = match &typ { + Type::Slice(elements) => elements, + _ => panic!("Expected slice type"), + }; + + let then_len = get_slice_length_pub(then_value_id, dfg); + dbg!(then_len); + let else_len = get_slice_length_pub(else_value_id, dfg); + dbg!(else_len); + let len = then_len.max(else_len); + + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index_value = ((i * element_types.len() + element_index) as u128).into(); + let index = dfg.make_constant(index_value, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars, len| { + // The smaller slice is filled with placeholder data. Codegen for slice accesses must + // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. + if (len - 1) < index_value.to_u128() as usize { + let zero = FieldElement::zero(); + dfg.make_constant(zero, Type::field()) + } else { + let get = Instruction::ArrayGet { array, index }; + dfg.insert_instruction_and_results( + get, + block, + typevars, + CallStack::new(), + ).first() + } + }; + + let then_element = get_element(then_value_id, typevars.clone(), then_len); + let else_element = get_element(else_value_id, typevars, else_len); + + merged.push_back(merge_values_pub( + then_condition, + else_condition, + then_element, + else_element, + block, + dfg + )) + } + } + + dfg.make_array(merged, typ) +} + +fn get_slice_length_pub(value_id: ValueId, dfg: &DataFlowGraph) -> usize { + let value = &dfg[value_id]; + match value { + Value::Array { array, .. } => array.len(), + Value::Instruction { instruction: instruction_id, .. } => { + let instruction = &dfg[*instruction_id]; + match instruction { + Instruction::ArraySet { array, .. } => get_slice_length_pub(*array, dfg), + Instruction::Load { address } => { + panic!("should not have a load"); + // let context_store = if let Some(store) = self.store_values.get(address) { + // store + // } else { + // self.outer_block_stores + // .get(address) + // .expect("ICE: load in merger should have store from outer block") + // }; + + // self.get_slice_length(context_store.new_value) + } + Instruction::Call { func, arguments } => { + let func = &dfg[*func]; + let slice_contents = arguments[1]; + match func { + Value::Intrinsic(intrinsic) => match intrinsic { + Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SliceInsert => { + get_slice_length_pub(slice_contents, dfg) + 1 + } + Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceRemove => { + get_slice_length_pub(slice_contents, dfg) - 1 + } + _ => { + unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") + } + }, + _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), + } + } + _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), + } + } + _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), + } +} + +/// Given an if expression that returns an array: `if c { array1 } else { array2 }`, +/// this function will recursively merge array1 and array2 into a single resulting array +/// by creating a new array containing the result of self.merge_values for each element. +pub(crate) fn merge_array_values_pub( + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + block: BasicBlockId, + dfg: &mut DataFlowGraph, +) -> ValueId { + let mut merged = im::Vector::new(); + + let (element_types, len) = match &typ { + Type::Array(elements, len) => (elements, *len), + _ => panic!("Expected array type"), + }; + + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index = ((i * element_types.len() + element_index) as u128).into(); + let index = dfg.make_constant(index, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars| { + let get = Instruction::ArrayGet { array, index }; + dfg.insert_instruction_and_results( + get, + block, + typevars, + CallStack::new(), + ).first() + }; + + let then_element = get_element(then_value, typevars.clone()); + let else_element = get_element(else_value, typevars); + + merged.push_back(merge_values_pub( + then_condition, + else_condition, + then_element, + else_element, + block, + dfg + )) + } + } + + dfg.make_array(merged, typ) +} + +/// Merge two numeric values a and b from separate basic blocks to a single value. This +/// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. +pub(crate) fn merge_numeric_values_pub( + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + block: BasicBlockId, + dfg: &mut DataFlowGraph, +) -> ValueId { + let then_type = dfg.type_of_value(then_value); + let else_type = dfg.type_of_value(else_value); + assert_eq!( + then_type, else_type, + "Expected values merged to be of the same type but found {then_type} and {else_type}" + ); + + let then_call_stack = dfg.get_value_call_stack(then_value); + let else_call_stack = dfg.get_value_call_stack(else_value); + + let call_stack = if then_call_stack.is_empty() { + else_call_stack.clone() + } else { + then_call_stack.clone() + }; + + // We must cast the bool conditions to the actual numeric type used by each value. + let then_condition = dfg + .insert_instruction_and_results(Instruction::Cast(then_condition, then_type), block, None, call_stack.clone()) + .first(); + let else_condition = dfg + .insert_instruction_and_results(Instruction::Cast(else_condition, else_type), block, None, call_stack.clone()) + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); + let then_value = dfg + .insert_instruction_and_results(mul, block, None, call_stack.clone()) + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); + let else_value = dfg + .insert_instruction_and_results(mul, block, None, call_stack.clone()) + .first(); + + let add = Instruction::binary(BinaryOp::Add, then_value, else_value); + dfg + .insert_instruction_and_results(add, block, None, call_stack) + .first() +} + + #[cfg(test)] mod test { use std::rc::Rc; diff --git a/crates/noirc_evaluator/src/ssa/opt/mod.rs b/crates/noirc_evaluator/src/ssa/opt/mod.rs index 9b9350118a..4d003c0594 100644 --- a/crates/noirc_evaluator/src/ssa/opt/mod.rs +++ b/crates/noirc_evaluator/src/ssa/opt/mod.rs @@ -8,7 +8,7 @@ mod assert_constant; mod constant_folding; mod defunctionalize; mod die; -mod flatten_cfg; +pub(crate) mod flatten_cfg; mod inlining; mod mem2reg; mod simplify_cfg; diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 652869bdc9..51477a4574 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -346,7 +346,7 @@ impl<'a> FunctionContext<'a> { let is_offset_out_of_bounds = self.builder.insert_binary(index, BinaryOp::Lt, array_len); let true_const = self.builder.numeric_constant(true, Type::bool()); - self.builder.insert_constrain(is_offset_out_of_bounds, true_const, None); + self.builder.insert_constrain(is_offset_out_of_bounds, true_const, Some("Index out of bounds".to_owned())); } fn codegen_cast(&mut self, cast: &ast::Cast) -> Values { From 620c84eca42b6ca1ee983e490a7c85e04706c009 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 11 Sep 2023 19:59:51 +0000 Subject: [PATCH 02/16] merge_slices_mutate_between_ifs and merge_slices_mutate_two_ifs working with SlicePushBack --- crates/nargo/src/ops/test.rs | 3 +- .../execution_success/slices/src/main.nr | 122 +++----- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 4 +- .../src/ssa/ir/instruction/call.rs | 71 ++++- .../src/ssa/opt/flatten_cfg.rs | 274 +++++++++++++----- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 6 +- .../src/hir/def_collector/dc_mod.rs | 4 +- 7 files changed, 319 insertions(+), 165 deletions(-) diff --git a/crates/nargo/src/ops/test.rs b/crates/nargo/src/ops/test.rs index eb19448b5c..8c21eb047b 100644 --- a/crates/nargo/src/ops/test.rs +++ b/crates/nargo/src/ops/test.rs @@ -106,8 +106,7 @@ fn check_expected_failure_message(test_function: TestFunction, got_error: &str) None => return TestStatus::Pass, }; - let expected_failure_message_matches = - got_error == expected_failure_message; + let expected_failure_message_matches = got_error == expected_failure_message; if expected_failure_message_matches { return TestStatus::Pass; } diff --git a/crates/nargo_cli/tests/execution_success/slices/src/main.nr b/crates/nargo_cli/tests/execution_success/slices/src/main.nr index 773a1ae4b6..8c22e0e90c 100644 --- a/crates/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/slices/src/main.nr @@ -1,40 +1,6 @@ use dep::std::slice; use dep::std; -// fn main(x : Field, y : pub Field) { -// let slice = merge_slices_mutate_two_ifs_test(x, y); -// let len = slice.len(); -// dep::std::println(f"len: {len}"); -// dep::std::println(slice[0]); -// dep::std::println(slice[1]); -// dep::std::println(slice[2]); -// dep::std::println(slice[3]); -// dep::std::println(slice[4]); -// dep::std::println(slice[5]); -// } - -// fn merge_slices_mutate_two_ifs_test(x: Field, y: Field) -> [Field] { -// let mut slice = [0; 2]; -// if x != y { -// slice = slice.push_back(y); -// slice = slice.push_back(x); -// } else { -// slice = slice.push_back(x); -// } -// if x == 20 { -// slice = slice.push_back(20); -// // slice = slice.push_back(40); -// } -// // else { -// // slice = slice.push_back(15); -// // } -// // TODO(#2599): Breaks if the push back happens without the else case -// // slice = slice.push_back(15); -// slice = slice.push_back(30); - -// slice -// } - fn main(x : Field, y : pub Field) { // let mut slice = [0; 2]; // assert(slice[0] == 0); @@ -122,42 +88,44 @@ fn regression_2083() { // The parameters to this function must come from witness values (inputs to main) fn regression_merge_slices(x: Field, y: Field) { merge_slices_if(x, y); - // merge_slices_else(x); + merge_slices_else(x); } fn merge_slices_if(x: Field, y: Field) { - // let slice = merge_slices_return(x, y); - // assert(slice[2] == 10); - // assert(slice.len() == 3); - - // let slice = merge_slices_mutate(x, y); - // assert(slice[3] == 5); - // assert(slice.len() == 4); - - // let slice = merge_slices_mutate_in_loop(x, y); - // assert(slice[6] == 4); - // assert(slice.len() == 7); + let slice = merge_slices_return(x, y); + assert(slice[2] == 10); + assert(slice.len() == 3); - // let slice = merge_slices_mutate_two_ifs(x, y); - // let len = slice.len(); - // dep::std::println(f"len: {len}"); - // assert(slice.len() == 6); - // assert(slice[3] == 5); - // assert(slice[4] == 15); - // assert(slice[5] == 30); + let slice = merge_slices_mutate(x, y); + assert(slice[3] == 5); + assert(slice.len() == 4); - // let slice = merge_slices_mutate_between_ifs(x, y); - // let len = slice.len(); - // dep::std::println(f"len: {len}"); - // for i in 0..6 { - // let elem = slice[i]; - // dep::std::println(f"slice[{i}]: {elem}"); - // } - // assert(slice.len() == 6); + let slice = merge_slices_mutate_in_loop(x, y); + assert(slice[6] == 4); + assert(slice.len() == 7); - // assert(slice[3] == 5); - // assert(slice[4] == 30); - // assert(slice[5] == 15); + let slice = merge_slices_mutate_two_ifs(x, y); + let len = slice.len(); + dep::std::println(f"len: {len}"); + assert(slice.len() == 6); + assert(slice[3] == 5); + assert(slice[4] == 15); + assert(slice[5] == 30); + + let slice = merge_slices_mutate_between_ifs(x, y); + assert(slice.len() == 8); + assert(slice[3] == 5); + assert(slice[4] == 30); + assert(slice[5] == 15); + assert(slice[6] == 50); + assert(slice[7] == 60); + let len = slice.len(); + dep::std::println(f"len: {len}"); + for i in 0..8 { + let elem = slice[i]; + dep::std::println(f"slice[{i}]: {elem}"); + } + } fn merge_slices_else(x: Field) { @@ -222,16 +190,11 @@ fn merge_slices_mutate_two_ifs(x: Field, y: Field) -> [Field] { } else { slice = slice.push_back(x); } - let len = slice.len(); - dep::std::println(f"len: {len}"); + if x == 20 { slice = slice.push_back(20); } - // else { - // slice = slice.push_back(15); - // } - // TODO(#2599): Breaks if the push back happens without the else case - // Working now + slice = slice.push_back(15); slice = slice.push_back(30); @@ -249,20 +212,19 @@ fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { } slice = slice.push_back(30); - let len = slice.len(); - dep::std::println(f"len: {len}"); + // let len = slice.len(); + // dep::std::println(f"len: {len}"); if x == 20 { slice = slice.push_back(20); } - // else { - // slice = slice.push_back(15); - // } - let len = slice.len(); - dep::std::println(f"len: {len}"); + slice = slice.push_back(15); + + if x != 20 { + slice = slice.push_back(50); + } - // TODO(#2599): Breaks if the push back happens without the else case - // slice = slice.push_back(15); + slice = slice.push_back(60); slice } diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index d9a7417711..2b8c90601c 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -716,7 +716,9 @@ impl Context { // dbg!(array.clone()); array.len() } else { - panic!("ICE: slice length should be fully tracked and constant by ACIR gen"); + panic!( + "ICE: slice length should be fully tracked and constant by ACIR gen" + ); } } // let len = len diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs index 98ab900c79..4cdef219d5 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -92,35 +92,84 @@ pub(super) fn simplify_call( Intrinsic::SlicePushBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((mut slice, element_type)) = slice { - let length = dfg.get_numeric_constant(arguments[0]); - dbg!(length); - dbg!(slice.len()); + // let length = dfg.get_numeric_constant(arguments[0]); + // dbg!(length); + // dbg!(slice.len()); // The capacity must be an integer so that we can compare it against the slice length which is represented as a field let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); - let len_equals_capacity_instr = Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, rhs: capacity }); + let len_equals_capacity_instr = Instruction::Binary(Binary { + lhs: arguments[0], + operator: BinaryOp::Eq, + rhs: capacity, + }); let call_stack = dfg.get_value_call_stack(arguments[0]); - let len_equals_capacity = dfg.insert_instruction_and_results(len_equals_capacity_instr, block, None, call_stack.clone()).first(); + let len_equals_capacity = dfg + .insert_instruction_and_results( + len_equals_capacity_instr, + block, + None, + call_stack.clone(), + ) + .first(); let len_not_equals_capacity_instr = Instruction::Not(len_equals_capacity); - let len_not_equals_capacity = dfg.insert_instruction_and_results(len_not_equals_capacity_instr, block, None, call_stack.clone()).first(); + let len_not_equals_capacity = dfg + .insert_instruction_and_results( + len_not_equals_capacity_instr, + block, + None, + call_stack.clone(), + ) + .first(); let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); for elem in &arguments[2..] { slice.push_back(*elem); } - dbg!(slice.len()); + // dbg!(slice.len()); let new_slice = dfg.make_array(slice, element_type); + // dbg!(new_slice); // let slice_length_minus_one = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); - let set_last_slice_value_instr = Instruction::ArraySet { array: new_slice, index: arguments[0], value: arguments[2], length: Some(new_slice_length) }; - let set_last_slice_value = dfg.insert_instruction_and_results(set_last_slice_value_instr, block, None, call_stack.clone()).first(); - - let new_slice = merge_values_pub(len_not_equals_capacity, len_equals_capacity, set_last_slice_value, new_slice, block, dfg); + let set_last_slice_value_instr = Instruction::ArraySet { + array: new_slice, + index: arguments[0], + value: arguments[2], + length: Some(new_slice_length), + }; + let set_last_slice_value = dfg + .insert_instruction_and_results( + set_last_slice_value_instr, + block, + None, + call_stack.clone(), + ) + .first(); + + let new_slice = merge_values_pub( + len_not_equals_capacity, + len_equals_capacity, + set_last_slice_value, + new_slice, + block, + dfg, + ); // let new_slice_val = dfg.get_array_constant(new_slice); // dbg!(new_slice_val); + // dbg!(arguments[1]); + // dbg!(new_slice); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) + + // for elem in &arguments[2..] { + // slice.push_back(*elem); + // } + + // let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + + // let new_slice = dfg.make_array(slice, element_type); + // SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) } else { SimplifyResult::None } diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 47bc80e52c..71dea25c4b 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -141,7 +141,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, - dfg::{CallStack, InsertInstructionResult, DataFlowGraph}, + dfg::{CallStack, DataFlowGraph, InsertInstructionResult}, function::Function, function_inserter::FunctionInserter, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, @@ -205,8 +205,11 @@ struct Context<'f> { /// condition. If we are under multiple conditions (a nested if), the topmost condition is /// the most recent condition combined with all previous conditions via `And` instructions. conditions: Vec<(BasicBlockId, ValueId)>, + + slice_sizes: HashMap, } +#[derive(Debug, Clone)] struct Store { old_value: ValueId, new_value: ValueId, @@ -237,6 +240,7 @@ fn flatten_function_cfg(function: &mut Function) { branch_ends, conditions: Vec::new(), outer_block_stores: HashMap::default(), + slice_sizes: HashMap::default(), }; context.flatten(); } @@ -419,7 +423,14 @@ impl<'f> Context<'f> { match self.inserter.function.dfg.type_of_value(then_value) { Type::Numeric(_) => { // self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - merge_numeric_values_pub(then_condition, else_condition, then_value, else_value, self.inserter.function.entry_block(), &mut self.inserter.function.dfg) + merge_numeric_values_pub( + then_condition, + else_condition, + then_value, + else_value, + self.inserter.function.entry_block(), + &mut self.inserter.function.dfg, + ) } typ @ Type::Array(_, _) => { self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) @@ -447,15 +458,34 @@ impl<'f> Context<'f> { _ => panic!("Expected slice type"), }; + dbg!(then_value_id); let then_len = self.get_slice_length(then_value_id); dbg!(then_len); + dbg!(self.slice_sizes.insert(then_value_id, then_len)); + + if then_len == 6 { + dbg!(self.outer_block_stores.clone()); + dbg!(self.store_values.clone()); + } + dbg!(else_value_id); let else_len = self.get_slice_length(else_value_id); dbg!(else_len); - if else_len == 4 { - dbg!(&self.inserter.function.dfg.resolve(then_value_id)); - dbg!(&self.inserter.function.dfg.resolve(else_value_id)); - dbg!(&self.inserter.function.dfg[else_value_id]); + dbg!(self.slice_sizes.insert(else_value_id, else_len)); + + // just for debugging `merge_slices_mutate_between_ifs` + if else_len == 4 && then_len == 6 { + // let x = self.outer_block_stores; + dbg!(self.outer_block_stores.clone()); + dbg!(self.store_values.clone()); + + // else_len = 5; + // println!("then_value_id: {then_value_id}"); + // println!("else_value_id: {else_value_id}"); + // dbg!(&self.inserter.function.dfg.resolve(then_value_id)); + // dbg!(&self.inserter.function.dfg.resolve(else_value_id)); + // dbg!(&self.inserter.function.dfg[else_value_id]); } + // dbg!(self.slice_sizes.clone()); let len = then_len.max(else_len); @@ -489,48 +519,125 @@ impl<'f> Context<'f> { )); } } - - self.inserter.function.dfg.make_array(merged, typ) + dbg!(merged.len()); + let merged = self.inserter.function.dfg.make_array(merged, typ); + dbg!(merged); + merged } fn get_slice_length(&mut self, value_id: ValueId) -> usize { let value = &self.inserter.function.dfg[value_id]; - // dbg!(value); + dbg!(value_id); + dbg!(value); match value { - Value::Array { array, .. } => array.len(), + Value::Array { array, .. } => { + // dbg!(value_id); + // self.slice_sizes.insert(value_id, array.len()); + // dbg!(value_id); + array.len() + } Value::Instruction { instruction: instruction_id, .. } => { let instruction = &self.inserter.function.dfg[*instruction_id]; + // TODO: it may be better to check the results + // let results = self.inserter.function.dfg.instruction_results(*instruction_id); + // dbg!(results); + // dbg!(instruction.clone()); + // let results = + // self.inserter.function.dfg.instruction_results(*instruction_id); + // dbg!(results); match instruction { - Instruction::ArraySet { array, .. } => self.get_slice_length(*array), + Instruction::ArraySet { array, .. } => { + dbg!("got an array set"); + self.get_slice_length(*array) + } Instruction::Load { address } => { + let outer_store = self + .outer_block_stores + .get(address) + .expect("ICE: load in merger should have store from outer block"); + dbg!(self.slice_sizes.get(&outer_store.new_value)); + if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { + dbg!(outer_store.new_value); + dbg!(len); + return *len; + } + let context_store = if let Some(store) = self.store_values.get(address) { + dbg!("got here"); + dbg!(self.slice_sizes.get(&store.new_value)); + if let Some(len) = self.slice_sizes.get(&store.new_value) { + dbg!(store.new_value); + dbg!(len); + return *len; + } + store + // panic!("ahhh") } else { - self.outer_block_stores - .get(address) - .expect("ICE: load in merger should have store from outer block") + // dbg!("in outer_block"); + // self.outer_block_stores + // .get(address) + // .expect("ICE: load in merger should have store from outer block") + outer_store + // panic!("ahhh") }; - // let context_store = - // self.outer_block_stores + + // let outer_store = self.outer_block_stores // .get(address) // .expect("ICE: load in merger should have store from outer block"); - + // dbg!(self.slice_sizes.get(&outer_store.new_value)); + // if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { + // dbg!(outer_store.new_value); + // dbg!(len); + // return *len + // } else { + // self.get_slice_length(outer_store.new_value) + // } + dbg!("about to recursively call get_slice_length"); self.get_slice_length(context_store.new_value) } Instruction::Call { func, arguments } => { + // TODO: look into just storing a map of the slice lengths to reference so we don't have to recursively + // search here let func = &self.inserter.function.dfg[*func]; let slice_contents = arguments[1]; + // dbg!(slice_contents); + // dbg!(&self.inserter.function.dfg[slice_contents]); + // match &self.inserter.function.dfg[slice_contents] { + // Value::Instruction { instruction, .. } => { + // dbg!(&self.inserter.function.dfg[*instruction]); + // } + // _ => (), + // } + + // self.get_slice_length(results[0]); match func { Value::Intrinsic(intrinsic) => match intrinsic { Intrinsic::SlicePushBack | Intrinsic::SlicePushFront | Intrinsic::SliceInsert => { - self.get_slice_length(slice_contents) + 1 + dbg!("about to call get_slice_length"); + let inner_len = self.get_slice_length(slice_contents); + dbg!(slice_contents); + dbg!(self.slice_sizes.get(&slice_contents)); + dbg!(inner_len); + dbg!(self.slice_sizes.insert(slice_contents, inner_len)); + + // dbg!(self.slice_sizes.insert(results[1], inner_len + 1)); + + // let results = + // self.inserter.function.dfg.instruction_results(*instruction_id); + // dbg!(results); + // self.get_slice_length(slice_contents, loaded_before) + 1 + inner_len + 1 } Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceRemove => { self.get_slice_length(slice_contents) - 1 + // let slice_size = self.slice_sizes.get(&slice_contents); + // dbg!(slice_size); + // slice_size.expect("ahhh") - 1 } _ => { unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") @@ -780,8 +887,13 @@ impl<'f> Context<'f> { if let Some(store) = self.store_values.get_mut(&address) { store.new_value = value; + // if let Some(store) = self.outer_block_stores.get_mut(&address) { + // println!("new_value: {value}"); + // store.new_value = value; + // } } else { self.store_values.insert(address, Store { old_value, new_value: value }); + // self.outer_block_stores.insert(address, Store { old_value, new_value: value }); } } } @@ -796,6 +908,11 @@ impl<'f> Context<'f> { let old_value = self.insert_instruction_with_typevars(load, load_type).first(); self.store_values.insert(address, Store { old_value, new_value }); + // if let Type::Slice(_) = self.inserter.function.dfg.type_of_value(new_value) { + // dbg!("got a slice bitch"); + // dbg!(new_value); + // dbg!(&self.inserter.function.dfg[new_value]); + // } } } } @@ -831,15 +948,21 @@ impl<'f> Context<'f> { fn push_instruction(&mut self, id: InstructionId) { let (instruction, call_stack) = self.inserter.map_instruction(id); let instruction = self.handle_instruction_side_effects(instruction, call_stack.clone()); - let is_allocate = matches!(instruction, Instruction::Allocate); + let is_allocate = matches!(instruction.clone(), Instruction::Allocate); let entry = self.inserter.function.entry_block(); - let results = self.inserter.push_instruction_value(instruction, id, entry, call_stack); + let results = + self.inserter.push_instruction_value(instruction.clone(), id, entry, call_stack); + let results = results.results(); + // if matches!(instruction, Instruction::Call { .. }) { + // dbg!(results.clone()); + // } + // dbg!(is_allocate); // Remember an allocate was created local to this branch so that we do not try to merge store // values across branches for it later. if is_allocate { - self.local_allocations.insert(results.first()); + self.local_allocations.insert(results[0]); } } @@ -892,7 +1015,6 @@ impl<'f> Context<'f> { } } - pub(crate) fn merge_values_pub( then_condition: ValueId, else_condition: ValueId, @@ -905,16 +1027,39 @@ pub(crate) fn merge_values_pub( match dfg.type_of_value(then_value) { Type::Numeric(_) => { // self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - merge_numeric_values_pub(then_condition, else_condition, then_value, else_value, block, dfg) + merge_numeric_values_pub( + then_condition, + else_condition, + then_value, + else_value, + block, + dfg, + ) } typ @ Type::Array(_, _) => { // self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) - merge_array_values_pub(typ, then_condition, else_condition, then_value, else_value, block, dfg) + merge_array_values_pub( + typ, + then_condition, + else_condition, + then_value, + else_value, + block, + dfg, + ) } typ @ Type::Slice(_) => { // self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) // panic!("ahh"); - merge_slice_values_pub(typ, then_condition, else_condition, then_value, else_value, block, dfg) + merge_slice_values_pub( + typ, + then_condition, + else_condition, + then_value, + else_value, + block, + dfg, + ) } Type::Reference => panic!("Cannot return references from an if expression"), Type::Function => panic!("Cannot return functions from an if expression"), @@ -938,9 +1083,9 @@ pub(crate) fn merge_slice_values_pub( }; let then_len = get_slice_length_pub(then_value_id, dfg); - dbg!(then_len); + // dbg!(then_len); let else_len = get_slice_length_pub(else_value_id, dfg); - dbg!(else_len); + // dbg!(else_len); let len = then_len.max(else_len); for i in 0..len { @@ -958,12 +1103,8 @@ pub(crate) fn merge_slice_values_pub( dfg.make_constant(zero, Type::field()) } else { let get = Instruction::ArrayGet { array, index }; - dfg.insert_instruction_and_results( - get, - block, - typevars, - CallStack::new(), - ).first() + dfg.insert_instruction_and_results(get, block, typevars, CallStack::new()) + .first() } }; @@ -971,12 +1112,12 @@ pub(crate) fn merge_slice_values_pub( let else_element = get_element(else_value_id, typevars, else_len); merged.push_back(merge_values_pub( - then_condition, - else_condition, - then_element, - else_element, - block, - dfg + then_condition, + else_condition, + then_element, + else_element, + block, + dfg, )) } } @@ -1061,24 +1202,19 @@ pub(crate) fn merge_array_values_pub( let mut get_element = |array, typevars| { let get = Instruction::ArrayGet { array, index }; - dfg.insert_instruction_and_results( - get, - block, - typevars, - CallStack::new(), - ).first() + dfg.insert_instruction_and_results(get, block, typevars, CallStack::new()).first() }; let then_element = get_element(then_value, typevars.clone()); let else_element = get_element(else_value, typevars); merged.push_back(merge_values_pub( - then_condition, - else_condition, - then_element, - else_element, - block, - dfg + then_condition, + else_condition, + then_element, + else_element, + block, + dfg, )) } } @@ -1106,37 +1242,39 @@ pub(crate) fn merge_numeric_values_pub( let then_call_stack = dfg.get_value_call_stack(then_value); let else_call_stack = dfg.get_value_call_stack(else_value); - let call_stack = if then_call_stack.is_empty() { - else_call_stack.clone() - } else { - then_call_stack.clone() - }; + let call_stack = + if then_call_stack.is_empty() { else_call_stack.clone() } else { then_call_stack.clone() }; // We must cast the bool conditions to the actual numeric type used by each value. let then_condition = dfg - .insert_instruction_and_results(Instruction::Cast(then_condition, then_type), block, None, call_stack.clone()) + .insert_instruction_and_results( + Instruction::Cast(then_condition, then_type), + block, + None, + call_stack.clone(), + ) .first(); let else_condition = dfg - .insert_instruction_and_results(Instruction::Cast(else_condition, else_type), block, None, call_stack.clone()) + .insert_instruction_and_results( + Instruction::Cast(else_condition, else_type), + block, + None, + call_stack.clone(), + ) .first(); let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); - let then_value = dfg - .insert_instruction_and_results(mul, block, None, call_stack.clone()) - .first(); + let then_value = + dfg.insert_instruction_and_results(mul, block, None, call_stack.clone()).first(); let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); - let else_value = dfg - .insert_instruction_and_results(mul, block, None, call_stack.clone()) - .first(); + let else_value = + dfg.insert_instruction_and_results(mul, block, None, call_stack.clone()).first(); let add = Instruction::binary(BinaryOp::Add, then_value, else_value); - dfg - .insert_instruction_and_results(add, block, None, call_stack) - .first() + dfg.insert_instruction_and_results(add, block, None, call_stack).first() } - #[cfg(test)] mod test { use std::rc::Rc; diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 51477a4574..1efe77de64 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -346,7 +346,11 @@ impl<'a> FunctionContext<'a> { let is_offset_out_of_bounds = self.builder.insert_binary(index, BinaryOp::Lt, array_len); let true_const = self.builder.numeric_constant(true, Type::bool()); - self.builder.insert_constrain(is_offset_out_of_bounds, true_const, Some("Index out of bounds".to_owned())); + self.builder.insert_constrain( + is_offset_out_of_bounds, + true_const, + Some("Index out of bounds".to_owned()), + ); } fn codegen_cast(&mut self, cast: &ast::Cast) -> Values { diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs index 0784fb467f..2c35d66d78 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -6,8 +6,8 @@ use crate::{ hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, node_interner::TraitId, parser::SubModule, - FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, - NoirTrait, NoirTypeAlias, ParsedModule, TraitImpl, TraitImplItem, TraitItem, TypeImpl, + FunctionDefinition, Ident, LetStatement, NoirFunction, NoirStruct, NoirTrait, NoirTypeAlias, + ParsedModule, TraitImpl, TraitImplItem, TraitItem, TypeImpl, }; use super::{ From e91e18d9c0cc010364b9abd8c9a6c183c5b4ed1a Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 11 Sep 2023 20:36:03 +0000 Subject: [PATCH 03/16] a little cleanup --- .../execution_success/slices/src/main.nr | 92 +++++++++---------- .../src/ssa/ir/instruction/call.rs | 12 +-- .../src/ssa/opt/flatten_cfg.rs | 55 +---------- 3 files changed, 47 insertions(+), 112 deletions(-) diff --git a/crates/nargo_cli/tests/execution_success/slices/src/main.nr b/crates/nargo_cli/tests/execution_success/slices/src/main.nr index 8c22e0e90c..a552056480 100644 --- a/crates/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/slices/src/main.nr @@ -2,49 +2,49 @@ 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); + 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); // TODO: need to implement dynamic arrays for // regression_2083(); @@ -119,12 +119,6 @@ fn merge_slices_if(x: Field, y: Field) { assert(slice[5] == 15); assert(slice[6] == 50); assert(slice[7] == 60); - let len = slice.len(); - dep::std::println(f"len: {len}"); - for i in 0..8 { - let elem = slice[i]; - dep::std::println(f"slice[{i}]: {elem}"); - } } diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs index 4cdef219d5..c28b44af5f 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -92,13 +92,11 @@ pub(super) fn simplify_call( Intrinsic::SlicePushBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((mut slice, element_type)) = slice { - // let length = dfg.get_numeric_constant(arguments[0]); - // dbg!(length); - // dbg!(slice.len()); - // The capacity must be an integer so that we can compare it against the slice length which is represented as a field let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); + // TODO: This is the user-facing length, we need to handle the element_type size + // to appropriately handle arrays of complex types let len_equals_capacity_instr = Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, @@ -128,9 +126,7 @@ pub(super) fn simplify_call( for elem in &arguments[2..] { slice.push_back(*elem); } - // dbg!(slice.len()); let new_slice = dfg.make_array(slice, element_type); - // dbg!(new_slice); // let slice_length_minus_one = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); let set_last_slice_value_instr = Instruction::ArraySet { @@ -156,10 +152,6 @@ pub(super) fn simplify_call( block, dfg, ); - // let new_slice_val = dfg.get_array_constant(new_slice); - // dbg!(new_slice_val); - // dbg!(arguments[1]); - // dbg!(new_slice); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) // for elem in &arguments[2..] { diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 71dea25c4b..303816b508 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -527,24 +527,10 @@ impl<'f> Context<'f> { fn get_slice_length(&mut self, value_id: ValueId) -> usize { let value = &self.inserter.function.dfg[value_id]; - dbg!(value_id); - dbg!(value); match value { - Value::Array { array, .. } => { - // dbg!(value_id); - // self.slice_sizes.insert(value_id, array.len()); - // dbg!(value_id); - array.len() - } + Value::Array { array, .. } => array.len(), Value::Instruction { instruction: instruction_id, .. } => { let instruction = &self.inserter.function.dfg[*instruction_id]; - // TODO: it may be better to check the results - // let results = self.inserter.function.dfg.instruction_results(*instruction_id); - // dbg!(results); - // dbg!(instruction.clone()); - // let results = - // self.inserter.function.dfg.instruction_results(*instruction_id); - // dbg!(results); match instruction { Instruction::ArraySet { array, .. } => { dbg!("got an array set"); @@ -572,28 +558,10 @@ impl<'f> Context<'f> { } store - // panic!("ahhh") } else { - // dbg!("in outer_block"); - // self.outer_block_stores - // .get(address) - // .expect("ICE: load in merger should have store from outer block") outer_store - // panic!("ahhh") }; - // let outer_store = self.outer_block_stores - // .get(address) - // .expect("ICE: load in merger should have store from outer block"); - // dbg!(self.slice_sizes.get(&outer_store.new_value)); - // if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { - // dbg!(outer_store.new_value); - // dbg!(len); - // return *len - // } else { - // self.get_slice_length(outer_store.new_value) - // } - dbg!("about to recursively call get_slice_length"); self.get_slice_length(context_store.new_value) } Instruction::Call { func, arguments } => { @@ -601,16 +569,6 @@ impl<'f> Context<'f> { // search here let func = &self.inserter.function.dfg[*func]; let slice_contents = arguments[1]; - // dbg!(slice_contents); - // dbg!(&self.inserter.function.dfg[slice_contents]); - // match &self.inserter.function.dfg[slice_contents] { - // Value::Instruction { instruction, .. } => { - // dbg!(&self.inserter.function.dfg[*instruction]); - // } - // _ => (), - // } - - // self.get_slice_length(results[0]); match func { Value::Intrinsic(intrinsic) => match intrinsic { Intrinsic::SlicePushBack @@ -622,22 +580,13 @@ impl<'f> Context<'f> { dbg!(self.slice_sizes.get(&slice_contents)); dbg!(inner_len); dbg!(self.slice_sizes.insert(slice_contents, inner_len)); - - // dbg!(self.slice_sizes.insert(results[1], inner_len + 1)); - - // let results = - // self.inserter.function.dfg.instruction_results(*instruction_id); - // dbg!(results); - // self.get_slice_length(slice_contents, loaded_before) + 1 inner_len + 1 } Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceRemove => { + // TODO: handle slice removal intrinsics self.get_slice_length(slice_contents) - 1 - // let slice_size = self.slice_sizes.get(&slice_contents); - // dbg!(slice_size); - // slice_size.expect("ahhh") - 1 } _ => { unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") From ef6baa7ec74fecbf06cb335fe00dee97b1f1f345 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 13 Sep 2023 13:48:06 +0000 Subject: [PATCH 04/16] working pop_back with multiple slice mergers --- .../execution_success/slices/src/main.nr | 173 +++++++++++------- .../src/ssa/ir/instruction/call.rs | 93 +++++++--- 2 files changed, 176 insertions(+), 90 deletions(-) diff --git a/crates/nargo_cli/tests/execution_success/slices/src/main.nr b/crates/nargo_cli/tests/execution_success/slices/src/main.nr index a552056480..8d47d3da93 100644 --- a/crates/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/slices/src/main.nr @@ -2,49 +2,49 @@ 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); + // 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); // TODO: need to implement dynamic arrays for // regression_2083(); @@ -88,38 +88,39 @@ fn regression_2083() { // The parameters to this function must come from witness values (inputs to main) fn regression_merge_slices(x: Field, y: Field) { merge_slices_if(x, y); - merge_slices_else(x); + // merge_slices_else(x); } fn merge_slices_if(x: Field, y: Field) { - let slice = merge_slices_return(x, y); - assert(slice[2] == 10); - assert(slice.len() == 3); + // let slice = merge_slices_return(x, y); + // assert(slice[2] == 10); + // assert(slice.len() == 3); - let slice = merge_slices_mutate(x, y); - assert(slice[3] == 5); - assert(slice.len() == 4); + // let slice = merge_slices_mutate(x, y); + // assert(slice[3] == 5); + // assert(slice.len() == 4); - let slice = merge_slices_mutate_in_loop(x, y); - assert(slice[6] == 4); - assert(slice.len() == 7); + // let slice = merge_slices_mutate_in_loop(x, y); + // assert(slice[6] == 4); + // assert(slice.len() == 7); - let slice = merge_slices_mutate_two_ifs(x, y); - let len = slice.len(); - dep::std::println(f"len: {len}"); - assert(slice.len() == 6); - assert(slice[3] == 5); - assert(slice[4] == 15); - assert(slice[5] == 30); - - let slice = merge_slices_mutate_between_ifs(x, y); - assert(slice.len() == 8); - assert(slice[3] == 5); - assert(slice[4] == 30); - assert(slice[5] == 15); - assert(slice[6] == 50); - assert(slice[7] == 60); + // let slice = merge_slices_mutate_two_ifs(x, y); + // let len = slice.len(); + // dep::std::println(f"len: {len}"); + // assert(slice.len() == 6); + // assert(slice[3] == 5); + // assert(slice[4] == 15); + // assert(slice[5] == 30); + + // let slice = merge_slices_mutate_between_ifs(x, y); + // assert(slice.len() == 8); + // assert(slice[3] == 5); + // assert(slice[4] == 30); + // assert(slice[5] == 15); + // assert(slice[6] == 50); + // assert(slice[7] == 60); + let slice = merge_slices_push_then_pop(x, y); } fn merge_slices_else(x: Field) { @@ -195,7 +196,6 @@ fn merge_slices_mutate_two_ifs(x: Field, y: Field) -> [Field] { slice } -// Still broken fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { let mut slice = [0; 2]; if x != y { @@ -222,3 +222,38 @@ fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { slice } + +fn merge_slices_push_then_pop(x: Field, y: Field) -> [Field] { + let mut slice = [0; 2]; + if x != y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + slice = slice.push_back(30); + + if x == 20 { + slice = slice.push_back(20); + } + let len = slice.len(); + std::println(f"len: {len}"); + let (slice, elem) = slice.pop_back(); + let len = slice.len(); + std::println(f"len: {len}"); + std::println(elem); + assert(elem == 30); + + let (slice, elem) = slice.pop_back(); + std::println(elem); + + assert(elem == x); + + // let (elem, slice) = slice.pop_front(); + // assert(elem == 0); + // let (elem, slice) = slice.pop_front(); + // assert(elem == 0); + + slice +} diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs index c28b44af5f..5d25af7c05 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -6,7 +6,7 @@ use num_bigint::BigUint; use crate::ssa::ir::{ basic_block::BasicBlockId, - dfg::DataFlowGraph, + dfg::{DataFlowGraph, CallStack}, instruction::Intrinsic, map::Id, types::Type, @@ -97,6 +97,28 @@ pub(super) fn simplify_call( // TODO: This is the user-facing length, we need to handle the element_type size // to appropriately handle arrays of complex types + dbg!(element_type.element_size()); + if element_type.element_size() != 1 { + // Old code before implementing multiple slice mergers + for elem in &arguments[2..] { + slice.push_back(*elem); + } + + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + + let new_slice = dfg.make_array(slice, element_type); + return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) + } + // For nested arrays + // let element_size = dfg.make_constant((element_type.element_size() as u128).into(), Type::field()); + // let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size); + // let flattened_len = dfg.insert_instruction_and_results( + // flattened_len_instr, block, None, CallStack::new()).first(); + // let len_equals_capacity_instr = Instruction::Binary(Binary { + // lhs: flattened_len, + // operator: BinaryOp::Eq, + // rhs: capacity, + // }); let len_equals_capacity_instr = Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, @@ -128,7 +150,6 @@ pub(super) fn simplify_call( } let new_slice = dfg.make_array(slice, element_type); - // let slice_length_minus_one = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); let set_last_slice_value_instr = Instruction::ArraySet { array: new_slice, index: arguments[0], @@ -143,7 +164,6 @@ pub(super) fn simplify_call( call_stack.clone(), ) .first(); - let new_slice = merge_values_pub( len_not_equals_capacity, len_equals_capacity, @@ -153,15 +173,29 @@ pub(super) fn simplify_call( dfg, ); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) - + // array set multiple elements for nested arrays + // let mut set_last_slice_value_instr = new_slice; + // let mut set_last_slice_value = new_slice; + // let mut flattened_index = flattened_len; + // let one = dfg.make_constant(FieldElement::one(), Type::field()); // for elem in &arguments[2..] { - // slice.push_back(*elem); + // let set_last_slice_value_instr = Instruction::ArraySet { + // array: set_last_slice_value, + // index: flattened_index, + // value: *elem, + // length: Some(new_slice_length), + // }; + // set_last_slice_value = dfg + // .insert_instruction_and_results( + // set_last_slice_value_instr, + // block, + // None, + // CallStack::new(), + // ) + // .first(); + // let flattened_index_instr = Instruction::binary(BinaryOp::Add, flattened_index, one); + // flattened_index = dfg.insert_instruction_and_results(flattened_index_instr, block, None, CallStack::new()).first(); // } - - // let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); - - // let new_slice = dfg.make_array(slice, element_type); - // SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) } else { SimplifyResult::None } @@ -183,25 +217,42 @@ pub(super) fn simplify_call( } Intrinsic::SlicePopBack => { let slice = dfg.get_array_constant(arguments[1]); - if let Some((mut slice, typ)) = slice { + dbg!(slice.clone()); + if let Some((_, typ)) = slice { let element_count = typ.element_size(); let mut results = VecDeque::with_capacity(element_count + 1); - // We must pop multiple elements in the case of a slice of tuples - for _ in 0..element_count { - let elem = slice - .pop_back() - .expect("There are no elements in this slice to be removed"); - results.push_front(elem); - } + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); - let new_slice = dfg.make_array(slice, typ); - results.push_front(new_slice); + let get_last_elem_instr = Instruction::ArrayGet { array: arguments[1], index: new_slice_length }; + let get_last_elem = dfg + .insert_instruction_and_results( + get_last_elem_instr, + block, + Some(vec![typ]), + CallStack::new(), + ) + .first(); - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + results.push_front(get_last_elem); + results.push_front(arguments[1]); results.push_front(new_slice_length); SimplifyResult::SimplifiedToMultiple(results.into()) + + // Old code + // We must pop multiple elements in the case of a slice of tuples + // for _ in 0..element_count { + // let elem = slice + // .pop_back() + // .expect("There are no elements in this slice to be removed"); + // results.push_front(elem); + // } + + // let new_slice = dfg.make_array(slice, typ); + // results.push_front(new_slice); + + } else { SimplifyResult::None } From 8346673b4c53071fcecab12ddd67528154ab1543 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 13 Sep 2023 21:24:05 +0000 Subject: [PATCH 05/16] ValueMerger struct and additional tests --- .../src/ssa/ir/instruction/call.rs | 73 +- .../src/ssa/opt/flatten_cfg.rs | 663 ++++++++++-------- .../slice_dynamic_index/src/main.nr | 143 ++-- .../execution_success/slices/src/main.nr | 214 +++--- 4 files changed, 649 insertions(+), 444 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 5d25af7c05..18b94148ad 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -1,18 +1,21 @@ use std::{collections::VecDeque, rc::Rc}; use acvm::{acir::BlackBoxFunc, BlackBoxResolutionError, FieldElement}; +use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; use num_bigint::BigUint; -use crate::ssa::ir::{ - basic_block::BasicBlockId, - dfg::{DataFlowGraph, CallStack}, - instruction::Intrinsic, - map::Id, - types::Type, - value::{Value, ValueId}, +use crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + dfg::{CallStack, DataFlowGraph}, + instruction::Intrinsic, + map::Id, + types::Type, + value::{Value, ValueId}, + }, + opt::flatten_cfg::ValueMerger, }; -use crate::ssa::opt::flatten_cfg::merge_values_pub; use super::{Binary, BinaryOp, Endian, Instruction, SimplifyResult}; @@ -97,19 +100,20 @@ pub(super) fn simplify_call( // TODO: This is the user-facing length, we need to handle the element_type size // to appropriately handle arrays of complex types - dbg!(element_type.element_size()); + // dbg!(element_type.element_size()); if element_type.element_size() != 1 { // Old code before implementing multiple slice mergers for elem in &arguments[2..] { slice.push_back(*elem); } - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + let new_slice_length = + update_slice_length(arguments[0], dfg, BinaryOp::Add, block); let new_slice = dfg.make_array(slice, element_type); - return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) + return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]); } - // For nested arrays + // For nested arrays // let element_size = dfg.make_constant((element_type.element_size() as u128).into(), Type::field()); // let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size); // let flattened_len = dfg.insert_instruction_and_results( @@ -164,16 +168,21 @@ pub(super) fn simplify_call( call_stack.clone(), ) .first(); - let new_slice = merge_values_pub( + + let store_values = &HashMap::default(); + let outer_block_stores = &HashMap::default(); + let slice_sizes = &mut HashMap::default(); + let mut value_merger = + ValueMerger::new(dfg, block, store_values, outer_block_stores, slice_sizes); + let new_slice = value_merger.merge_values( len_not_equals_capacity, len_equals_capacity, set_last_slice_value, new_slice, - block, - dfg, ); + SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) - // array set multiple elements for nested arrays + // array set multiple elements for nested arrays // let mut set_last_slice_value_instr = new_slice; // let mut set_last_slice_value = new_slice; // let mut flattened_index = flattened_len; @@ -217,24 +226,42 @@ pub(super) fn simplify_call( } Intrinsic::SlicePopBack => { let slice = dfg.get_array_constant(arguments[1]); - dbg!(slice.clone()); if let Some((_, typ)) = slice { let element_count = typ.element_size(); let mut results = VecDeque::with_capacity(element_count + 1); let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); - let get_last_elem_instr = Instruction::ArrayGet { array: arguments[1], index: new_slice_length }; - let get_last_elem = dfg + let element_size = dfg.make_constant((element_count as u128).into(), Type::field()); + let flattened_len_instr = + Instruction::binary(BinaryOp::Mul, arguments[0], element_size); + let mut flattened_len = dfg .insert_instruction_and_results( - get_last_elem_instr, + flattened_len_instr, block, - Some(vec![typ]), + None, CallStack::new(), ) .first(); + flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + let mut flattened_index = flattened_len; + for _ in 0..element_count { + let get_last_elem_instr = + Instruction::ArrayGet { array: arguments[1], index: flattened_index }; + let get_last_elem = dfg + .insert_instruction_and_results( + get_last_elem_instr, + block, + Some(vec![typ.clone()]), + CallStack::new(), + ) + .first(); + results.push_front(get_last_elem); + + flattened_index = + update_slice_length(flattened_index, dfg, BinaryOp::Sub, block); + } - results.push_front(get_last_elem); results.push_front(arguments[1]); results.push_front(new_slice_length); @@ -251,8 +278,6 @@ pub(super) fn simplify_call( // let new_slice = dfg.make_array(slice, typ); // results.push_front(new_slice); - - } else { SimplifyResult::None } diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 303816b508..fa8b0f97d7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -152,6 +152,7 @@ use crate::ssa::{ }; mod branch_analysis; +// mod value_merger; impl Ssa { /// Flattens the control flow graph of main such that the function is left with a @@ -207,10 +208,11 @@ struct Context<'f> { conditions: Vec<(BasicBlockId, ValueId)>, slice_sizes: HashMap, + // value_merger: ValueMergerNew<'f>, } #[derive(Debug, Clone)] -struct Store { +pub(crate) struct Store { old_value: ValueId, new_value: ValueId, } @@ -242,10 +244,43 @@ fn flatten_function_cfg(function: &mut Function) { outer_block_stores: HashMap::default(), slice_sizes: HashMap::default(), }; + // let mut context = Context::new(function); context.flatten(); } impl<'f> Context<'f> { + // fn new(function: &'f mut Function) -> Self { + // let cfg = ControlFlowGraph::with_function(function); + // let branch_ends = branch_analysis::find_branch_ends(function, &cfg); + + // let inserter = FunctionInserter::new(function); + + // let store_values = HashMap::default(); + // let outer_block_stores = HashMap::default(); + // let mut slice_sizes = HashMap::default(); + + // let block = inserter.function.entry_block(); + // let value_merger = ValueMergerNew { + // dfg: &mut inserter.function.dfg, + // block, + // store_values: &store_values, + // outer_block_stores: &outer_block_stores, + // slice_sizes: &mut slice_sizes, + // }; + + // Context { + // inserter, + // cfg, + // store_values, + // local_allocations: HashSet::new(), + // branch_ends, + // conditions: Vec::new(), + // outer_block_stores, + // slice_sizes, + // value_merger, + // } + // } + fn flatten(&mut self) { // Start with following the terminator of the entry block since we don't // need to flatten the entry block into itself. @@ -422,15 +457,15 @@ impl<'f> Context<'f> { ) -> ValueId { match self.inserter.function.dfg.type_of_value(then_value) { Type::Numeric(_) => { - // self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - merge_numeric_values_pub( - then_condition, - else_condition, - then_value, - else_value, - self.inserter.function.entry_block(), - &mut self.inserter.function.dfg, - ) + self.merge_numeric_values(then_condition, else_condition, then_value, else_value) + // merge_numeric_values_pub( + // then_condition, + // else_condition, + // then_value, + // else_value, + // self.inserter.function.entry_block(), + // &mut self.inserter.function.dfg, + // ) } typ @ Type::Array(_, _) => { self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) @@ -463,30 +498,11 @@ impl<'f> Context<'f> { dbg!(then_len); dbg!(self.slice_sizes.insert(then_value_id, then_len)); - if then_len == 6 { - dbg!(self.outer_block_stores.clone()); - dbg!(self.store_values.clone()); - } dbg!(else_value_id); let else_len = self.get_slice_length(else_value_id); dbg!(else_len); dbg!(self.slice_sizes.insert(else_value_id, else_len)); - // just for debugging `merge_slices_mutate_between_ifs` - if else_len == 4 && then_len == 6 { - // let x = self.outer_block_stores; - dbg!(self.outer_block_stores.clone()); - dbg!(self.store_values.clone()); - - // else_len = 5; - // println!("then_value_id: {then_value_id}"); - // println!("else_value_id: {else_value_id}"); - // dbg!(&self.inserter.function.dfg.resolve(then_value_id)); - // dbg!(&self.inserter.function.dfg.resolve(else_value_id)); - // dbg!(&self.inserter.function.dfg[else_value_id]); - } - // dbg!(self.slice_sizes.clone()); - let len = then_len.max(else_len); for i in 0..len { @@ -534,7 +550,10 @@ impl<'f> Context<'f> { match instruction { Instruction::ArraySet { array, .. } => { dbg!("got an array set"); - self.get_slice_length(*array) + let array = *array; + let len = self.get_slice_length(array); + dbg!(self.slice_sizes.insert(array, len)); + len } Instruction::Load { address } => { let outer_store = self @@ -549,24 +568,26 @@ impl<'f> Context<'f> { } let context_store = if let Some(store) = self.store_values.get(address) { - dbg!("got here"); - dbg!(self.slice_sizes.get(&store.new_value)); if let Some(len) = self.slice_sizes.get(&store.new_value) { - dbg!(store.new_value); - dbg!(len); return *len; } store } else { + // let outer_store = self + // .outer_block_stores + // .get(address) + // .expect("ICE: load in merger should have store from outer block"); + + // if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { + // return *len; + // } outer_store }; self.get_slice_length(context_store.new_value) } Instruction::Call { func, arguments } => { - // TODO: look into just storing a map of the slice lengths to reference so we don't have to recursively - // search here let func = &self.inserter.function.dfg[*func]; let slice_contents = arguments[1]; match func { @@ -575,17 +596,17 @@ impl<'f> Context<'f> { | Intrinsic::SlicePushFront | Intrinsic::SliceInsert => { dbg!("about to call get_slice_length"); - let inner_len = self.get_slice_length(slice_contents); - dbg!(slice_contents); - dbg!(self.slice_sizes.get(&slice_contents)); - dbg!(inner_len); - dbg!(self.slice_sizes.insert(slice_contents, inner_len)); - inner_len + 1 + let initial_len = self.get_slice_length(slice_contents); + dbg!(self.slice_sizes.insert(slice_contents, initial_len)); + initial_len + 1 } Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceRemove => { - // TODO: handle slice removal intrinsics + // TODO: for some reason this breaks lol + // let initial_len = self.get_slice_length(slice_contents); + // self.slice_sizes.insert(slice_contents, initial_len); + // initial_len + 1 self.get_slice_length(slice_contents) - 1 } _ => { @@ -795,9 +816,23 @@ impl<'f> Context<'f> { (self.inserter.resolve(*then_arg), self.inserter.resolve(else_arg)) }); + let block = self.inserter.function.entry_block(); + let mut value_merger = ValueMerger { + dfg: &mut self.inserter.function.dfg, + block, + store_values: &self.store_values, + outer_block_stores: &self.outer_block_stores, + slice_sizes: &mut self.slice_sizes, + }; + // Cannot include this in the previous vecmap since it requires exclusive access to self let args = vecmap(args, |(then_arg, else_arg)| { - self.merge_values(then_branch.condition, else_branch.condition, then_arg, else_arg) + value_merger.merge_values( + then_branch.condition, + else_branch.condition, + then_arg, + else_arg, + ) }); self.merge_stores(then_branch, else_branch); @@ -830,19 +865,34 @@ impl<'f> Context<'f> { let then_condition = then_branch.condition; let else_condition = else_branch.condition; - for (address, (then_case, else_case, old_value)) in new_map { - let value = self.merge_values(then_condition, else_condition, then_case, else_case); + let block = self.inserter.function.entry_block(); + let mut value_merger = ValueMerger { + dfg: &mut self.inserter.function.dfg, + block, + store_values: &self.store_values, + outer_block_stores: &self.outer_block_stores, + slice_sizes: &mut self.slice_sizes, + }; + + // Merging must occur in a separate loop as we cannot `*self` as mutable more than one at a time + let mut new_values = HashMap::default(); + for (address, (then_case, else_case, _)) in &new_map { + let value = + value_merger.merge_values(then_condition, else_condition, *then_case, *else_case); + new_values.insert(address, value); + } + + // Replace stores with new merged values + for (address, (_, _, old_value)) in &new_map { + let value = new_values[address]; + let address = *address; self.insert_instruction_with_typevars(Instruction::Store { address, value }, None); if let Some(store) = self.store_values.get_mut(&address) { store.new_value = value; - // if let Some(store) = self.outer_block_stores.get_mut(&address) { - // println!("new_value: {value}"); - // store.new_value = value; - // } } else { - self.store_values.insert(address, Store { old_value, new_value: value }); - // self.outer_block_stores.insert(address, Store { old_value, new_value: value }); + self.store_values + .insert(address, Store { old_value: *old_value, new_value: value }); } } } @@ -857,11 +907,6 @@ impl<'f> Context<'f> { let old_value = self.insert_instruction_with_typevars(load, load_type).first(); self.store_values.insert(address, Store { old_value, new_value }); - // if let Type::Slice(_) = self.inserter.function.dfg.type_of_value(new_value) { - // dbg!("got a slice bitch"); - // dbg!(new_value); - // dbg!(&self.inserter.function.dfg[new_value]); - // } } } } @@ -903,15 +948,10 @@ impl<'f> Context<'f> { let results = self.inserter.push_instruction_value(instruction.clone(), id, entry, call_stack); - let results = results.results(); - // if matches!(instruction, Instruction::Call { .. }) { - // dbg!(results.clone()); - // } - // dbg!(is_allocate); // Remember an allocate was created local to this branch so that we do not try to merge store // values across branches for it later. if is_allocate { - self.local_allocations.insert(results[0]); + self.local_allocations.insert(results.first()); } } @@ -964,264 +1004,299 @@ impl<'f> Context<'f> { } } -pub(crate) fn merge_values_pub( - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, +pub(crate) struct ValueMerger<'a> { + dfg: &'a mut DataFlowGraph, block: BasicBlockId, - dfg: &mut DataFlowGraph, -) -> ValueId { - // let block = self.inserter.function.entry_block(); - match dfg.type_of_value(then_value) { - Type::Numeric(_) => { - // self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - merge_numeric_values_pub( - then_condition, - else_condition, - then_value, - else_value, - block, - dfg, - ) + store_values: &'a HashMap, + outer_block_stores: &'a HashMap, + slice_sizes: &'a mut HashMap, +} + +impl<'a> ValueMerger<'a> { + pub(crate) fn new( + dfg: &'a mut DataFlowGraph, + block: BasicBlockId, + store_values: &'a HashMap, + outer_block_stores: &'a HashMap, + slice_sizes: &'a mut HashMap, + ) -> Self { + // let store_values = Box::new(HashMap::default()); + ValueMerger { dfg, block, store_values, outer_block_stores, slice_sizes } + } + + pub(crate) fn merge_values( + &mut self, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + match self.dfg.type_of_value(then_value) { + Type::Numeric(_) => { + self.merge_numeric_values(then_condition, else_condition, then_value, else_value) + } + typ @ Type::Array(_, _) => { + self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) + } + typ @ Type::Slice(_) => { + self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) + } + Type::Reference => panic!("Cannot return references from an if expression"), + Type::Function => panic!("Cannot return functions from an if expression"), } - typ @ Type::Array(_, _) => { - // self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) - merge_array_values_pub( - typ, - then_condition, - else_condition, - then_value, - else_value, - block, - dfg, + } + + /// Merge two numeric values a and b from separate basic blocks to a single value. This + /// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. + pub(crate) fn merge_numeric_values( + &mut self, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + let then_type = self.dfg.type_of_value(then_value); + let else_type = self.dfg.type_of_value(else_value); + assert_eq!( + then_type, else_type, + "Expected values merged to be of the same type but found {then_type} and {else_type}" + ); + + let then_call_stack = self.dfg.get_value_call_stack(then_value); + let else_call_stack = self.dfg.get_value_call_stack(else_value); + + let call_stack = if then_call_stack.is_empty() { + else_call_stack.clone() + } else { + then_call_stack.clone() + }; + + // We must cast the bool conditions to the actual numeric type used by each value. + let then_condition = self + .dfg + .insert_instruction_and_results( + Instruction::Cast(then_condition, then_type), + self.block, + None, + call_stack.clone(), ) - } - typ @ Type::Slice(_) => { - // self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) - // panic!("ahh"); - merge_slice_values_pub( - typ, - then_condition, - else_condition, - then_value, - else_value, - block, - dfg, + .first(); + let else_condition = self + .dfg + .insert_instruction_and_results( + Instruction::Cast(else_condition, else_type), + self.block, + None, + call_stack.clone(), ) - } - Type::Reference => panic!("Cannot return references from an if expression"), - Type::Function => panic!("Cannot return functions from an if expression"), + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); + let then_value = self + .dfg + .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); + let else_value = self + .dfg + .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) + .first(); + + let add = Instruction::binary(BinaryOp::Add, then_value, else_value); + self.dfg.insert_instruction_and_results(add, self.block, None, call_stack).first() } -} -pub(crate) fn merge_slice_values_pub( - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value_id: ValueId, - else_value_id: ValueId, - block: BasicBlockId, - dfg: &mut DataFlowGraph, -) -> ValueId { - let mut merged = im::Vector::new(); + /// Given an if expression that returns an array: `if c { array1 } else { array2 }`, + /// this function will recursively merge array1 and array2 into a single resulting array + /// by creating a new array containing the result of self.merge_values for each element. + pub(crate) fn merge_array_values( + &mut self, + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + let mut merged = im::Vector::new(); - let element_types = match &typ { - Type::Slice(elements) => elements, - _ => panic!("Expected slice type"), - }; + let (element_types, len) = match &typ { + Type::Array(elements, len) => (elements, *len), + _ => panic!("Expected array type"), + }; - let then_len = get_slice_length_pub(then_value_id, dfg); - // dbg!(then_len); - let else_len = get_slice_length_pub(else_value_id, dfg); - // dbg!(else_len); - let len = then_len.max(else_len); - - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index_value = ((i * element_types.len() + element_index) as u128).into(); - let index = dfg.make_constant(index_value, Type::field()); - - let typevars = Some(vec![element_type.clone()]); - - let mut get_element = |array, typevars, len| { - // The smaller slice is filled with placeholder data. Codegen for slice accesses must - // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. - if (len - 1) < index_value.to_u128() as usize { - let zero = FieldElement::zero(); - dfg.make_constant(zero, Type::field()) - } else { + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index = ((i * element_types.len() + element_index) as u128).into(); + let index = self.dfg.make_constant(index, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars| { let get = Instruction::ArrayGet { array, index }; - dfg.insert_instruction_and_results(get, block, typevars, CallStack::new()) + self.dfg + .insert_instruction_and_results(get, self.block, typevars, CallStack::new()) .first() - } - }; - - let then_element = get_element(then_value_id, typevars.clone(), then_len); - let else_element = get_element(else_value_id, typevars, else_len); - - merged.push_back(merge_values_pub( - then_condition, - else_condition, - then_element, - else_element, - block, - dfg, - )) - } - } + }; - dfg.make_array(merged, typ) -} + let then_element = get_element(then_value, typevars.clone()); + let else_element = get_element(else_value, typevars); -fn get_slice_length_pub(value_id: ValueId, dfg: &DataFlowGraph) -> usize { - let value = &dfg[value_id]; - match value { - Value::Array { array, .. } => array.len(), - Value::Instruction { instruction: instruction_id, .. } => { - let instruction = &dfg[*instruction_id]; - match instruction { - Instruction::ArraySet { array, .. } => get_slice_length_pub(*array, dfg), - Instruction::Load { address } => { - panic!("should not have a load"); - // let context_store = if let Some(store) = self.store_values.get(address) { - // store - // } else { - // self.outer_block_stores - // .get(address) - // .expect("ICE: load in merger should have store from outer block") - // }; - - // self.get_slice_length(context_store.new_value) - } - Instruction::Call { func, arguments } => { - let func = &dfg[*func]; - let slice_contents = arguments[1]; - match func { - Value::Intrinsic(intrinsic) => match intrinsic { - Intrinsic::SlicePushBack - | Intrinsic::SlicePushFront - | Intrinsic::SliceInsert => { - get_slice_length_pub(slice_contents, dfg) + 1 - } - Intrinsic::SlicePopBack - | Intrinsic::SlicePopFront - | Intrinsic::SliceRemove => { - get_slice_length_pub(slice_contents, dfg) - 1 - } - _ => { - unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") - } - }, - _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), - } - } - _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), + merged.push_back(self.merge_values( + then_condition, + else_condition, + then_element, + else_element, + )) } } - _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), + + self.dfg.make_array(merged, typ) } -} -/// Given an if expression that returns an array: `if c { array1 } else { array2 }`, -/// this function will recursively merge array1 and array2 into a single resulting array -/// by creating a new array containing the result of self.merge_values for each element. -pub(crate) fn merge_array_values_pub( - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - block: BasicBlockId, - dfg: &mut DataFlowGraph, -) -> ValueId { - let mut merged = im::Vector::new(); + fn merge_slice_values( + &mut self, + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value_id: ValueId, + else_value_id: ValueId, + ) -> ValueId { + let mut merged = im::Vector::new(); - let (element_types, len) = match &typ { - Type::Array(elements, len) => (elements, *len), - _ => panic!("Expected array type"), - }; + let element_types = match &typ { + Type::Slice(elements) => elements, + _ => panic!("Expected slice type"), + }; - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index = ((i * element_types.len() + element_index) as u128).into(); - let index = dfg.make_constant(index, Type::field()); + dbg!(then_value_id); + let then_len = self.get_slice_length(then_value_id); + dbg!(then_len); + dbg!(self.slice_sizes.insert(then_value_id, then_len)); - let typevars = Some(vec![element_type.clone()]); + dbg!(else_value_id); + let else_len = self.get_slice_length(else_value_id); + dbg!(else_len); + dbg!(self.slice_sizes.insert(else_value_id, else_len)); - let mut get_element = |array, typevars| { - let get = Instruction::ArrayGet { array, index }; - dfg.insert_instruction_and_results(get, block, typevars, CallStack::new()).first() - }; + let len = then_len.max(else_len); - let then_element = get_element(then_value, typevars.clone()); - let else_element = get_element(else_value, typevars); - - merged.push_back(merge_values_pub( - then_condition, - else_condition, - then_element, - else_element, - block, - dfg, - )) + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index_value = ((i * element_types.len() + element_index) as u128).into(); + let index = self.dfg.make_constant(index_value, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars, len| { + // The smaller slice is filled with placeholder data. Codegen for slice accesses must + // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. + if (len - 1) < index_value.to_u128() as usize { + let zero = FieldElement::zero(); + self.dfg.make_constant(zero, Type::field()) + } else { + let get = Instruction::ArrayGet { array, index }; + self.dfg + .insert_instruction_and_results( + get, + self.block, + typevars, + CallStack::new(), + ) + .first() + } + }; + + let then_element = get_element(then_value_id, typevars.clone(), then_len); + let else_element = get_element(else_value_id, typevars, else_len); + + merged.push_back(self.merge_values( + then_condition, + else_condition, + then_element, + else_element, + )); + } } + dbg!(merged.len()); + let merged = self.dfg.make_array(merged, typ); + dbg!(merged); + merged } - dfg.make_array(merged, typ) -} - -/// Merge two numeric values a and b from separate basic blocks to a single value. This -/// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. -pub(crate) fn merge_numeric_values_pub( - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - block: BasicBlockId, - dfg: &mut DataFlowGraph, -) -> ValueId { - let then_type = dfg.type_of_value(then_value); - let else_type = dfg.type_of_value(else_value); - assert_eq!( - then_type, else_type, - "Expected values merged to be of the same type but found {then_type} and {else_type}" - ); - - let then_call_stack = dfg.get_value_call_stack(then_value); - let else_call_stack = dfg.get_value_call_stack(else_value); - - let call_stack = - if then_call_stack.is_empty() { else_call_stack.clone() } else { then_call_stack.clone() }; - - // We must cast the bool conditions to the actual numeric type used by each value. - let then_condition = dfg - .insert_instruction_and_results( - Instruction::Cast(then_condition, then_type), - block, - None, - call_stack.clone(), - ) - .first(); - let else_condition = dfg - .insert_instruction_and_results( - Instruction::Cast(else_condition, else_type), - block, - None, - call_stack.clone(), - ) - .first(); + fn get_slice_length(&mut self, value_id: ValueId) -> usize { + let value = &self.dfg[value_id]; + match value { + Value::Array { array, .. } => array.len(), + Value::Instruction { instruction: instruction_id, .. } => { + let instruction = &self.dfg[*instruction_id]; + match instruction { + Instruction::ArraySet { array, .. } => { + dbg!("got an array set"); + let array = *array; + let len = self.get_slice_length(array); + dbg!(self.slice_sizes.insert(array, len)); + len + } + Instruction::Load { address } => { + let outer_store = self + .outer_block_stores + .get(address) + .expect("ICE: load in merger should have store from outer block"); + dbg!(self.slice_sizes.get(&outer_store.new_value)); + if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { + dbg!(outer_store.new_value); + dbg!(len); + return *len; + } - let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); - let then_value = - dfg.insert_instruction_and_results(mul, block, None, call_stack.clone()).first(); + let context_store = if let Some(store) = self.store_values.get(address) { + if let Some(len) = self.slice_sizes.get(&store.new_value) { + return *len; + } - let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); - let else_value = - dfg.insert_instruction_and_results(mul, block, None, call_stack.clone()).first(); + store + } else { + outer_store + }; - let add = Instruction::binary(BinaryOp::Add, then_value, else_value); - dfg.insert_instruction_and_results(add, block, None, call_stack).first() + self.get_slice_length(context_store.new_value) + } + Instruction::Call { func, arguments } => { + let func = &self.dfg[*func]; + let slice_contents = arguments[1]; + match func { + Value::Intrinsic(intrinsic) => match intrinsic { + Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SliceInsert => { + dbg!("about to call get_slice_length"); + let initial_len = self.get_slice_length(slice_contents); + dbg!(self.slice_sizes.insert(slice_contents, initial_len)); + initial_len + 1 + } + Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceRemove => { + // TODO: for some reason this breaks lol + // let initial_len = self.get_slice_length(slice_contents); + // self.slice_sizes.insert(slice_contents, initial_len); + // initial_len + 1 + self.get_slice_length(slice_contents) - 1 + } + _ => { + unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") + } + }, + _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), + } + } + _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), + } + } + _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), + } + } } #[cfg(test)] diff --git a/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr b/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr index de5b4caef2..e4a3111682 100644 --- a/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr @@ -10,17 +10,19 @@ fn regression_dynamic_slice_index(x: Field, y: Field) { } assert(slice.len() == 5); - dynamic_slice_index_set_if(slice, x, y); - dynamic_slice_index_set_else(slice, x, y); - dynamic_slice_index_set_nested_if_else_else(slice, x, y); - dynamic_slice_index_set_nested_if_else_if(slice, x, y + 1); - dynamic_slice_index_if(slice, x); - dynamic_array_index_if([0, 1, 2, 3, 4], x); - dynamic_slice_index_else(slice, x); - - dynamic_slice_merge_if(slice, x); - dynamic_slice_merge_else(slice, x); - dynamic_slice_merge_two_ifs(slice, x); + // dynamic_slice_index_set_if(slice, x, y); + // dynamic_slice_index_set_else(slice, x, y); + // dynamic_slice_index_set_nested_if_else_else(slice, x, y); + // dynamic_slice_index_set_nested_if_else_if(slice, x, y + 1); + // dynamic_slice_index_if(slice, x); + // dynamic_array_index_if([0, 1, 2, 3, 4], x); + // dynamic_slice_index_else(slice, x); + + // dynamic_slice_merge_if(slice, x); + // dynamic_slice_merge_else(slice, x); + // dynamic_slice_merge_two_ifs(slice, x); + // dynamic_slice_merge_mutate_between_ifs(slice, x, y); + dynamic_slice_merge_push_then_pop(slice, x, y); } fn dynamic_slice_index_set_if(mut slice: [Field], x: Field, y: Field) { @@ -165,35 +167,6 @@ fn dynamic_slice_merge_else(mut slice: [Field], x: Field) { assert(slice[slice.len() - 1] == 20); } -fn dynamic_slice_merge_two_ifs(mut slice: [Field], x: Field) { - if x as u32 > 10 { - assert(slice[x] == 0); - slice[x] = 2; - } else { - assert(slice[x] == 4); - slice[x] = slice[x] - 2; - slice = slice.push_back(10); - } - - assert(slice.len() == 6); - assert(slice[slice.len() - 1] == 10); - - if x == 20 { - slice = slice.push_back(20); - } else { - slice = slice.push_back(15); - } - // TODO(#2599): Breaks if the push back happens without the else case - // slice = slice.push_back(15); - - assert(slice.len() == 7); - assert(slice[slice.len() - 1] == 15); - - slice = slice.push_back(20); - assert(slice.len() == 8); - assert(slice[slice.len() - 1] == 20); -} - fn dynamic_slice_index_set_nested_if_else_else(mut slice: [Field], x: Field, y: Field) { assert(slice[x] == 4); assert(slice[y] == 1); @@ -250,3 +223,93 @@ fn dynamic_slice_index_set_nested_if_else_if(mut slice: [Field], x: Field, y: Fi assert(slice[4] == 5); } +fn dynamic_slice_merge_two_ifs(mut slice: [Field], x: Field) { + if x as u32 > 10 { + assert(slice[x] == 0); + slice[x] = 2; + } else { + assert(slice[x] == 4); + slice[x] = slice[x] - 2; + slice = slice.push_back(10); + } + + assert(slice.len() == 6); + assert(slice[slice.len() - 1] == 10); + + if x == 20 { + slice = slice.push_back(20); + } + + slice = slice.push_back(15); + + assert(slice.len() == 7); + assert(slice[slice.len() - 1] == 15); + + slice = slice.push_back(20); + assert(slice.len() == 8); + assert(slice[slice.len() - 1] == 20); +} + + +fn dynamic_slice_merge_mutate_between_ifs(mut slice: [Field], x: Field, y: Field) { + if x != y { + slice[x] = 50; + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice[x] = slice[x] - 2; + slice = slice.push_back(x); + } + + slice = slice.push_back(30); + assert(slice.len() == 8); + + if x == 20 { + slice = slice.push_back(20); + } + + slice = slice.push_back(15); + + if x != 20 { + slice = slice.push_back(50); + } + + slice = slice.push_back(60); + assert(slice.len() == 11); + assert(slice[x] == 50); + assert(slice[slice.len() - 4] == 30); + assert(slice[slice.len() - 3] == 15); + assert(slice[slice.len() - 2] == 50); + assert(slice[slice.len() - 1] == 60); +} + +fn dynamic_slice_merge_push_then_pop(mut slice: [Field], x: Field, y: Field) { + if x != y { + slice[x] = 5; + slice = slice.push_back(y); + slice = slice.push_back(x); + assert(slice.len() == 7); + + let (popped_slice, elem) = slice.pop_back(); + assert(slice.len() == 7); + assert(elem == x); + slice = popped_slice; + } else { + slice = slice.push_back(x); + } + + slice = slice.push_back(30); + assert(slice.len() == 7); + + if x == 20 { + slice = slice.push_back(20); + } + + let (slice, elem) = slice.pop_back(); + assert(elem == 30); + + let (_, elem) = slice.pop_back(); + assert(elem == y); +} + + diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 8d47d3da93..5de7a431c4 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -2,52 +2,52 @@ 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); + 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); // TODO: need to implement dynamic arrays for - // regression_2083(); + regression_2083(); // The parameters to this function must come from witness values (inputs to main) regression_merge_slices(x, y); } @@ -92,35 +92,41 @@ fn regression_merge_slices(x: Field, y: Field) { } fn merge_slices_if(x: Field, y: Field) { - // let slice = merge_slices_return(x, y); - // assert(slice[2] == 10); - // assert(slice.len() == 3); - - // let slice = merge_slices_mutate(x, y); - // assert(slice[3] == 5); - // assert(slice.len() == 4); - - // let slice = merge_slices_mutate_in_loop(x, y); - // assert(slice[6] == 4); - // assert(slice.len() == 7); - - // let slice = merge_slices_mutate_two_ifs(x, y); - // let len = slice.len(); - // dep::std::println(f"len: {len}"); - // assert(slice.len() == 6); - // assert(slice[3] == 5); - // assert(slice[4] == 15); - // assert(slice[5] == 30); - - // let slice = merge_slices_mutate_between_ifs(x, y); - // assert(slice.len() == 8); - // assert(slice[3] == 5); - // assert(slice[4] == 30); - // assert(slice[5] == 15); - // assert(slice[6] == 50); - // assert(slice[7] == 60); + let slice = merge_slices_return(x, y); + assert(slice[2] == 10); + assert(slice.len() == 3); + + let slice = merge_slices_mutate(x, y); + assert(slice[3] == 5); + assert(slice.len() == 4); + + let slice = merge_slices_mutate_in_loop(x, y); + assert(slice[6] == 4); + assert(slice.len() == 7); + + let slice = merge_slices_mutate_two_ifs(x, y); + assert(slice.len() == 6); + assert(slice[3] == 5); + assert(slice[4] == 15); + assert(slice[5] == 30); + + let slice = merge_slices_mutate_between_ifs(x, y); + assert(slice.len() == 8); + assert(slice[3] == 5); + assert(slice[4] == 30); + assert(slice[5] == 15); + assert(slice[6] == 50); + assert(slice[7] == 60); - let slice = merge_slices_push_then_pop(x, y); + merge_slices_push_then_pop(x, y); + + let slice = merge_slices_push_then_insert(x, y); + assert(slice[1] == 50); + assert(slice[2] == 0); + assert(slice[5] == 30); + + let slice = merge_slices_remove_between_ifs(x, y); + assert(slice.len() == 5); } fn merge_slices_else(x: Field) { @@ -206,8 +212,6 @@ fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { } slice = slice.push_back(30); - // let len = slice.len(); - // dep::std::println(f"len: {len}"); if x == 20 { slice = slice.push_back(20); } @@ -223,7 +227,7 @@ fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { slice } -fn merge_slices_push_then_pop(x: Field, y: Field) -> [Field] { +fn merge_slices_push_then_pop(x: Field, y: Field) { let mut slice = [0; 2]; if x != y { slice = slice.push_back(y); @@ -237,23 +241,61 @@ fn merge_slices_push_then_pop(x: Field, y: Field) -> [Field] { if x == 20 { slice = slice.push_back(20); } - let len = slice.len(); - std::println(f"len: {len}"); + let (slice, elem) = slice.pop_back(); - let len = slice.len(); - std::println(f"len: {len}"); - std::println(elem); + assert(slice.len() == 4); assert(elem == 30); let (slice, elem) = slice.pop_back(); - std::println(elem); - + assert(slice.len() == 3); assert(elem == x); +} + +fn merge_slices_push_then_insert(x: Field, y: Field) -> [Field] { + let mut slice = [0; 2]; + if x != y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + slice = slice.push_back(30); - // let (elem, slice) = slice.pop_front(); - // assert(elem == 0); - // let (elem, slice) = slice.pop_front(); - // assert(elem == 0); + if x == 20 { + slice = slice.push_back(20); + slice = slice.push_back(15); + } + + // TODO: need to constrain no insert that is greater than the length of the slice + // Currently, we can potentially insert into placeholder information that is greater than the slice length + // and we will end up with the last elem being zero rather than the elem we are inserting + let slice = slice.insert(1, 50); + + slice +} + +fn merge_slices_remove_between_ifs(x: Field, y: Field) -> [Field] { + let mut slice = [0; 2]; + if x != y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + let (mut slice, elem) = slice.remove(2); + assert(elem == y); + + if x == 20 { + slice = slice.push_back(20); + } + + slice = slice.push_back(15); + + if x != 20 { + slice = slice.push_back(50); + } slice } From 7a2dfdefd19af27e4e0c192b4c85b1eb016022c4 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 13 Sep 2023 21:35:29 +0000 Subject: [PATCH 06/16] moved ValueMerger into its own file --- .../src/ssa/ir/instruction/call.rs | 5 +- .../src/ssa/opt/flatten_cfg.rs | 645 +----------------- .../src/ssa/opt/flatten_cfg/value_merger.rs | 319 +++++++++ 3 files changed, 336 insertions(+), 633 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 18b94148ad..4ec993a83e 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -14,7 +14,7 @@ use crate::ssa::{ types::Type, value::{Value, ValueId}, }, - opt::flatten_cfg::ValueMerger, + opt::flatten_cfg::value_merger::ValueMerger, }; use super::{Binary, BinaryOp, Endian, Instruction, SimplifyResult}; @@ -171,9 +171,8 @@ pub(super) fn simplify_call( let store_values = &HashMap::default(); let outer_block_stores = &HashMap::default(); - let slice_sizes = &mut HashMap::default(); let mut value_merger = - ValueMerger::new(dfg, block, store_values, outer_block_stores, slice_sizes); + ValueMerger::new(dfg, block, store_values, outer_block_stores); let new_slice = value_merger.merge_values( len_not_equals_capacity, len_equals_capacity, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index fa8b0f97d7..c95e946575 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -141,7 +141,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, cfg::ControlFlowGraph, - dfg::{CallStack, DataFlowGraph, InsertInstructionResult}, + dfg::{CallStack, InsertInstructionResult}, function::Function, function_inserter::FunctionInserter, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, @@ -152,7 +152,9 @@ use crate::ssa::{ }; mod branch_analysis; -// mod value_merger; +pub(crate) mod value_merger; + +use value_merger::ValueMerger; impl Ssa { /// Flattens the control flow graph of main such that the function is left with a @@ -206,9 +208,6 @@ struct Context<'f> { /// condition. If we are under multiple conditions (a nested if), the topmost condition is /// the most recent condition combined with all previous conditions via `And` instructions. conditions: Vec<(BasicBlockId, ValueId)>, - - slice_sizes: HashMap, - // value_merger: ValueMergerNew<'f>, } #[derive(Debug, Clone)] @@ -242,45 +241,11 @@ fn flatten_function_cfg(function: &mut Function) { branch_ends, conditions: Vec::new(), outer_block_stores: HashMap::default(), - slice_sizes: HashMap::default(), }; - // let mut context = Context::new(function); context.flatten(); } impl<'f> Context<'f> { - // fn new(function: &'f mut Function) -> Self { - // let cfg = ControlFlowGraph::with_function(function); - // let branch_ends = branch_analysis::find_branch_ends(function, &cfg); - - // let inserter = FunctionInserter::new(function); - - // let store_values = HashMap::default(); - // let outer_block_stores = HashMap::default(); - // let mut slice_sizes = HashMap::default(); - - // let block = inserter.function.entry_block(); - // let value_merger = ValueMergerNew { - // dfg: &mut inserter.function.dfg, - // block, - // store_values: &store_values, - // outer_block_stores: &outer_block_stores, - // slice_sizes: &mut slice_sizes, - // }; - - // Context { - // inserter, - // cfg, - // store_values, - // local_allocations: HashSet::new(), - // branch_ends, - // conditions: Vec::new(), - // outer_block_stores, - // slice_sizes, - // value_merger, - // } - // } - fn flatten(&mut self) { // Start with following the terminator of the entry block since we don't // need to flatten the entry block into itself. @@ -440,290 +405,6 @@ impl<'f> Context<'f> { self.insert_instruction_with_typevars(enable_side_effects, None); } - /// Merge two values a and b from separate basic blocks to a single value. - /// If these two values are numeric, the result will be - /// `then_condition * then_value + else_condition * else_value`. - /// Otherwise, if the values being merged are arrays, a new array will be made - /// recursively from combining each element of both input arrays. - /// - /// It is currently an error to call this function on reference or function values - /// as it is less clear how to merge these. - fn merge_values( - &mut self, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - match self.inserter.function.dfg.type_of_value(then_value) { - Type::Numeric(_) => { - self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - // merge_numeric_values_pub( - // then_condition, - // else_condition, - // then_value, - // else_value, - // self.inserter.function.entry_block(), - // &mut self.inserter.function.dfg, - // ) - } - typ @ Type::Array(_, _) => { - self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) - } - typ @ Type::Slice(_) => { - self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) - } - Type::Reference => panic!("Cannot return references from an if expression"), - Type::Function => panic!("Cannot return functions from an if expression"), - } - } - - fn merge_slice_values( - &mut self, - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value_id: ValueId, - else_value_id: ValueId, - ) -> ValueId { - let mut merged = im::Vector::new(); - - let element_types = match &typ { - Type::Slice(elements) => elements, - _ => panic!("Expected slice type"), - }; - - dbg!(then_value_id); - let then_len = self.get_slice_length(then_value_id); - dbg!(then_len); - dbg!(self.slice_sizes.insert(then_value_id, then_len)); - - dbg!(else_value_id); - let else_len = self.get_slice_length(else_value_id); - dbg!(else_len); - dbg!(self.slice_sizes.insert(else_value_id, else_len)); - - let len = then_len.max(else_len); - - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index_value = ((i * element_types.len() + element_index) as u128).into(); - let index = self.inserter.function.dfg.make_constant(index_value, Type::field()); - - let typevars = Some(vec![element_type.clone()]); - - let mut get_element = |array, typevars, len| { - // The smaller slice is filled with placeholder data. Codegen for slice accesses must - // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. - if (len - 1) < index_value.to_u128() as usize { - let zero = FieldElement::zero(); - self.inserter.function.dfg.make_constant(zero, Type::field()) - } else { - let get = Instruction::ArrayGet { array, index }; - self.insert_instruction_with_typevars(get, typevars).first() - } - }; - - let then_element = get_element(then_value_id, typevars.clone(), then_len); - let else_element = get_element(else_value_id, typevars, else_len); - - merged.push_back(self.merge_values( - then_condition, - else_condition, - then_element, - else_element, - )); - } - } - dbg!(merged.len()); - let merged = self.inserter.function.dfg.make_array(merged, typ); - dbg!(merged); - merged - } - - fn get_slice_length(&mut self, value_id: ValueId) -> usize { - let value = &self.inserter.function.dfg[value_id]; - match value { - Value::Array { array, .. } => array.len(), - Value::Instruction { instruction: instruction_id, .. } => { - let instruction = &self.inserter.function.dfg[*instruction_id]; - match instruction { - Instruction::ArraySet { array, .. } => { - dbg!("got an array set"); - let array = *array; - let len = self.get_slice_length(array); - dbg!(self.slice_sizes.insert(array, len)); - len - } - Instruction::Load { address } => { - let outer_store = self - .outer_block_stores - .get(address) - .expect("ICE: load in merger should have store from outer block"); - dbg!(self.slice_sizes.get(&outer_store.new_value)); - if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { - dbg!(outer_store.new_value); - dbg!(len); - return *len; - } - - let context_store = if let Some(store) = self.store_values.get(address) { - if let Some(len) = self.slice_sizes.get(&store.new_value) { - return *len; - } - - store - } else { - // let outer_store = self - // .outer_block_stores - // .get(address) - // .expect("ICE: load in merger should have store from outer block"); - - // if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { - // return *len; - // } - outer_store - }; - - self.get_slice_length(context_store.new_value) - } - Instruction::Call { func, arguments } => { - let func = &self.inserter.function.dfg[*func]; - let slice_contents = arguments[1]; - match func { - Value::Intrinsic(intrinsic) => match intrinsic { - Intrinsic::SlicePushBack - | Intrinsic::SlicePushFront - | Intrinsic::SliceInsert => { - dbg!("about to call get_slice_length"); - let initial_len = self.get_slice_length(slice_contents); - dbg!(self.slice_sizes.insert(slice_contents, initial_len)); - initial_len + 1 - } - Intrinsic::SlicePopBack - | Intrinsic::SlicePopFront - | Intrinsic::SliceRemove => { - // TODO: for some reason this breaks lol - // let initial_len = self.get_slice_length(slice_contents); - // self.slice_sizes.insert(slice_contents, initial_len); - // initial_len + 1 - self.get_slice_length(slice_contents) - 1 - } - _ => { - unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") - } - }, - _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), - } - } - _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), - } - } - _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), - } - } - - /// Given an if expression that returns an array: `if c { array1 } else { array2 }`, - /// this function will recursively merge array1 and array2 into a single resulting array - /// by creating a new array containing the result of self.merge_values for each element. - fn merge_array_values( - &mut self, - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - let mut merged = im::Vector::new(); - - let (element_types, len) = match &typ { - Type::Array(elements, len) => (elements, *len), - _ => panic!("Expected array type"), - }; - - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index = ((i * element_types.len() + element_index) as u128).into(); - let index = self.inserter.function.dfg.make_constant(index, Type::field()); - - let typevars = Some(vec![element_type.clone()]); - - let mut get_element = |array, typevars| { - let get = Instruction::ArrayGet { array, index }; - self.insert_instruction_with_typevars(get, typevars).first() - }; - - let then_element = get_element(then_value, typevars.clone()); - let else_element = get_element(else_value, typevars); - - merged.push_back(self.merge_values( - then_condition, - else_condition, - then_element, - else_element, - )); - } - } - - self.inserter.function.dfg.make_array(merged, typ) - } - - /// Merge two numeric values a and b from separate basic blocks to a single value. This - /// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. - fn merge_numeric_values( - &mut self, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - let block = self.inserter.function.entry_block(); - let then_type = self.inserter.function.dfg.type_of_value(then_value); - let else_type = self.inserter.function.dfg.type_of_value(else_value); - assert_eq!( - then_type, else_type, - "Expected values merged to be of the same type but found {then_type} and {else_type}" - ); - - let then_call_stack = self.inserter.function.dfg.get_value_call_stack(then_value); - let else_call_stack = self.inserter.function.dfg.get_value_call_stack(else_value); - - let call_stack = if then_call_stack.is_empty() { - else_call_stack.clone() - } else { - then_call_stack.clone() - }; - - // We must cast the bool conditions to the actual numeric type used by each value. - let then_condition = - self.insert_instruction(Instruction::Cast(then_condition, then_type), then_call_stack); - let else_condition = - self.insert_instruction(Instruction::Cast(else_condition, else_type), else_call_stack); - - let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); - let then_value = self - .inserter - .function - .dfg - .insert_instruction_and_results(mul, block, None, call_stack.clone()) - .first(); - - let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); - let else_value = self - .inserter - .function - .dfg - .insert_instruction_and_results(mul, block, None, call_stack.clone()) - .first(); - - let add = Instruction::binary(BinaryOp::Add, then_value, else_value); - self.inserter - .function - .dfg - .insert_instruction_and_results(add, block, None, call_stack) - .first() - } - /// Inline one branch of a jmpif instruction. /// /// This will continue inlining recursively until the next end block is reached where each branch @@ -817,13 +498,12 @@ impl<'f> Context<'f> { }); let block = self.inserter.function.entry_block(); - let mut value_merger = ValueMerger { - dfg: &mut self.inserter.function.dfg, + let mut value_merger = ValueMerger::new( + &mut self.inserter.function.dfg, block, - store_values: &self.store_values, - outer_block_stores: &self.outer_block_stores, - slice_sizes: &mut self.slice_sizes, - }; + &self.store_values, + &self.outer_block_stores, + ); // Cannot include this in the previous vecmap since it requires exclusive access to self let args = vecmap(args, |(then_arg, else_arg)| { @@ -866,13 +546,13 @@ impl<'f> Context<'f> { let else_condition = else_branch.condition; let block = self.inserter.function.entry_block(); - let mut value_merger = ValueMerger { - dfg: &mut self.inserter.function.dfg, + + let mut value_merger = ValueMerger::new( + &mut self.inserter.function.dfg, block, - store_values: &self.store_values, - outer_block_stores: &self.outer_block_stores, - slice_sizes: &mut self.slice_sizes, - }; + &self.store_values, + &self.outer_block_stores, + ); // Merging must occur in a separate loop as we cannot `*self` as mutable more than one at a time let mut new_values = HashMap::default(); @@ -1004,301 +684,6 @@ impl<'f> Context<'f> { } } -pub(crate) struct ValueMerger<'a> { - dfg: &'a mut DataFlowGraph, - block: BasicBlockId, - store_values: &'a HashMap, - outer_block_stores: &'a HashMap, - slice_sizes: &'a mut HashMap, -} - -impl<'a> ValueMerger<'a> { - pub(crate) fn new( - dfg: &'a mut DataFlowGraph, - block: BasicBlockId, - store_values: &'a HashMap, - outer_block_stores: &'a HashMap, - slice_sizes: &'a mut HashMap, - ) -> Self { - // let store_values = Box::new(HashMap::default()); - ValueMerger { dfg, block, store_values, outer_block_stores, slice_sizes } - } - - pub(crate) fn merge_values( - &mut self, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - match self.dfg.type_of_value(then_value) { - Type::Numeric(_) => { - self.merge_numeric_values(then_condition, else_condition, then_value, else_value) - } - typ @ Type::Array(_, _) => { - self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) - } - typ @ Type::Slice(_) => { - self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) - } - Type::Reference => panic!("Cannot return references from an if expression"), - Type::Function => panic!("Cannot return functions from an if expression"), - } - } - - /// Merge two numeric values a and b from separate basic blocks to a single value. This - /// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. - pub(crate) fn merge_numeric_values( - &mut self, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - let then_type = self.dfg.type_of_value(then_value); - let else_type = self.dfg.type_of_value(else_value); - assert_eq!( - then_type, else_type, - "Expected values merged to be of the same type but found {then_type} and {else_type}" - ); - - let then_call_stack = self.dfg.get_value_call_stack(then_value); - let else_call_stack = self.dfg.get_value_call_stack(else_value); - - let call_stack = if then_call_stack.is_empty() { - else_call_stack.clone() - } else { - then_call_stack.clone() - }; - - // We must cast the bool conditions to the actual numeric type used by each value. - let then_condition = self - .dfg - .insert_instruction_and_results( - Instruction::Cast(then_condition, then_type), - self.block, - None, - call_stack.clone(), - ) - .first(); - let else_condition = self - .dfg - .insert_instruction_and_results( - Instruction::Cast(else_condition, else_type), - self.block, - None, - call_stack.clone(), - ) - .first(); - - let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); - let then_value = self - .dfg - .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) - .first(); - - let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); - let else_value = self - .dfg - .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) - .first(); - - let add = Instruction::binary(BinaryOp::Add, then_value, else_value); - self.dfg.insert_instruction_and_results(add, self.block, None, call_stack).first() - } - - /// Given an if expression that returns an array: `if c { array1 } else { array2 }`, - /// this function will recursively merge array1 and array2 into a single resulting array - /// by creating a new array containing the result of self.merge_values for each element. - pub(crate) fn merge_array_values( - &mut self, - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value: ValueId, - else_value: ValueId, - ) -> ValueId { - let mut merged = im::Vector::new(); - - let (element_types, len) = match &typ { - Type::Array(elements, len) => (elements, *len), - _ => panic!("Expected array type"), - }; - - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index = ((i * element_types.len() + element_index) as u128).into(); - let index = self.dfg.make_constant(index, Type::field()); - - let typevars = Some(vec![element_type.clone()]); - - let mut get_element = |array, typevars| { - let get = Instruction::ArrayGet { array, index }; - self.dfg - .insert_instruction_and_results(get, self.block, typevars, CallStack::new()) - .first() - }; - - let then_element = get_element(then_value, typevars.clone()); - let else_element = get_element(else_value, typevars); - - merged.push_back(self.merge_values( - then_condition, - else_condition, - then_element, - else_element, - )) - } - } - - self.dfg.make_array(merged, typ) - } - - fn merge_slice_values( - &mut self, - typ: Type, - then_condition: ValueId, - else_condition: ValueId, - then_value_id: ValueId, - else_value_id: ValueId, - ) -> ValueId { - let mut merged = im::Vector::new(); - - let element_types = match &typ { - Type::Slice(elements) => elements, - _ => panic!("Expected slice type"), - }; - - dbg!(then_value_id); - let then_len = self.get_slice_length(then_value_id); - dbg!(then_len); - dbg!(self.slice_sizes.insert(then_value_id, then_len)); - - dbg!(else_value_id); - let else_len = self.get_slice_length(else_value_id); - dbg!(else_len); - dbg!(self.slice_sizes.insert(else_value_id, else_len)); - - let len = then_len.max(else_len); - - for i in 0..len { - for (element_index, element_type) in element_types.iter().enumerate() { - let index_value = ((i * element_types.len() + element_index) as u128).into(); - let index = self.dfg.make_constant(index_value, Type::field()); - - let typevars = Some(vec![element_type.clone()]); - - let mut get_element = |array, typevars, len| { - // The smaller slice is filled with placeholder data. Codegen for slice accesses must - // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. - if (len - 1) < index_value.to_u128() as usize { - let zero = FieldElement::zero(); - self.dfg.make_constant(zero, Type::field()) - } else { - let get = Instruction::ArrayGet { array, index }; - self.dfg - .insert_instruction_and_results( - get, - self.block, - typevars, - CallStack::new(), - ) - .first() - } - }; - - let then_element = get_element(then_value_id, typevars.clone(), then_len); - let else_element = get_element(else_value_id, typevars, else_len); - - merged.push_back(self.merge_values( - then_condition, - else_condition, - then_element, - else_element, - )); - } - } - dbg!(merged.len()); - let merged = self.dfg.make_array(merged, typ); - dbg!(merged); - merged - } - - fn get_slice_length(&mut self, value_id: ValueId) -> usize { - let value = &self.dfg[value_id]; - match value { - Value::Array { array, .. } => array.len(), - Value::Instruction { instruction: instruction_id, .. } => { - let instruction = &self.dfg[*instruction_id]; - match instruction { - Instruction::ArraySet { array, .. } => { - dbg!("got an array set"); - let array = *array; - let len = self.get_slice_length(array); - dbg!(self.slice_sizes.insert(array, len)); - len - } - Instruction::Load { address } => { - let outer_store = self - .outer_block_stores - .get(address) - .expect("ICE: load in merger should have store from outer block"); - dbg!(self.slice_sizes.get(&outer_store.new_value)); - if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { - dbg!(outer_store.new_value); - dbg!(len); - return *len; - } - - let context_store = if let Some(store) = self.store_values.get(address) { - if let Some(len) = self.slice_sizes.get(&store.new_value) { - return *len; - } - - store - } else { - outer_store - }; - - self.get_slice_length(context_store.new_value) - } - Instruction::Call { func, arguments } => { - let func = &self.dfg[*func]; - let slice_contents = arguments[1]; - match func { - Value::Intrinsic(intrinsic) => match intrinsic { - Intrinsic::SlicePushBack - | Intrinsic::SlicePushFront - | Intrinsic::SliceInsert => { - dbg!("about to call get_slice_length"); - let initial_len = self.get_slice_length(slice_contents); - dbg!(self.slice_sizes.insert(slice_contents, initial_len)); - initial_len + 1 - } - Intrinsic::SlicePopBack - | Intrinsic::SlicePopFront - | Intrinsic::SliceRemove => { - // TODO: for some reason this breaks lol - // let initial_len = self.get_slice_length(slice_contents); - // self.slice_sizes.insert(slice_contents, initial_len); - // initial_len + 1 - self.get_slice_length(slice_contents) - 1 - } - _ => { - unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") - } - }, - _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), - } - } - _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), - } - } - _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), - } - } -} - #[cfg(test)] mod test { use std::rc::Rc; diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs new file mode 100644 index 0000000000..fb2427335c --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -0,0 +1,319 @@ +use acvm::FieldElement; +use fxhash::FxHashMap as HashMap; + +use crate::ssa::ir::{ + basic_block::BasicBlockId, + dfg::{CallStack, DataFlowGraph}, + instruction::{BinaryOp, Instruction, Intrinsic}, + types::Type, + value::{Value, ValueId}, +}; + +use crate::ssa::opt::flatten_cfg::Store; + +pub(crate) struct ValueMerger<'a> { + dfg: &'a mut DataFlowGraph, + block: BasicBlockId, + store_values: &'a HashMap, + outer_block_stores: &'a HashMap, + slice_sizes: HashMap, +} + +impl<'a> ValueMerger<'a> { + pub(crate) fn new( + dfg: &'a mut DataFlowGraph, + block: BasicBlockId, + store_values: &'a HashMap, + outer_block_stores: &'a HashMap, + ) -> Self { + ValueMerger { + dfg, + block, + store_values, + outer_block_stores, + slice_sizes: HashMap::default(), + } + } + + /// Merge two values a and b from separate basic blocks to a single value. + /// If these two values are numeric, the result will be + /// `then_condition * then_value + else_condition * else_value`. + /// Otherwise, if the values being merged are arrays, a new array will be made + /// recursively from combining each element of both input arrays. + /// + /// It is currently an error to call this function on reference or function values + /// as it is less clear how to merge these. + pub(crate) fn merge_values( + &mut self, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + match self.dfg.type_of_value(then_value) { + Type::Numeric(_) => { + self.merge_numeric_values(then_condition, else_condition, then_value, else_value) + } + typ @ Type::Array(_, _) => { + self.merge_array_values(typ, then_condition, else_condition, then_value, else_value) + } + typ @ Type::Slice(_) => { + self.merge_slice_values(typ, then_condition, else_condition, then_value, else_value) + } + Type::Reference => panic!("Cannot return references from an if expression"), + Type::Function => panic!("Cannot return functions from an if expression"), + } + } + + /// Merge two numeric values a and b from separate basic blocks to a single value. This + /// function would return the result of `if c { a } else { b }` as `c*a + (!c)*b`. + pub(crate) fn merge_numeric_values( + &mut self, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + let then_type = self.dfg.type_of_value(then_value); + let else_type = self.dfg.type_of_value(else_value); + assert_eq!( + then_type, else_type, + "Expected values merged to be of the same type but found {then_type} and {else_type}" + ); + + let then_call_stack = self.dfg.get_value_call_stack(then_value); + let else_call_stack = self.dfg.get_value_call_stack(else_value); + + let call_stack = if then_call_stack.is_empty() { + else_call_stack.clone() + } else { + then_call_stack.clone() + }; + + // We must cast the bool conditions to the actual numeric type used by each value. + let then_condition = self + .dfg + .insert_instruction_and_results( + Instruction::Cast(then_condition, then_type), + self.block, + None, + call_stack.clone(), + ) + .first(); + let else_condition = self + .dfg + .insert_instruction_and_results( + Instruction::Cast(else_condition, else_type), + self.block, + None, + call_stack.clone(), + ) + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, then_condition, then_value); + let then_value = self + .dfg + .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) + .first(); + + let mul = Instruction::binary(BinaryOp::Mul, else_condition, else_value); + let else_value = self + .dfg + .insert_instruction_and_results(mul, self.block, None, call_stack.clone()) + .first(); + + let add = Instruction::binary(BinaryOp::Add, then_value, else_value); + self.dfg.insert_instruction_and_results(add, self.block, None, call_stack).first() + } + + /// Given an if expression that returns an array: `if c { array1 } else { array2 }`, + /// this function will recursively merge array1 and array2 into a single resulting array + /// by creating a new array containing the result of self.merge_values for each element. + pub(crate) fn merge_array_values( + &mut self, + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value: ValueId, + else_value: ValueId, + ) -> ValueId { + let mut merged = im::Vector::new(); + + let (element_types, len) = match &typ { + Type::Array(elements, len) => (elements, *len), + _ => panic!("Expected array type"), + }; + + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index = ((i * element_types.len() + element_index) as u128).into(); + let index = self.dfg.make_constant(index, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars| { + let get = Instruction::ArrayGet { array, index }; + self.dfg + .insert_instruction_and_results(get, self.block, typevars, CallStack::new()) + .first() + }; + + let then_element = get_element(then_value, typevars.clone()); + let else_element = get_element(else_value, typevars); + + merged.push_back(self.merge_values( + then_condition, + else_condition, + then_element, + else_element, + )) + } + } + + self.dfg.make_array(merged, typ) + } + + fn merge_slice_values( + &mut self, + typ: Type, + then_condition: ValueId, + else_condition: ValueId, + then_value_id: ValueId, + else_value_id: ValueId, + ) -> ValueId { + let mut merged = im::Vector::new(); + + let element_types = match &typ { + Type::Slice(elements) => elements, + _ => panic!("Expected slice type"), + }; + + dbg!(then_value_id); + let then_len = self.get_slice_length(then_value_id); + dbg!(then_len); + dbg!(self.slice_sizes.insert(then_value_id, then_len)); + + dbg!(else_value_id); + let else_len = self.get_slice_length(else_value_id); + dbg!(else_len); + dbg!(self.slice_sizes.insert(else_value_id, else_len)); + + let len = then_len.max(else_len); + + for i in 0..len { + for (element_index, element_type) in element_types.iter().enumerate() { + let index_value = ((i * element_types.len() + element_index) as u128).into(); + let index = self.dfg.make_constant(index_value, Type::field()); + + let typevars = Some(vec![element_type.clone()]); + + let mut get_element = |array, typevars, len| { + // The smaller slice is filled with placeholder data. Codegen for slice accesses must + // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. + if (len - 1) < index_value.to_u128() as usize { + let zero = FieldElement::zero(); + self.dfg.make_constant(zero, Type::field()) + } else { + let get = Instruction::ArrayGet { array, index }; + self.dfg + .insert_instruction_and_results( + get, + self.block, + typevars, + CallStack::new(), + ) + .first() + } + }; + + let then_element = get_element(then_value_id, typevars.clone(), then_len); + let else_element = get_element(else_value_id, typevars, else_len); + + merged.push_back(self.merge_values( + then_condition, + else_condition, + then_element, + else_element, + )); + } + } + dbg!(merged.len()); + let merged = self.dfg.make_array(merged, typ); + dbg!(merged); + merged + } + + fn get_slice_length(&mut self, value_id: ValueId) -> usize { + let value = &self.dfg[value_id]; + match value { + Value::Array { array, .. } => array.len(), + Value::Instruction { instruction: instruction_id, .. } => { + let instruction = &self.dfg[*instruction_id]; + match instruction { + Instruction::ArraySet { array, .. } => { + dbg!("got an array set"); + let array = *array; + let len = self.get_slice_length(array); + dbg!(self.slice_sizes.insert(array, len)); + len + } + Instruction::Load { address } => { + let outer_store = self + .outer_block_stores + .get(address) + .expect("ICE: load in merger should have store from outer block"); + dbg!(self.slice_sizes.get(&outer_store.new_value)); + if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { + dbg!(outer_store.new_value); + dbg!(len); + return *len; + } + + let context_store = if let Some(store) = self.store_values.get(address) { + if let Some(len) = self.slice_sizes.get(&store.new_value) { + return *len; + } + + store + } else { + outer_store + }; + + self.get_slice_length(context_store.new_value) + } + Instruction::Call { func, arguments } => { + let func = &self.dfg[*func]; + let slice_contents = arguments[1]; + match func { + Value::Intrinsic(intrinsic) => match intrinsic { + Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SliceInsert => { + dbg!("about to call get_slice_length"); + let initial_len = self.get_slice_length(slice_contents); + dbg!(self.slice_sizes.insert(slice_contents, initial_len)); + initial_len + 1 + } + Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceRemove => { + // TODO: for some reason this breaks lol + // let initial_len = self.get_slice_length(slice_contents); + // self.slice_sizes.insert(slice_contents, initial_len); + // initial_len + 1 + self.get_slice_length(slice_contents) - 1 + } + _ => { + unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") + } + }, + _ => unreachable!("ICE: Expected intrinsic value but got {func:?}"), + } + } + _ => unreachable!("ICE: Got unexpected instruction: {instruction:?}"), + } + } + _ => unreachable!("ICE: Got unexpected value when resolving slice length {value:?}"), + } + } +} From c8892f9446dca8c556078670ec31220bca894fc6 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 13 Sep 2023 21:36:39 +0000 Subject: [PATCH 07/16] update tests --- .../slice_dynamic_index/src/main.nr | 24 +++++++++---------- .../execution_success/slices/src/main.nr | 1 + 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr b/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr index e4a3111682..724f3f1618 100644 --- a/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slice_dynamic_index/src/main.nr @@ -10,18 +10,18 @@ fn regression_dynamic_slice_index(x: Field, y: Field) { } assert(slice.len() == 5); - // dynamic_slice_index_set_if(slice, x, y); - // dynamic_slice_index_set_else(slice, x, y); - // dynamic_slice_index_set_nested_if_else_else(slice, x, y); - // dynamic_slice_index_set_nested_if_else_if(slice, x, y + 1); - // dynamic_slice_index_if(slice, x); - // dynamic_array_index_if([0, 1, 2, 3, 4], x); - // dynamic_slice_index_else(slice, x); - - // dynamic_slice_merge_if(slice, x); - // dynamic_slice_merge_else(slice, x); - // dynamic_slice_merge_two_ifs(slice, x); - // dynamic_slice_merge_mutate_between_ifs(slice, x, y); + dynamic_slice_index_set_if(slice, x, y); + dynamic_slice_index_set_else(slice, x, y); + dynamic_slice_index_set_nested_if_else_else(slice, x, y); + dynamic_slice_index_set_nested_if_else_if(slice, x, y + 1); + dynamic_slice_index_if(slice, x); + dynamic_array_index_if([0, 1, 2, 3, 4], x); + dynamic_slice_index_else(slice, x); + + dynamic_slice_merge_if(slice, x); + dynamic_slice_merge_else(slice, x); + dynamic_slice_merge_two_ifs(slice, x); + dynamic_slice_merge_mutate_between_ifs(slice, x, y); dynamic_slice_merge_push_then_pop(slice, x, y); } diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 5de7a431c4..bfa6e18b2f 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -212,6 +212,7 @@ fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] { } slice = slice.push_back(30); + if x == 20 { slice = slice.push_back(20); } From c57fb975dedfecfd6841c76628b93d245cd36ce3 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 14 Sep 2023 14:07:27 +0000 Subject: [PATCH 08/16] small edits to call --- .../src/ssa/ir/instruction/call.rs | 53 ++----------------- 1 file changed, 4 insertions(+), 49 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 4ec993a83e..d22ac5a44b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -95,12 +95,8 @@ pub(super) fn simplify_call( Intrinsic::SlicePushBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((mut slice, element_type)) = slice { - // The capacity must be an integer so that we can compare it against the slice length which is represented as a field - let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); - // TODO: This is the user-facing length, we need to handle the element_type size // to appropriately handle arrays of complex types - // dbg!(element_type.element_size()); if element_type.element_size() != 1 { // Old code before implementing multiple slice mergers for elem in &arguments[2..] { @@ -113,16 +109,9 @@ pub(super) fn simplify_call( let new_slice = dfg.make_array(slice, element_type); return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]); } - // For nested arrays - // let element_size = dfg.make_constant((element_type.element_size() as u128).into(), Type::field()); - // let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size); - // let flattened_len = dfg.insert_instruction_and_results( - // flattened_len_instr, block, None, CallStack::new()).first(); - // let len_equals_capacity_instr = Instruction::Binary(Binary { - // lhs: flattened_len, - // operator: BinaryOp::Eq, - // rhs: capacity, - // }); + + // The capacity must be an integer so that we can compare it against the slice length which is represented as a field + let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); let len_equals_capacity_instr = Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, @@ -181,29 +170,6 @@ pub(super) fn simplify_call( ); SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) - // array set multiple elements for nested arrays - // let mut set_last_slice_value_instr = new_slice; - // let mut set_last_slice_value = new_slice; - // let mut flattened_index = flattened_len; - // let one = dfg.make_constant(FieldElement::one(), Type::field()); - // for elem in &arguments[2..] { - // let set_last_slice_value_instr = Instruction::ArraySet { - // array: set_last_slice_value, - // index: flattened_index, - // value: *elem, - // length: Some(new_slice_length), - // }; - // set_last_slice_value = dfg - // .insert_instruction_and_results( - // set_last_slice_value_instr, - // block, - // None, - // CallStack::new(), - // ) - // .first(); - // let flattened_index_instr = Instruction::binary(BinaryOp::Add, flattened_index, one); - // flattened_index = dfg.insert_instruction_and_results(flattened_index_instr, block, None, CallStack::new()).first(); - // } } else { SimplifyResult::None } @@ -244,6 +210,7 @@ pub(super) fn simplify_call( .first(); flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); let mut flattened_index = flattened_len; + // We must pop multiple elements in the case of a slice of tuples for _ in 0..element_count { let get_last_elem_instr = Instruction::ArrayGet { array: arguments[1], index: flattened_index }; @@ -265,18 +232,6 @@ pub(super) fn simplify_call( results.push_front(new_slice_length); SimplifyResult::SimplifiedToMultiple(results.into()) - - // Old code - // We must pop multiple elements in the case of a slice of tuples - // for _ in 0..element_count { - // let elem = slice - // .pop_back() - // .expect("There are no elements in this slice to be removed"); - // results.push_front(elem); - // } - - // let new_slice = dfg.make_array(slice, typ); - // results.push_front(new_slice); } else { SimplifyResult::None } From ff8b66526a51227a077ebbbc41ba9768d9d71f8b Mon Sep 17 00:00:00 2001 From: vezenovm Date: Thu, 14 Sep 2023 17:49:22 +0000 Subject: [PATCH 09/16] cleanup the value merger a bit but need to brainstorm more on how to remove outer_block_stores --- .../src/ssa/ir/instruction/call.rs | 4 +-- .../src/ssa/opt/flatten_cfg.rs | 15 +++++------ .../src/ssa/opt/flatten_cfg/value_merger.rs | 27 +++++++++---------- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index d22ac5a44b..6527f495e0 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -158,10 +158,8 @@ pub(super) fn simplify_call( ) .first(); - let store_values = &HashMap::default(); - let outer_block_stores = &HashMap::default(); let mut value_merger = - ValueMerger::new(dfg, block, store_values, outer_block_stores); + ValueMerger::new(dfg, block, None, None); let new_slice = value_merger.merge_values( len_not_equals_capacity, len_equals_capacity, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index c95e946575..8532f0d2a9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -192,7 +192,7 @@ struct Context<'f> { /// are overwritten as new stores are found. This overwriting is the desired behavior, /// as we want the most update to date value to be stored at a given address as /// we walk through blocks to flatten. - outer_block_stores: HashMap, + outer_block_stores: HashMap, /// Stores all allocations local to the current branch. /// Since these branches are local to the current branch (ie. only defined within one branch of @@ -275,10 +275,7 @@ impl<'f> Context<'f> { for instruction in instructions { let (instruction, _) = self.inserter.map_instruction(instruction); if let Instruction::Store { address, value } = instruction { - let load = Instruction::Load { address }; - let load_type = Some(vec![self.inserter.function.dfg.type_of_value(value)]); - let old_value = self.insert_instruction_with_typevars(load, load_type).first(); - self.outer_block_stores.insert(address, Store { old_value, new_value: value }); + self.outer_block_stores.insert(address, value); } } } @@ -501,8 +498,8 @@ impl<'f> Context<'f> { let mut value_merger = ValueMerger::new( &mut self.inserter.function.dfg, block, - &self.store_values, - &self.outer_block_stores, + Some(&self.store_values), + Some(&self.outer_block_stores), ); // Cannot include this in the previous vecmap since it requires exclusive access to self @@ -550,8 +547,8 @@ impl<'f> Context<'f> { let mut value_merger = ValueMerger::new( &mut self.inserter.function.dfg, block, - &self.store_values, - &self.outer_block_stores, + Some(&self.store_values), + Some(&self.outer_block_stores), ); // Merging must occur in a separate loop as we cannot `*self` as mutable more than one at a time diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index fb2427335c..40b0c1671f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -14,8 +14,8 @@ use crate::ssa::opt::flatten_cfg::Store; pub(crate) struct ValueMerger<'a> { dfg: &'a mut DataFlowGraph, block: BasicBlockId, - store_values: &'a HashMap, - outer_block_stores: &'a HashMap, + store_values: Option<&'a HashMap>, + outer_block_stores: Option<&'a HashMap>, slice_sizes: HashMap, } @@ -23,8 +23,8 @@ impl<'a> ValueMerger<'a> { pub(crate) fn new( dfg: &'a mut DataFlowGraph, block: BasicBlockId, - store_values: &'a HashMap, - outer_block_stores: &'a HashMap, + store_values: Option<&'a HashMap>, + outer_block_stores: Option<&'a HashMap>, ) -> Self { ValueMerger { dfg, @@ -258,28 +258,27 @@ impl<'a> ValueMerger<'a> { len } Instruction::Load { address } => { - let outer_store = self - .outer_block_stores + let outer_block_stores = self.outer_block_stores.expect("ICE: A map of previous stores is required in order to resolve a slice load"); + let store_values = self.store_values.expect("ICE: A map of previous stores is required in order to resolve a slice load"); + let store_value = outer_block_stores .get(address) .expect("ICE: load in merger should have store from outer block"); - dbg!(self.slice_sizes.get(&outer_store.new_value)); - if let Some(len) = self.slice_sizes.get(&outer_store.new_value) { - dbg!(outer_store.new_value); - dbg!(len); + + if let Some(len) = self.slice_sizes.get(store_value) { return *len; } - let context_store = if let Some(store) = self.store_values.get(address) { + let store_value = if let Some(store) = store_values.get(address) { if let Some(len) = self.slice_sizes.get(&store.new_value) { return *len; } - store + store.new_value } else { - outer_store + *store_value }; - self.get_slice_length(context_store.new_value) + self.get_slice_length(store_value) } Instruction::Call { func, arguments } => { let func = &self.dfg[*func]; From a553cce7f929c6b245c48ff83d14a7a13954e070 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 19:53:04 +0000 Subject: [PATCH 10/16] cleanup debugs and comments --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 5 ---- .../src/ssa/ir/instruction/call.rs | 8 +++--- .../src/ssa/opt/flatten_cfg.rs | 6 ++--- .../src/ssa/opt/flatten_cfg/value_merger.rs | 27 +++++++------------ .../execution_success/slices/src/main.nr | 3 +-- 5 files changed, 17 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 016df47a03..96513473be 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -699,7 +699,6 @@ impl Context { Type::Slice(typ) => { if typ.len() != 1 { // TODO(#2461) - dbg!(typ); unimplemented!( "Non-const array indices is not implemented for non-homogenous array" ); @@ -755,7 +754,6 @@ impl Context { len.to_u128() as usize } else { if let Some((array, _)) = dfg.get_array_constant(array) { - // dbg!(array.clone()); array.len() } else { panic!( @@ -763,9 +761,6 @@ impl Context { ); } } - // let len = len - // .expect("ICE: slice length should be fully tracked and constant by ACIR gen"); - // len.to_u128() as usize } _ => unreachable!("ICE - expected an array"), }; diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 6527f495e0..66d86a39e9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -95,8 +95,9 @@ pub(super) fn simplify_call( Intrinsic::SlicePushBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((mut slice, element_type)) = slice { - // TODO: This is the user-facing length, we need to handle the element_type size - // to appropriately handle arrays of complex types + // TODO(#2752): This is the user-facing length, we need to handle the element_type size + // to appropriately handle slices of complex types. This is reliant on + // dynamic indices of non-homogenous slices also being implemented. if element_type.element_size() != 1 { // Old code before implementing multiple slice mergers for elem in &arguments[2..] { @@ -158,8 +159,7 @@ pub(super) fn simplify_call( ) .first(); - let mut value_merger = - ValueMerger::new(dfg, block, None, None); + let mut value_merger = ValueMerger::new(dfg, block, None, None); let new_slice = value_merger.merge_values( len_not_equals_capacity, len_equals_capacity, diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 8532f0d2a9..8440472ce8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -210,7 +210,6 @@ struct Context<'f> { conditions: Vec<(BasicBlockId, ValueId)>, } -#[derive(Debug, Clone)] pub(crate) struct Store { old_value: ValueId, new_value: ValueId, @@ -619,11 +618,10 @@ impl<'f> Context<'f> { fn push_instruction(&mut self, id: InstructionId) { let (instruction, call_stack) = self.inserter.map_instruction(id); let instruction = self.handle_instruction_side_effects(instruction, call_stack.clone()); - let is_allocate = matches!(instruction.clone(), Instruction::Allocate); + let is_allocate = matches!(instruction, Instruction::Allocate); let entry = self.inserter.function.entry_block(); - let results = - self.inserter.push_instruction_value(instruction.clone(), id, entry, call_stack); + let results = self.inserter.push_instruction_value(instruction, id, entry, call_stack); // Remember an allocate was created local to this branch so that we do not try to merge store // values across branches for it later. diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 40b0c1671f..258dfff5d9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -188,15 +188,11 @@ impl<'a> ValueMerger<'a> { _ => panic!("Expected slice type"), }; - dbg!(then_value_id); let then_len = self.get_slice_length(then_value_id); - dbg!(then_len); - dbg!(self.slice_sizes.insert(then_value_id, then_len)); + self.slice_sizes.insert(then_value_id, then_len); - dbg!(else_value_id); let else_len = self.get_slice_length(else_value_id); - dbg!(else_len); - dbg!(self.slice_sizes.insert(else_value_id, else_len)); + self.slice_sizes.insert(else_value_id, else_len); let len = then_len.max(else_len); @@ -237,10 +233,8 @@ impl<'a> ValueMerger<'a> { )); } } - dbg!(merged.len()); - let merged = self.dfg.make_array(merged, typ); - dbg!(merged); - merged + + self.dfg.make_array(merged, typ) } fn get_slice_length(&mut self, value_id: ValueId) -> usize { @@ -281,14 +275,14 @@ impl<'a> ValueMerger<'a> { self.get_slice_length(store_value) } Instruction::Call { func, arguments } => { - let func = &self.dfg[*func]; let slice_contents = arguments[1]; + let func = &self.dfg[*func]; match func { Value::Intrinsic(intrinsic) => match intrinsic { Intrinsic::SlicePushBack | Intrinsic::SlicePushFront | Intrinsic::SliceInsert => { - dbg!("about to call get_slice_length"); + // `get_slice_length` needs to be called here as it is borrows self as mutable let initial_len = self.get_slice_length(slice_contents); dbg!(self.slice_sizes.insert(slice_contents, initial_len)); initial_len + 1 @@ -296,11 +290,10 @@ impl<'a> ValueMerger<'a> { Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceRemove => { - // TODO: for some reason this breaks lol - // let initial_len = self.get_slice_length(slice_contents); - // self.slice_sizes.insert(slice_contents, initial_len); - // initial_len + 1 - self.get_slice_length(slice_contents) - 1 + // `get_slice_length` needs to be called here as it is borrows self as mutable + let initial_len = self.get_slice_length(slice_contents); + self.slice_sizes.insert(slice_contents, initial_len); + initial_len - 1 } _ => { unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}") diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index bfa6e18b2f..453b5ccef3 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -46,7 +46,6 @@ fn main(x : Field, y : pub Field) { assert(append[0] == 1); assert(append[4] == 5); - // TODO: need to implement dynamic arrays for regression_2083(); // The parameters to this function must come from witness values (inputs to main) regression_merge_slices(x, y); @@ -88,7 +87,7 @@ fn regression_2083() { // The parameters to this function must come from witness values (inputs to main) fn regression_merge_slices(x: Field, y: Field) { merge_slices_if(x, y); - // merge_slices_else(x); + merge_slices_else(x); } fn merge_slices_if(x: Field, y: Field) { From 9d15cd42755c469ac6b2dd82dea5b07faf4addf6 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 19 Sep 2023 20:07:54 +0000 Subject: [PATCH 11/16] cleanup by moving complex slice intrinsic logic to their own methods --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 10 +- .../src/ssa/ir/instruction/call.rs | 199 +++++++++--------- .../src/ssa/opt/flatten_cfg.rs | 4 +- .../src/ssa/opt/flatten_cfg/value_merger.rs | 10 +- 4 files changed, 106 insertions(+), 117 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 96513473be..08b5c58687 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -752,14 +752,10 @@ impl Context { let len = self.acir_context.var_to_expression(length_acir_var)?.to_const(); if let Some(len) = len { len.to_u128() as usize + } else if let Some((array, _)) = dfg.get_array_constant(array) { + array.len() } else { - if let Some((array, _)) = dfg.get_array_constant(array) { - array.len() - } else { - panic!( - "ICE: slice length should be fully tracked and constant by ACIR gen" - ); - } + panic!("ICE: slice length should be fully tracked and constant by ACIR gen"); } } _ => unreachable!("ICE - expected an array"), diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 66d86a39e9..f01738d87b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -1,7 +1,6 @@ use std::{collections::VecDeque, rc::Rc}; use acvm::{acir::BlackBoxFunc, BlackBoxResolutionError, FieldElement}; -use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; use num_bigint::BigUint; @@ -95,9 +94,8 @@ pub(super) fn simplify_call( Intrinsic::SlicePushBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((mut slice, element_type)) = slice { - // TODO(#2752): This is the user-facing length, we need to handle the element_type size - // to appropriately handle slices of complex types. This is reliant on - // dynamic indices of non-homogenous slices also being implemented. + // TODO(#2752): We need to handle the element_type size to appropriately handle slices of complex types. + // This is reliant on dynamic indices of non-homogenous slices also being implemented. if element_type.element_size() != 1 { // Old code before implementing multiple slice mergers for elem in &arguments[2..] { @@ -111,63 +109,7 @@ pub(super) fn simplify_call( return SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]); } - // The capacity must be an integer so that we can compare it against the slice length which is represented as a field - let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); - let len_equals_capacity_instr = Instruction::Binary(Binary { - lhs: arguments[0], - operator: BinaryOp::Eq, - rhs: capacity, - }); - let call_stack = dfg.get_value_call_stack(arguments[0]); - let len_equals_capacity = dfg - .insert_instruction_and_results( - len_equals_capacity_instr, - block, - None, - call_stack.clone(), - ) - .first(); - let len_not_equals_capacity_instr = Instruction::Not(len_equals_capacity); - let len_not_equals_capacity = dfg - .insert_instruction_and_results( - len_not_equals_capacity_instr, - block, - None, - call_stack.clone(), - ) - .first(); - - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); - - for elem in &arguments[2..] { - slice.push_back(*elem); - } - let new_slice = dfg.make_array(slice, element_type); - - let set_last_slice_value_instr = Instruction::ArraySet { - array: new_slice, - index: arguments[0], - value: arguments[2], - length: Some(new_slice_length), - }; - let set_last_slice_value = dfg - .insert_instruction_and_results( - set_last_slice_value_instr, - block, - None, - call_stack.clone(), - ) - .first(); - - let mut value_merger = ValueMerger::new(dfg, block, None, None); - let new_slice = value_merger.merge_values( - len_not_equals_capacity, - len_equals_capacity, - set_last_slice_value, - new_slice, - ); - - SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) + simplify_slice_push_back(slice, element_type, arguments, dfg, block) } else { SimplifyResult::None } @@ -190,46 +132,7 @@ pub(super) fn simplify_call( Intrinsic::SlicePopBack => { let slice = dfg.get_array_constant(arguments[1]); if let Some((_, typ)) = slice { - let element_count = typ.element_size(); - let mut results = VecDeque::with_capacity(element_count + 1); - - let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); - - let element_size = dfg.make_constant((element_count as u128).into(), Type::field()); - let flattened_len_instr = - Instruction::binary(BinaryOp::Mul, arguments[0], element_size); - let mut flattened_len = dfg - .insert_instruction_and_results( - flattened_len_instr, - block, - None, - CallStack::new(), - ) - .first(); - flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); - let mut flattened_index = flattened_len; - // We must pop multiple elements in the case of a slice of tuples - for _ in 0..element_count { - let get_last_elem_instr = - Instruction::ArrayGet { array: arguments[1], index: flattened_index }; - let get_last_elem = dfg - .insert_instruction_and_results( - get_last_elem_instr, - block, - Some(vec![typ.clone()]), - CallStack::new(), - ) - .first(); - results.push_front(get_last_elem); - - flattened_index = - update_slice_length(flattened_index, dfg, BinaryOp::Sub, block); - } - - results.push_front(arguments[1]); - - results.push_front(new_slice_length); - SimplifyResult::SimplifiedToMultiple(results.into()) + simplify_slice_pop_back(typ, arguments, dfg, block) } else { SimplifyResult::None } @@ -336,6 +239,100 @@ fn update_slice_length( dfg.insert_instruction_and_results(instruction, block, None, call_stack).first() } +fn simplify_slice_push_back( + mut slice: im::Vector, + element_type: Type, + arguments: &[ValueId], + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> SimplifyResult { + // The capacity must be an integer so that we can compare it against the slice length which is represented as a field + let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64)); + let len_equals_capacity_instr = + Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, rhs: capacity }); + let call_stack = dfg.get_value_call_stack(arguments[0]); + let len_equals_capacity = dfg + .insert_instruction_and_results(len_equals_capacity_instr, block, None, call_stack.clone()) + .first(); + let len_not_equals_capacity_instr = Instruction::Not(len_equals_capacity); + let len_not_equals_capacity = dfg + .insert_instruction_and_results( + len_not_equals_capacity_instr, + block, + None, + call_stack.clone(), + ) + .first(); + + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Add, block); + + for elem in &arguments[2..] { + slice.push_back(*elem); + } + let new_slice = dfg.make_array(slice, element_type); + + let set_last_slice_value_instr = Instruction::ArraySet { + array: new_slice, + index: arguments[0], + value: arguments[2], + length: Some(new_slice_length), + }; + let set_last_slice_value = dfg + .insert_instruction_and_results(set_last_slice_value_instr, block, None, call_stack) + .first(); + + let mut value_merger = ValueMerger::new(dfg, block, None, None); + let new_slice = value_merger.merge_values( + len_not_equals_capacity, + len_equals_capacity, + set_last_slice_value, + new_slice, + ); + + SimplifyResult::SimplifiedToMultiple(vec![new_slice_length, new_slice]) +} + +fn simplify_slice_pop_back( + element_type: Type, + arguments: &[ValueId], + dfg: &mut DataFlowGraph, + block: BasicBlockId, +) -> SimplifyResult { + let element_count = element_type.element_size(); + let mut results = VecDeque::with_capacity(element_count + 1); + + let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block); + + let element_size = dfg.make_constant((element_count as u128).into(), Type::field()); + let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size); + let mut flattened_len = dfg + .insert_instruction_and_results(flattened_len_instr, block, None, CallStack::new()) + .first(); + flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + + // We must pop multiple elements in the case of a slice of tuples + for _ in 0..element_count { + let get_last_elem_instr = + Instruction::ArrayGet { array: arguments[1], index: flattened_len }; + let get_last_elem = dfg + .insert_instruction_and_results( + get_last_elem_instr, + block, + Some(vec![element_type.clone()]), + CallStack::new(), + ) + .first(); + results.push_front(get_last_elem); + + flattened_len = update_slice_length(flattened_len, dfg, BinaryOp::Sub, block); + } + + results.push_front(arguments[1]); + + results.push_front(new_slice_length); + SimplifyResult::SimplifiedToMultiple(results.into()) +} + /// Try to simplify this black box call. If the call can be simplified to a known value, /// that value is returned. Otherwise [`SimplifyResult::None`] is returned. fn simplify_black_box_func( diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 8440472ce8..469de4dc86 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -144,9 +144,9 @@ use crate::ssa::{ dfg::{CallStack, InsertInstructionResult}, function::Function, function_inserter::FunctionInserter, - instruction::{BinaryOp, Instruction, InstructionId, Intrinsic, TerminatorInstruction}, + instruction::{BinaryOp, Instruction, InstructionId, TerminatorInstruction}, types::Type, - value::{Value, ValueId}, + value::ValueId, }, ssa_gen::Ssa, }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index 258dfff5d9..ad553efd26 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -84,11 +84,7 @@ impl<'a> ValueMerger<'a> { let then_call_stack = self.dfg.get_value_call_stack(then_value); let else_call_stack = self.dfg.get_value_call_stack(else_value); - let call_stack = if then_call_stack.is_empty() { - else_call_stack.clone() - } else { - then_call_stack.clone() - }; + let call_stack = if then_call_stack.is_empty() { else_call_stack } else { then_call_stack }; // We must cast the bool conditions to the actual numeric type used by each value. let then_condition = self @@ -166,7 +162,7 @@ impl<'a> ValueMerger<'a> { else_condition, then_element, else_element, - )) + )); } } @@ -284,7 +280,7 @@ impl<'a> ValueMerger<'a> { | Intrinsic::SliceInsert => { // `get_slice_length` needs to be called here as it is borrows self as mutable let initial_len = self.get_slice_length(slice_contents); - dbg!(self.slice_sizes.insert(slice_contents, initial_len)); + self.slice_sizes.insert(slice_contents, initial_len); initial_len + 1 } Intrinsic::SlicePopBack From 952f9ea32868de57d9cc1ad8e7298f793106438e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 20 Sep 2023 18:07:15 +0000 Subject: [PATCH 12/16] add slice access checks on slice insert and slice remove --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 18 ++++++++++-- .../src/ssa/function_builder/mod.rs | 4 +++ compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 26 +++++++++++++++++ .../src/ssa/ir/instruction/call.rs | 16 ++++++++++ .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 29 +++++++++++++++++-- .../slice_insert_failure/Nargo.toml | 7 +++++ .../slice_insert_failure/Prover.toml | 2 ++ .../slice_insert_failure/src/main.nr | 11 +++++++ .../slice_remove_failure/Nargo.toml | 7 +++++ .../slice_remove_failure/Prover.toml | 2 ++ .../slice_remove_failure/src/main.nr | 11 +++++++ .../execution_success/slices/src/main.nr | 12 ++++---- 12 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/slice_insert_failure/src/main.nr create mode 100644 tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Prover.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/slice_remove_failure/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 08b5c58687..a05933ab38 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1300,7 +1300,13 @@ impl Context { let mut new_slice = Vector::new(); self.slice_intrinsic_input(&mut new_slice, slice)?; // TODO(#2461): make sure that we have handled nested struct inputs - new_slice.insert(index, element); + + // Constraints should be generated during SSA gen to tell the user + // they are attempting to insert at too large of an index. + // This check prevents a panic inside of the im::Vector insert method. + if index <= new_slice.len() { + new_slice.insert(index, element); + } Ok(vec![ AcirValue::Var(new_slice_length, AcirType::field()), @@ -1328,7 +1334,15 @@ impl Context { let mut new_slice = Vector::new(); self.slice_intrinsic_input(&mut new_slice, slice)?; // TODO(#2461): make sure that we have handled nested struct inputs - let removed_elem = new_slice.remove(index); + // Constraints should be generated during SSA gen to tell the user + // they are attempting to remove at too large of an index. + // This check prevents a panic inside of the im::Vector remove method. + let removed_elem = if index < new_slice.len() { + new_slice.remove(index) + } else { + // This is a dummy value which should never be used. + AcirValue::Var(slice_length, AcirType::field()) + }; Ok(vec![ AcirValue::Var(new_slice_length, AcirType::field()), diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 961ff8cc33..e315f6249c 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -343,6 +343,10 @@ impl FunctionBuilder { self.current_function.dfg.import_intrinsic(intrinsic) } + pub(crate) fn get_intrinsic_from_value(&mut self, value: ValueId) -> Option { + self.current_function.dfg.get_intrinsic_from_value(value) + } + /// Removes the given instruction from the current block or panics otherwise. pub(crate) fn remove_instruction_from_current_block(&mut self, instruction: InstructionId) { self.current_function.dfg[self.current_block].remove_instruction(instruction); diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 73914c5467..f23ce9ce14 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -269,6 +269,32 @@ impl DataFlowGraph { self.intrinsics.get(&intrinsic) } + pub(crate) fn get_intrinsic_from_value(&self, value: ValueId) -> Option { + let intrinsics = self + .intrinsics + .iter() + .filter_map( + |(intrinsic, intrinsic_value_id)| { + if value == *intrinsic_value_id { + Some(*intrinsic) + } else { + None + } + }, + ) + .collect::>(); + if intrinsics.is_empty() { + None + } else { + assert_eq!( + intrinsics.len(), + 1, + "ICE: Multiple intrinsics associated with the same ValueId" + ); + Some(intrinsics[0]) + } + } + /// Attaches results to the instruction, clearing any previous results. /// /// This does not normally need to be called manually as it is called within diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index f01738d87b..a3e081a1aa 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -167,6 +167,14 @@ pub(super) fn simplify_call( let elements = &arguments[3..]; let mut index = index.to_u128() as usize * elements.len(); + // Do not simplify the index is greater than the slice capacity + // or else we will panic inside of the im::Vector insert method + // Constraints should be generated during SSA gen to tell the user + // they are attempting to insert at too large of an index + if index > slice.len() { + return SimplifyResult::None; + } + for elem in &arguments[3..] { slice.insert(index, *elem); index += 1; @@ -188,6 +196,14 @@ pub(super) fn simplify_call( let mut results = Vec::with_capacity(element_count + 1); let index = index.to_u128() as usize * element_count; + // Do not simplify if the index is not less than the slice capacity + // or else we will panic inside of the im::Vector remove method. + // Constraints should be generated during SSA gen to tell the user + // they are attempting to remove at too large of an index. + if index >= slice.len() { + return SimplifyResult::None; + } + for _ in 0..element_count { results.push(slice.remove(index)); } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 6dacf36397..8d52a874d7 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -12,7 +12,7 @@ use noirc_frontend::{ BinaryOpKind, }; -use crate::ssa::ir::types::NumericType; +use crate::ssa::ir::{instruction::Intrinsic, types::NumericType}; use self::{ context::FunctionContext, @@ -496,11 +496,36 @@ impl<'a> FunctionContext<'a> { /// and intrinsics are also represented by the function call instruction. fn codegen_call(&mut self, call: &ast::Call) -> Values { let function = self.codegen_non_tuple_expression(&call.func); + // dbg!(call.func.clone()); + // dbg!(function); + // let slice_insert = self + let arguments = call .arguments .iter() .flat_map(|argument| self.codegen_expression(argument).into_value_list(self)) - .collect(); + .collect::>(); + + if let Some(intrinsic) = + self.builder.set_location(call.location).get_intrinsic_from_value(function) + { + match intrinsic { + Intrinsic::SliceInsert => { + let one = self.builder.numeric_constant(1u128, Type::field()); + // We add one here in the case of a slice insert as a slice insert at the length of the slice + // can be converted to a slice push back + let len_plus_one = self.builder.insert_binary(arguments[0], BinaryOp::Add, one); + + self.codegen_slice_access_check(arguments[2], Some(len_plus_one)); + } + Intrinsic::SliceRemove => { + self.codegen_slice_access_check(arguments[2], Some(arguments[0])); + } + _ => { + // Do nothing as the other intrinsics do not require checks + } + } + } self.insert_call(function, arguments, &call.return_type, call.location) } diff --git a/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Nargo.toml new file mode 100644 index 0000000000..fc7eb87be4 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_insert_failure" +type = "bin" +authors = [""] +compiler_version = "0.12.0" + +[dependencies] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Prover.toml b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Prover.toml new file mode 100644 index 0000000000..f28f2f8cc4 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/src/main.nr b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/src/main.nr new file mode 100644 index 0000000000..dad80bff7a --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_insert_failure/src/main.nr @@ -0,0 +1,11 @@ +fn main(x : Field, y : pub Field) { + let mut slice = [0; 2]; + if x == y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + slice = slice.insert(10, 100); +} \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Nargo.toml new file mode 100644 index 0000000000..795a4d863c --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "slice_remove_failure" +type = "bin" +authors = [""] +compiler_version = "0.12.0" + +[dependencies] \ No newline at end of file diff --git a/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Prover.toml b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Prover.toml new file mode 100644 index 0000000000..f28f2f8cc4 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/src/main.nr b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/src/main.nr new file mode 100644 index 0000000000..7c308488ae --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/slice_remove_failure/src/main.nr @@ -0,0 +1,11 @@ +fn main(x : Field, y : pub Field) { + let mut slice = [0; 2]; + if x == y { + slice = slice.push_back(y); + slice = slice.push_back(x); + } else { + slice = slice.push_back(x); + } + + let (removed_slice, removed_elem) = slice.remove(10); +} \ No newline at end of file diff --git a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr index 453b5ccef3..29fee0df4f 100644 --- a/tooling/nargo_cli/tests/execution_success/slices/src/main.nr +++ b/tooling/nargo_cli/tests/execution_success/slices/src/main.nr @@ -87,7 +87,7 @@ fn regression_2083() { // The parameters to this function must come from witness values (inputs to main) fn regression_merge_slices(x: Field, y: Field) { merge_slices_if(x, y); - merge_slices_else(x); + // merge_slices_else(x); } fn merge_slices_if(x: Field, y: Field) { @@ -120,9 +120,11 @@ fn merge_slices_if(x: Field, y: Field) { merge_slices_push_then_pop(x, y); let slice = merge_slices_push_then_insert(x, y); + assert(slice.len() == 7); assert(slice[1] == 50); assert(slice[2] == 0); assert(slice[5] == 30); + assert(slice[6] == 100); let slice = merge_slices_remove_between_ifs(x, y); assert(slice.len() == 5); @@ -267,10 +269,10 @@ fn merge_slices_push_then_insert(x: Field, y: Field) -> [Field] { slice = slice.push_back(15); } - // TODO: need to constrain no insert that is greater than the length of the slice - // Currently, we can potentially insert into placeholder information that is greater than the slice length - // and we will end up with the last elem being zero rather than the elem we are inserting - let slice = slice.insert(1, 50); + slice = slice.insert(1, 50); + + // Test that we can use slice insert the same as slice push back + slice = slice.insert(6, 100); slice } From f4cbb90db70cce0b400a42aace3d7e9370f7fcbf Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 20 Sep 2023 20:21:19 +0000 Subject: [PATCH 13/16] cleanup remove and insert access checks --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 16 +++++++++++++--- .../src/ssa/opt/flatten_cfg/value_merger.rs | 3 +-- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 19 ++++++++++++------- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a05933ab38..029c86617d 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1299,12 +1299,16 @@ impl Context { let mut new_slice = Vector::new(); self.slice_intrinsic_input(&mut new_slice, slice)?; - // TODO(#2461): make sure that we have handled nested struct inputs + // We do not return an index out of bounds error directly here + // as the length of the slice is dynamic, and length of `new_slice` + // represents the capacity of the slice, not the actual length. + // // Constraints should be generated during SSA gen to tell the user // they are attempting to insert at too large of an index. // This check prevents a panic inside of the im::Vector insert method. if index <= new_slice.len() { + // TODO(#2461): make sure that we have handled nested struct inputs new_slice.insert(index, element); } @@ -1333,14 +1337,20 @@ impl Context { let mut new_slice = Vector::new(); self.slice_intrinsic_input(&mut new_slice, slice)?; - // TODO(#2461): make sure that we have handled nested struct inputs + + // We do not return an index out of bounds error directly here + // as the length of the slice is dynamic, and length of `new_slice` + // represents the capacity of the slice, not the actual length. + // // Constraints should be generated during SSA gen to tell the user // they are attempting to remove at too large of an index. // This check prevents a panic inside of the im::Vector remove method. let removed_elem = if index < new_slice.len() { + // TODO(#2461): make sure that we have handled nested struct inputs new_slice.remove(index) } else { - // This is a dummy value which should never be used. + // This is a dummy value which should never be used if the appropriate + // slice access checks are generated before this slice remove call. AcirValue::Var(slice_length, AcirType::field()) }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs index ad553efd26..8deb74b7b8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg/value_merger.rs @@ -241,10 +241,9 @@ impl<'a> ValueMerger<'a> { let instruction = &self.dfg[*instruction_id]; match instruction { Instruction::ArraySet { array, .. } => { - dbg!("got an array set"); let array = *array; let len = self.get_slice_length(array); - dbg!(self.slice_sizes.insert(array, len)); + self.slice_sizes.insert(array, len); len } Instruction::Load { address } => { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 8d52a874d7..f3acf534d3 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -496,18 +496,25 @@ impl<'a> FunctionContext<'a> { /// and intrinsics are also represented by the function call instruction. fn codegen_call(&mut self, call: &ast::Call) -> Values { let function = self.codegen_non_tuple_expression(&call.func); - // dbg!(call.func.clone()); - // dbg!(function); - // let slice_insert = self - let arguments = call .arguments .iter() .flat_map(|argument| self.codegen_expression(argument).into_value_list(self)) .collect::>(); + self.codegen_intrinsic_call_checks(function, &arguments, call.location); + + self.insert_call(function, arguments, &call.return_type, call.location) + } + + fn codegen_intrinsic_call_checks( + &mut self, + function: ValueId, + arguments: &[ValueId], + location: Location, + ) { if let Some(intrinsic) = - self.builder.set_location(call.location).get_intrinsic_from_value(function) + self.builder.set_location(location).get_intrinsic_from_value(function) { match intrinsic { Intrinsic::SliceInsert => { @@ -526,8 +533,6 @@ impl<'a> FunctionContext<'a> { } } } - - self.insert_call(function, arguments, &call.return_type, call.location) } /// Generate SSA for the given variable. From 597a53735158a5897dda773e695207c42a435d23 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 25 Sep 2023 21:30:12 +0000 Subject: [PATCH 14/16] merge w/ master, fix up pop_back simplification --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index b8a3ea75b9..abde2c484f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1017,7 +1017,7 @@ impl Context { found: array_typ.to_string(), call_stack: self.acir_context.get_call_stack(), } - .into()) + .into()); } } // We do not have to initialize the last elem size value as that is the maximum array size diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 04210b1bc4..b240680c0b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -326,6 +326,13 @@ fn simplify_slice_pop_back( dfg: &mut DataFlowGraph, block: BasicBlockId, ) -> SimplifyResult { + let element_types = match element_type.clone() { + Type::Slice(element_types) | Type::Array(element_types, _) => element_types, + _ => { + unreachable!("ICE: Expected slice or array, but got {element_type}"); + } + }; + let element_count = element_type.element_size(); let mut results = VecDeque::with_capacity(element_count + 1); @@ -346,7 +353,7 @@ fn simplify_slice_pop_back( .insert_instruction_and_results( get_last_elem_instr, block, - Some(vec![element_type.clone()]), + Some(element_types.to_vec()), CallStack::new(), ) .first(); From 274173eda26083818a3528ddd2a9479d59c7a860 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Mon, 2 Oct 2023 17:38:51 +0000 Subject: [PATCH 15/16] fetch intrinsic using dfg --- .../src/ssa/function_builder/mod.rs | 5 +++- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 26 ------------------- .../src/ssa/ir/instruction/call.rs | 7 ++--- 3 files changed, 6 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index fc0f482371..3aa95e9f6e 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -343,7 +343,10 @@ impl FunctionBuilder { } pub(crate) fn get_intrinsic_from_value(&mut self, value: ValueId) -> Option { - self.current_function.dfg.get_intrinsic_from_value(value) + match self.current_function.dfg[value] { + Value::Intrinsic(intrinsic) => Some(intrinsic), + _ => None, + } } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 13521aac59..3cb6736007 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -266,32 +266,6 @@ impl DataFlowGraph { self.intrinsics.get(&intrinsic) } - pub(crate) fn get_intrinsic_from_value(&self, value: ValueId) -> Option { - let intrinsics = self - .intrinsics - .iter() - .filter_map( - |(intrinsic, intrinsic_value_id)| { - if value == *intrinsic_value_id { - Some(*intrinsic) - } else { - None - } - }, - ) - .collect::>(); - if intrinsics.is_empty() { - None - } else { - assert_eq!( - intrinsics.len(), - 1, - "ICE: Multiple intrinsics associated with the same ValueId" - ); - Some(intrinsics[0]) - } - } - /// Attaches results to the instruction, clearing any previous results. /// /// This does not normally need to be called manually as it is called within diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index a06c64a570..26d6739902 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -299,11 +299,8 @@ fn simplify_slice_push_back( } let new_slice = dfg.make_array(slice, element_type); - let set_last_slice_value_instr = Instruction::ArraySet { - array: new_slice, - index: arguments[0], - value: arguments[2], - }; + let set_last_slice_value_instr = + Instruction::ArraySet { array: new_slice, index: arguments[0], value: arguments[2] }; let set_last_slice_value = dfg .insert_instruction_and_results(set_last_slice_value_instr, block, None, call_stack) .first(); From f9e3db28a20d9ab9088b226160a8033d0416fa1f Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 2 Oct 2023 18:39:25 +0100 Subject: [PATCH 16/16] Update compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs Co-authored-by: jfecher --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 82c2b9e5fc..8d3fda3cfe 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -547,7 +547,7 @@ impl<'f> Context<'f> { Some(&self.outer_block_stores), ); - // Merging must occur in a separate loop as we cannot `*self` as mutable more than one at a time + // Merging must occur in a separate loop as we cannot borrow `self` as mutable while `value_merger` does let mut new_values = HashMap::default(); for (address, (then_case, else_case, _)) in &new_map { let value =