From c08a36ddf099a8f61d7f914d5d2e81d916e3a514 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 23 Jan 2025 15:28:58 +0000 Subject: [PATCH 1/4] Reproduce error in a test --- .../src/ssa/opt/preprocess_fns.rs | 26 ++- .../regression_11294/Nargo.toml | 7 + .../regression_11294/Prover.json | 56 ++++++ .../regression_11294/src/main.nr | 186 ++++++++++++++++++ 4 files changed, 260 insertions(+), 15 deletions(-) create mode 100644 test_programs/execution_success/regression_11294/Nargo.toml create mode 100644 test_programs/execution_success/regression_11294/Prover.json create mode 100644 test_programs/execution_success/regression_11294/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index a2011eb5ecc..ae20c9b8b4a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -13,12 +13,6 @@ impl Ssa { // Bottom-up order, starting with the "leaf" functions, so we inline already optimized code into the ones that call them. let bottom_up = inlining::compute_bottom_up_order(&self); - // As a heuristic to avoid optimizing functions near the entry point, find a cutoff weight. - let total_weight = - bottom_up.iter().fold(0usize, |acc, (_, (_, w))| (acc.saturating_add(*w))); - let mean_weight = total_weight / bottom_up.len(); - let cutoff_weight = mean_weight; - // Preliminary inlining decisions. let inline_infos = inlining::compute_inline_infos(&self, false, aggressiveness); @@ -36,19 +30,21 @@ impl Ssa { }; for (id, (own_weight, transitive_weight)) in bottom_up { - // Skip preprocessing heavy functions that gained most of their weight from transitive accumulation. + let function = &self.functions[&id]; + + // Skip preprocessing heavy functions that gained most of their weight from transitive accumulation, which tend to be near the entry. // These can be processed later by the regular SSA passes. - if transitive_weight >= cutoff_weight && transitive_weight > own_weight * 2 { - continue; - } + let is_heavy = transitive_weight > own_weight * 10; + // Functions which are inline targets will be processed in later passes. // Here we want to treat the functions which will be inlined into them. - if let Some(info) = inline_infos.get(&id) { - if info.is_inline_target() { - continue; - } + let is_target = + inline_infos.get(&id).map(|info| info.is_inline_target()).unwrap_or_default(); + + if is_heavy || is_target { + continue; } - let function = &self.functions[&id]; + // Start with an inline pass. let mut function = function.inlined(&self, &should_inline_call); // Help unrolling determine bounds. diff --git a/test_programs/execution_success/regression_11294/Nargo.toml b/test_programs/execution_success/regression_11294/Nargo.toml new file mode 100644 index 00000000000..42fcd7432ff --- /dev/null +++ b/test_programs/execution_success/regression_11294/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_11294" +version = "0.1.0" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/regression_11294/Prover.json b/test_programs/execution_success/regression_11294/Prover.json new file mode 100644 index 00000000000..fbb61746a8a --- /dev/null +++ b/test_programs/execution_success/regression_11294/Prover.json @@ -0,0 +1,56 @@ +{ + "previous_kernel_public_inputs": { + "end": { + "private_call_stack": [ + { + "args_hash": "0x0c78b411fc893c51d446c08daa5741b9ba6103126c9e450bed90fcde8793168a", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000002", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000007" + }, + { + "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" + }, + { + "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" + }, + { + "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" + }, + { + "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" + }, + { + "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" + }, + { + "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" + }, + { + "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", + "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", + "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" + } + ] + } + } + } diff --git a/test_programs/execution_success/regression_11294/src/main.nr b/test_programs/execution_success/regression_11294/src/main.nr new file mode 100644 index 00000000000..9440a8d1482 --- /dev/null +++ b/test_programs/execution_success/regression_11294/src/main.nr @@ -0,0 +1,186 @@ +// Capture the "attempt to subtract with overflow" from https://github.com/AztecProtocol/aztec-packages/pull/11294 + +pub global MAX_PRIVATE_CALL_STACK_LENGTH_PER_TX: u32 = 8; + +unconstrained fn main( + previous_kernel_public_inputs: PrivateKernelCircuitPublicInputs, +) -> pub PrivateKernelCircuitPublicInputs { + let private_inputs = PrivateKernelInnerCircuitPrivateInputs::new(previous_kernel_public_inputs); + private_inputs.execute() +} + +pub struct PrivateKernelCircuitPublicInputs { + pub end: PrivateAccumulatedData, +} + +pub struct PrivateKernelData { + pub public_inputs: PrivateKernelCircuitPublicInputs, +} + +pub struct PrivateAccumulatedData { + pub private_call_stack: [PrivateCallRequest; MAX_PRIVATE_CALL_STACK_LENGTH_PER_TX], +} + +pub struct PrivateCallRequest { + pub args_hash: Field, + pub returns_hash: Field, + pub start_side_effect_counter: u32, + pub end_side_effect_counter: u32, +} + +pub struct PrivateKernelCircuitPublicInputsComposer { + pub public_inputs: PrivateKernelCircuitPublicInputsBuilder, +} + +impl PrivateKernelCircuitPublicInputsComposer { + pub unconstrained fn new_from_previous_kernel( + previous_kernel_public_inputs: PrivateKernelCircuitPublicInputs, + ) -> Self { + let mut public_inputs = PrivateKernelCircuitPublicInputsBuilder { + end: PrivateAccumulatedDataBuilder { private_call_stack: BoundedVec::new() }, + }; + + let start = previous_kernel_public_inputs.end; + public_inputs.end.private_call_stack = array_to_bounded_vec(start.private_call_stack); + + PrivateKernelCircuitPublicInputsComposer { public_inputs } + } + + pub fn pop_top_call_request(&mut self) -> Self { + // Pop the top item in the call stack, which is the caller of the current call, and shouldn't be propagated to the output. + let _call_request = self.public_inputs.end.private_call_stack.pop(); + *self + } + + pub fn finish(self) -> PrivateKernelCircuitPublicInputs { + self.public_inputs.finish() + } +} + +pub struct PrivateKernelCircuitPublicInputsBuilder { + pub end: PrivateAccumulatedDataBuilder, +} + +impl PrivateKernelCircuitPublicInputsBuilder { + pub fn finish(self) -> PrivateKernelCircuitPublicInputs { + PrivateKernelCircuitPublicInputs { end: self.end.finish() } + } +} + +pub struct PrivateAccumulatedDataBuilder { + pub private_call_stack: BoundedVec, +} + +impl PrivateAccumulatedDataBuilder { + pub fn finish(self) -> PrivateAccumulatedData { + PrivateAccumulatedData { private_call_stack: self.private_call_stack.storage() } + } +} + +pub struct PrivateKernelInnerCircuitPrivateInputs { + previous_kernel: PrivateKernelData, +} + +impl PrivateKernelInnerCircuitPrivateInputs { + pub fn new(public_inputs: PrivateKernelCircuitPublicInputs) -> Self { + Self { previous_kernel: PrivateKernelData { public_inputs } } + } + + unconstrained fn generate_output(self) -> PrivateKernelCircuitPublicInputs { + // XXX: Declaring `let mut composer = ` would make the circuit pass. + PrivateKernelCircuitPublicInputsComposer::new_from_previous_kernel( + self.previous_kernel.public_inputs, + ) + .pop_top_call_request() + .finish() + } + + pub fn execute(self) -> PrivateKernelCircuitPublicInputs { + // XXX: Running both this and the bottom assertion would make the circuit pass. + // assert(!is_empty(self.previous_kernel.public_inputs.end.private_call_stack[0]), "not empty before"); + + // Safety: This is where the program treated the input as mutable. + let output = unsafe { self.generate_output() }; + + assert( + !is_empty(self.previous_kernel.public_inputs.end.private_call_stack[0]), + "not empty after", + ); + + output + } +} + +pub trait Empty { + fn empty() -> Self; +} + +pub fn is_empty(item: T) -> bool +where + T: Empty + Eq, +{ + item.eq(T::empty()) +} + +impl Eq for PrivateCallRequest { + fn eq(self, other: PrivateCallRequest) -> bool { + (self.args_hash == other.args_hash) + & (self.returns_hash == other.returns_hash) + & (self.start_side_effect_counter == other.start_side_effect_counter) + & (self.end_side_effect_counter == other.end_side_effect_counter) + } +} + +impl Empty for PrivateCallRequest { + fn empty() -> Self { + PrivateCallRequest { + args_hash: 0, + returns_hash: 0, + start_side_effect_counter: 0, + end_side_effect_counter: 0, + } + } +} + +// Copy of https://github.com/AztecProtocol/aztec-packages/blob/f1fd2d104d01a4582d8a48a6ab003d8791010967/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays.nr#L110 +pub fn array_length(array: [T; N]) -> u32 +where + T: Empty + Eq, +{ + // We get the length by checking the index of the first empty element. + + // Safety: This is safe because we have validated the array (see function doc above) and the emptiness + // of the element and non-emptiness of the previous element is checked below. + let length = unsafe { find_index_hint(array, |elem: T| is_empty(elem)) }; + // if length != 0 { + // assert(!is_empty(array[length - 1])); + // } + // if length != N { + // assert(is_empty(array[length])); + // } + length +} + +// Helper function to find the index of the first element in an array that satisfies a given predicate. If the element +// is not found, the function returns N as the index. +pub unconstrained fn find_index_hint( + array: [T; N], + find: fn[Env](T) -> bool, +) -> u32 { + let mut index = N; + for i in 0..N { + // We check `index == N` to ensure that we only update the index if we haven't found a match yet. + if (index == N) & find(array[i]) { + index = i; + } + } + index +} + +pub unconstrained fn array_to_bounded_vec(array: [T; N]) -> BoundedVec +where + T: Empty + Eq, +{ + let len = array_length(array); + BoundedVec::from_parts_unchecked(array, len) +} From f26294ee2323cf961c134f3956ad79ef4bd2a636 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 23 Jan 2025 15:32:08 +0000 Subject: [PATCH 2/4] Keep inc_rc during preprocessing --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 99 +++++++++++++++---- .../src/ssa/opt/preprocess_fns.rs | 2 +- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 2 +- 3 files changed, 84 insertions(+), 19 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index e80769756e8..1eaaa1de0dc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -26,14 +26,20 @@ impl Ssa { /// This step should come after the flattening of the CFG and mem2reg. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn dead_instruction_elimination(self) -> Ssa { - self.dead_instruction_elimination_inner(true) + self.dead_instruction_elimination_inner(true, false) } - fn dead_instruction_elimination_inner(mut self, flattened: bool) -> Ssa { + fn dead_instruction_elimination_inner( + mut self, + flattened: bool, + keep_rcs_of_parameters: bool, + ) -> Ssa { let mut used_global_values: HashSet<_> = self .functions .par_iter_mut() - .flat_map(|(_, func)| func.dead_instruction_elimination(true, flattened)) + .flat_map(|(_, func)| { + func.dead_instruction_elimination(true, flattened, keep_rcs_of_parameters) + }) .collect(); let globals = &self.functions[&self.main_id].dfg.globals; @@ -69,6 +75,7 @@ impl Function { &mut self, insert_out_of_bounds_checks: bool, flattened: bool, + keep_rcs_of_parameters: bool, ) -> HashSet { let mut context = Context { flattened, ..Default::default() }; @@ -84,6 +91,7 @@ impl Function { self, *block, insert_out_of_bounds_checks, + keep_rcs_of_parameters, ); } @@ -91,7 +99,7 @@ impl Function { // instructions (we don't want to remove those checks, or instructions that are // dependencies of those checks) if inserted_out_of_bounds_checks { - return self.dead_instruction_elimination(false, flattened); + return self.dead_instruction_elimination(false, flattened, keep_rcs_of_parameters); } context.remove_rc_instructions(&mut self.dfg); @@ -140,6 +148,7 @@ impl Context { function: &mut Function, block_id: BasicBlockId, insert_out_of_bounds_checks: bool, + keep_rcs_of_parameters: bool, ) -> bool { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); @@ -148,10 +157,17 @@ impl Context { let mut rc_tracker = RcTracker::default(); + // During the preprocessing of functions in isolation we don't want to + // get rid of IncRCs arrays that can potentially be mutated outside. + if keep_rcs_of_parameters { + rc_tracker.track_function_parameters(function); + } + // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. let mut possible_index_out_of_bounds_indexes = Vec::new(); + // Going in reverse so we know if a result of an instruction was used. for (instruction_index, instruction_id) in block.instructions().iter().rev().enumerate() { let instruction = &function.dfg[*instruction_id]; @@ -241,6 +257,8 @@ impl Context { } } + /// Go through the RC instructions collected when we figured out which values were unused; + /// for each RC that refers to an unused value, remove the RC as well. fn remove_rc_instructions(&self, dfg: &mut DataFlowGraph) { let unused_rc_values_by_block: HashMap> = self.rc_instructions.iter().fold(HashMap::default(), |mut acc, (rc, block)| { @@ -580,10 +598,12 @@ struct RcTracker { // with the same value but no array set in between. // If we see an inc/dec RC pair within a block we can safely remove both instructions. rcs_with_possible_pairs: HashMap>, + // Tracks repeated RC instructions: if there are two `inc_rc` for the same value in a row, the 2nd one is redundant. rc_pairs_to_remove: HashSet, // We also separately track all IncrementRc instructions and all array types which have been mutably borrowed. // If an array is the same type as one of those non-mutated array types, we can safely remove all IncrementRc instructions on that array. inc_rcs: HashMap>, + // When tracking mutations we consider arrays with the same type as all being possibly mutated. mutated_array_types: HashSet, // The SSA often creates patterns where after simplifications we end up with repeat // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc, @@ -593,9 +613,23 @@ struct RcTracker { } impl RcTracker { + /// Mark any array parameters to the function itself as possibly mutated, + /// so we don't get rid of RC instructions just because we don't mutate + /// them in this function, which could potentially cause them to be + /// mutated outside the function without our consent. + fn track_function_parameters(&mut self, function: &Function) { + for parameter in function.parameters() { + let typ = function.dfg.type_of_value(*parameter); + if typ.contains_an_array() { + self.mutated_array_types.insert(typ); + } + } + } + fn track_inc_rcs_to_remove(&mut self, instruction_id: InstructionId, function: &Function) { let instruction = &function.dfg[instruction_id]; + // Deduplicate IncRC instructions. if let Instruction::IncrementRc { value } = instruction { if let Some(previous_value) = self.previous_inc_rc { if previous_value == *value { @@ -604,6 +638,7 @@ impl RcTracker { } self.previous_inc_rc = Some(*value); } else { + // Reset the deduplication. self.previous_inc_rc = None; } @@ -611,6 +646,8 @@ impl RcTracker { // when we see a DecrementRc and check whether it was possibly mutated when we see an IncrementRc. match instruction { Instruction::IncrementRc { value } => { + // Get any RC instruction recorded further down the block for this array; + // if it exists and not marked as mutated, then both RCs can be removed. if let Some(inc_rc) = pop_rc_for(*value, function, &mut self.rcs_with_possible_pairs) { @@ -619,7 +656,7 @@ impl RcTracker { self.rc_pairs_to_remove.insert(instruction_id); } } - + // Remember that this array was RC'd by this instruction. self.inc_rcs.entry(*value).or_default().insert(instruction_id); } Instruction::DecrementRc { value } => { @@ -632,12 +669,12 @@ impl RcTracker { } Instruction::ArraySet { array, .. } => { let typ = function.dfg.type_of_value(*array); + // We mark all RCs that refer to arrays with a matching type as the one being set, as possibly mutated. if let Some(dec_rcs) = self.rcs_with_possible_pairs.get_mut(&typ) { for dec_rc in dec_rcs { dec_rc.possibly_mutated = true; } } - self.mutated_array_types.insert(typ); } Instruction::Store { value, .. } => { @@ -648,6 +685,7 @@ impl RcTracker { } } Instruction::Call { arguments, .. } => { + // Treat any array-type arguments to calls as possible sources of mutation. for arg in arguments { let typ = function.dfg.type_of_value(*arg); if matches!(&typ, Type::Array(..) | Type::Slice(..)) { @@ -659,6 +697,7 @@ impl RcTracker { } } + /// Get all RC instructions which work on arrays whose type has not been marked as mutated. fn get_non_mutated_arrays(&self, dfg: &DataFlowGraph) -> HashSet { self.inc_rcs .keys() @@ -857,16 +896,6 @@ mod test { #[test] fn keep_inc_rc_on_borrowed_array_set() { - // brillig(inline) fn main f0 { - // b0(v0: [u32; 2]): - // inc_rc v0 - // v3 = array_set v0, index u32 0, value u32 1 - // inc_rc v0 - // inc_rc v0 - // inc_rc v0 - // v4 = array_get v3, index u32 1 - // return v4 - // } let src = " brillig(inline) fn main f0 { b0(v0: [u32; 2]): @@ -919,6 +948,42 @@ mod test { assert_normalized_ssa_equals(ssa, src); } + #[test] + fn not_remove_inc_rcs_for_input_parameters() { + let src = " + brillig(inline) fn main f0 { + b0(v0: [Field; 2]): + inc_rc v0 + inc_rc v0 + inc_rc v0 + v2 = array_get v0, index u32 0 -> Field + inc_rc v0 + return v2 + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + let main = ssa.main(); + + // The instruction count never includes the terminator instruction + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); + + let expected = " + brillig(inline) fn main f0 { + b0(v0: [Field; 2]): + inc_rc v0 + v2 = array_get v0, index u32 0 -> Field + inc_rc v0 + return v2 + } + "; + + // We want to be able to switch this on during preprocessing. + let keep_rcs_of_parameters = true; + let ssa = ssa.dead_instruction_elimination_inner(true, keep_rcs_of_parameters); + assert_normalized_ssa_equals(ssa, expected); + } + #[test] fn remove_inc_rcs_that_are_never_mutably_borrowed() { let src = " @@ -1010,7 +1075,7 @@ mod test { let ssa = Ssa::from_str(src).unwrap(); // Even though these ACIR functions only have 1 block, we have not inlined and flattened anything yet. - let ssa = ssa.dead_instruction_elimination_inner(false); + let ssa = ssa.dead_instruction_elimination_inner(false, false); let expected = " acir(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index ae20c9b8b4a..9edbe4c3d16 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -58,7 +58,7 @@ impl Ssa { // Try to reduce the number of blocks. function.simplify_function(); // Remove leftover instructions. - function.dead_instruction_elimination(true, false); + function.dead_instruction_elimination(true, false, true); // Put it back into the SSA, so the next functions can pick it up. self.functions.insert(id, function); diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index eb0bbd8c532..eb502262776 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -1035,7 +1035,7 @@ fn brillig_bytecode_size( simplify_between_unrolls(&mut temp); // This is to try to prevent hitting ICE. - temp.dead_instruction_elimination(false, true); + temp.dead_instruction_elimination(false, true, false); convert_ssa_function(&temp, false, globals).byte_code.len() } From 7b0411fa419248535ea23f80a4cb4621cf244fdb Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 23 Jan 2025 21:23:40 +0000 Subject: [PATCH 3/4] Use TOML --- .../regression_11294/Prover.json | 56 ------------------- .../regression_11294/Prover.toml | 47 ++++++++++++++++ 2 files changed, 47 insertions(+), 56 deletions(-) delete mode 100644 test_programs/execution_success/regression_11294/Prover.json create mode 100644 test_programs/execution_success/regression_11294/Prover.toml diff --git a/test_programs/execution_success/regression_11294/Prover.json b/test_programs/execution_success/regression_11294/Prover.json deleted file mode 100644 index fbb61746a8a..00000000000 --- a/test_programs/execution_success/regression_11294/Prover.json +++ /dev/null @@ -1,56 +0,0 @@ -{ - "previous_kernel_public_inputs": { - "end": { - "private_call_stack": [ - { - "args_hash": "0x0c78b411fc893c51d446c08daa5741b9ba6103126c9e450bed90fcde8793168a", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000002", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000007" - }, - { - "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" - }, - { - "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" - }, - { - "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" - }, - { - "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" - }, - { - "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" - }, - { - "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" - }, - { - "args_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "returns_hash": "0x0000000000000000000000000000000000000000000000000000000000000000", - "start_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000", - "end_side_effect_counter": "0x0000000000000000000000000000000000000000000000000000000000000000" - } - ] - } - } - } diff --git a/test_programs/execution_success/regression_11294/Prover.toml b/test_programs/execution_success/regression_11294/Prover.toml new file mode 100644 index 00000000000..c0bc12aeed9 --- /dev/null +++ b/test_programs/execution_success/regression_11294/Prover.toml @@ -0,0 +1,47 @@ +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0c78b411fc893c51d446c08daa5741b9ba6103126c9e450bed90fcde8793168a" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000002" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000007" + +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" + +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" + +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" + +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" + +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" + +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" + +[[previous_kernel_public_inputs.end.private_call_stack]] +args_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +returns_hash = "0x0000000000000000000000000000000000000000000000000000000000000000" +start_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" +end_side_effect_counter = "0x0000000000000000000000000000000000000000000000000000000000000000" From 384cc38e467871508ce77bf9041e2bc07d4bebbc Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 17:03:04 -0500 Subject: [PATCH 4/4] fix(ssa): Check arrays used as a terminator argument in the RC tracker (#7165) Co-authored-by: Tom French Co-authored-by: Akosh Farkash --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 64 ++++++------------- .../src/ssa/opt/preprocess_fns.rs | 2 +- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 2 +- tooling/nargo_cli/src/cli/info_cmd.rs | 6 +- 4 files changed, 26 insertions(+), 48 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 1eaaa1de0dc..a8f0659f8db 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -26,20 +26,14 @@ impl Ssa { /// This step should come after the flattening of the CFG and mem2reg. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn dead_instruction_elimination(self) -> Ssa { - self.dead_instruction_elimination_inner(true, false) + self.dead_instruction_elimination_inner(true) } - fn dead_instruction_elimination_inner( - mut self, - flattened: bool, - keep_rcs_of_parameters: bool, - ) -> Ssa { + fn dead_instruction_elimination_inner(mut self, flattened: bool) -> Ssa { let mut used_global_values: HashSet<_> = self .functions .par_iter_mut() - .flat_map(|(_, func)| { - func.dead_instruction_elimination(true, flattened, keep_rcs_of_parameters) - }) + .flat_map(|(_, func)| func.dead_instruction_elimination(true, flattened)) .collect(); let globals = &self.functions[&self.main_id].dfg.globals; @@ -75,7 +69,6 @@ impl Function { &mut self, insert_out_of_bounds_checks: bool, flattened: bool, - keep_rcs_of_parameters: bool, ) -> HashSet { let mut context = Context { flattened, ..Default::default() }; @@ -91,7 +84,6 @@ impl Function { self, *block, insert_out_of_bounds_checks, - keep_rcs_of_parameters, ); } @@ -99,7 +91,7 @@ impl Function { // instructions (we don't want to remove those checks, or instructions that are // dependencies of those checks) if inserted_out_of_bounds_checks { - return self.dead_instruction_elimination(false, flattened, keep_rcs_of_parameters); + return self.dead_instruction_elimination(false, flattened); } context.remove_rc_instructions(&mut self.dfg); @@ -148,20 +140,14 @@ impl Context { function: &mut Function, block_id: BasicBlockId, insert_out_of_bounds_checks: bool, - keep_rcs_of_parameters: bool, ) -> bool { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); - let instructions_len = block.instructions().len(); - let mut rc_tracker = RcTracker::default(); + rc_tracker.mark_terminator_arrays_as_used(function, block); - // During the preprocessing of functions in isolation we don't want to - // get rid of IncRCs arrays that can potentially be mutated outside. - if keep_rcs_of_parameters { - rc_tracker.track_function_parameters(function); - } + let instructions_len = block.instructions().len(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. @@ -613,17 +599,13 @@ struct RcTracker { } impl RcTracker { - /// Mark any array parameters to the function itself as possibly mutated, - /// so we don't get rid of RC instructions just because we don't mutate - /// them in this function, which could potentially cause them to be - /// mutated outside the function without our consent. - fn track_function_parameters(&mut self, function: &Function) { - for parameter in function.parameters() { - let typ = function.dfg.type_of_value(*parameter); - if typ.contains_an_array() { + fn mark_terminator_arrays_as_used(&mut self, function: &Function, block: &BasicBlock) { + block.unwrap_terminator().for_each_value(|value| { + let typ = function.dfg.type_of_value(value); + if matches!(&typ, Type::Array(_, _) | Type::Slice(_)) { self.mutated_array_types.insert(typ); } - } + }); } fn track_inc_rcs_to_remove(&mut self, instruction_id: InstructionId, function: &Function) { @@ -686,6 +668,8 @@ impl RcTracker { } Instruction::Call { arguments, .. } => { // Treat any array-type arguments to calls as possible sources of mutation. + // During the preprocessing of functions in isolation we don't want to + // get rid of IncRCs arrays that can potentially be mutated outside. for arg in arguments { let typ = function.dfg.type_of_value(*arg); if matches!(&typ, Type::Array(..) | Type::Slice(..)) { @@ -949,7 +933,7 @@ mod test { } #[test] - fn not_remove_inc_rcs_for_input_parameters() { + fn remove_inc_rcs_that_are_never_mutably_borrowed() { let src = " brillig(inline) fn main f0 { b0(v0: [Field; 2]): @@ -971,21 +955,17 @@ mod test { let expected = " brillig(inline) fn main f0 { b0(v0: [Field; 2]): - inc_rc v0 v2 = array_get v0, index u32 0 -> Field - inc_rc v0 return v2 } "; - // We want to be able to switch this on during preprocessing. - let keep_rcs_of_parameters = true; - let ssa = ssa.dead_instruction_elimination_inner(true, keep_rcs_of_parameters); + let ssa = ssa.dead_instruction_elimination(); assert_normalized_ssa_equals(ssa, expected); } #[test] - fn remove_inc_rcs_that_are_never_mutably_borrowed() { + fn do_not_remove_inc_rcs_for_arrays_in_terminator() { let src = " brillig(inline) fn main f0 { b0(v0: [Field; 2]): @@ -994,21 +974,19 @@ mod test { inc_rc v0 v2 = array_get v0, index u32 0 -> Field inc_rc v0 - return v2 + return v0, v2 } "; let ssa = Ssa::from_str(src).unwrap(); - let main = ssa.main(); - - // The instruction count never includes the terminator instruction - assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); let expected = " brillig(inline) fn main f0 { b0(v0: [Field; 2]): + inc_rc v0 v2 = array_get v0, index u32 0 -> Field - return v2 + inc_rc v0 + return v0, v2 } "; @@ -1075,7 +1053,7 @@ mod test { let ssa = Ssa::from_str(src).unwrap(); // Even though these ACIR functions only have 1 block, we have not inlined and flattened anything yet. - let ssa = ssa.dead_instruction_elimination_inner(false, false); + let ssa = ssa.dead_instruction_elimination_inner(false); let expected = " acir(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index 9edbe4c3d16..ae20c9b8b4a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -58,7 +58,7 @@ impl Ssa { // Try to reduce the number of blocks. function.simplify_function(); // Remove leftover instructions. - function.dead_instruction_elimination(true, false, true); + function.dead_instruction_elimination(true, false); // Put it back into the SSA, so the next functions can pick it up. self.functions.insert(id, function); diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index eb502262776..eb0bbd8c532 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -1035,7 +1035,7 @@ fn brillig_bytecode_size( simplify_between_unrolls(&mut temp); // This is to try to prevent hitting ICE. - temp.dead_instruction_elimination(false, true, false); + temp.dead_instruction_elimination(false, true); convert_ssa_function(&temp, false, globals).byte_code.len() } diff --git a/tooling/nargo_cli/src/cli/info_cmd.rs b/tooling/nargo_cli/src/cli/info_cmd.rs index 8d0fc257e1c..b9d6225184f 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -13,11 +13,11 @@ use prettytable::{row, table, Row}; use rayon::prelude::*; use serde::Serialize; -use crate::{cli::fs::inputs::read_inputs_from_file, errors::CliError}; +use crate::errors::CliError; use super::{ compile_cmd::{compile_workspace_full, get_target_width}, - fs::program::read_program_from_file, + fs::{inputs::read_inputs_from_file, program::read_program_from_file}, NargoConfig, PackageOptions, }; @@ -243,7 +243,7 @@ fn profile_brillig_execution( ) -> Result, CliError> { let mut program_info = Vec::new(); for (package, program_artifact) in binary_packages.iter() { - // Parse the initial witness values from Prover.toml + // Parse the initial witness values from Prover.toml or Prover.json let (inputs_map, _) = read_inputs_from_file( &package.root_dir, prover_name,