From 3e35b8765a2cada0e484a83550b44b3cdd8d55ed Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 16:22:09 +0000 Subject: [PATCH 01/13] check references passed to a call in the rc tracker --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 1eaaa1de0dc..7769656b1a6 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -148,7 +148,7 @@ impl Context { function: &mut Function, block_id: BasicBlockId, insert_out_of_bounds_checks: bool, - keep_rcs_of_parameters: bool, + _keep_rcs_of_parameters: bool, ) -> bool { let block = &function.dfg[block_id]; self.mark_terminator_values_as_used(function, block); @@ -157,11 +157,7 @@ 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); - } + 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. @@ -686,9 +682,11 @@ 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(..)) { + if typ.contains_an_array() { self.mutated_array_types.insert(typ); } } From 91e5d0e2aee554559fa3c338a55473d3d7bdedf3 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 16:35:04 +0000 Subject: [PATCH 02/13] resolve binary value ids From e8d24fdc6fcd478f1b4504dd7a73fd11a205bb48 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 16:44:11 +0000 Subject: [PATCH 03/13] resovle --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 7769656b1a6..77ee9b22a55 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -157,8 +157,6 @@ impl Context { let mut rc_tracker = RcTracker::default(); - 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(); From 7e204be2d662ab286ba70c49ed5ba600d4d5b59c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 16:48:45 +0000 Subject: [PATCH 04/13] get rid of track_function_parameters --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 77ee9b22a55..cca8df9233a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -607,19 +607,6 @@ 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]; From 0af868ef98fa4da3a3189e09b097ddc8ec3584f1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 16:59:32 +0000 Subject: [PATCH 05/13] track arrays in the terminator --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index cca8df9233a..d7fe1647d11 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -153,9 +153,10 @@ impl Context { 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); + + 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. @@ -607,6 +608,15 @@ struct RcTracker { } impl RcTracker { + 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) { let instruction = &function.dfg[instruction_id]; @@ -671,7 +681,7 @@ impl RcTracker { // get rid of IncRCs arrays that can potentially be mutated outside. for arg in arguments { let typ = function.dfg.type_of_value(*arg); - if typ.contains_an_array() { + if matches!(&typ, Type::Array(_, _) | Type::Slice(_)) { self.mutated_array_types.insert(typ); } } From b3c515c6baa6ff03d32bc2ab358ca814877b4710 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 17:02:25 +0000 Subject: [PATCH 06/13] read from any input in info cmd --- tooling/nargo_cli/src/cli/info_cmd.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tooling/nargo_cli/src/cli/info_cmd.rs b/tooling/nargo_cli/src/cli/info_cmd.rs index 8d0fc257e1c..9148f17f782 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -243,11 +243,10 @@ 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 - let (inputs_map, _) = read_inputs_from_file( + // Parse the initial witness values from Prover.toml or Prover.json + let (inputs_map, _) = read_inputs_from_file_any_format( &package.root_dir, prover_name, - Format::Toml, &program_artifact.abi, )?; let initial_witness = program_artifact.abi.encode(&inputs_map, None)?; From 54317c664634701b6c89d6cf0fb24ee41649e863 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 17:02:34 +0000 Subject: [PATCH 07/13] fmt --- tooling/nargo_cli/src/cli/info_cmd.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tooling/nargo_cli/src/cli/info_cmd.rs b/tooling/nargo_cli/src/cli/info_cmd.rs index 9148f17f782..ddc652b36dc 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -6,18 +6,17 @@ use nargo::{ constants::PROVER_INPUT_FILE, foreign_calls::DefaultForeignCallBuilder, package::Package, }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml}; -use noirc_abi::input_parser::Format; use noirc_artifacts::program::ProgramArtifact; use noirc_driver::{CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; 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_any_format, program::read_program_from_file}, NargoConfig, PackageOptions, }; From 65e0151ba6516f77a26248c87e99f4422454f8fc Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 17:45:17 +0000 Subject: [PATCH 08/13] delete not_remove_inc_rcs_for_input_parameters test --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 36 --------------------- 1 file changed, 36 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index d7fe1647d11..f9ae6a668de 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -941,42 +941,6 @@ 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 = " From da4ff84260dee84eadbe25e0ad55d5221a58d0e0 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 23 Jan 2025 19:37:55 +0000 Subject: [PATCH 09/13] . From 56264e122225545d5304576f549a639c12f662df Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 19:50:57 +0000 Subject: [PATCH 10/13] cleanup --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index f9ae6a668de..f788a8e619b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -1032,7 +1032,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 { From 6aa559e1fabcc87aa1f8983d858c155a8ba76cf1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 23 Jan 2025 14:51:45 -0500 Subject: [PATCH 11/13] Update compiler/noirc_evaluator/src/ssa/opt/die.rs --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index f788a8e619b..3e56b070964 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -681,7 +681,7 @@ impl RcTracker { // 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(_)) { + if matches!(&typ, Type::Array(..) | Type::Slice(..)) { self.mutated_array_types.insert(typ); } } From b8b59352bf00a51d1f70a69e7829c7f9ed4335da Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 23 Jan 2025 21:35:12 +0000 Subject: [PATCH 12/13] Fix rebase --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 17 ++++------------- .../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, 10 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 3e56b070964..ec84e62589c 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,7 +140,6 @@ 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); 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 ddc652b36dc..b9d6225184f 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -6,6 +6,7 @@ use nargo::{ constants::PROVER_INPUT_FILE, foreign_calls::DefaultForeignCallBuilder, package::Package, }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml}; +use noirc_abi::input_parser::Format; use noirc_artifacts::program::ProgramArtifact; use noirc_driver::{CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; use prettytable::{row, table, Row}; @@ -16,7 +17,7 @@ use crate::errors::CliError; use super::{ compile_cmd::{compile_workspace_full, get_target_width}, - fs::{inputs::read_inputs_from_file_any_format, program::read_program_from_file}, + fs::{inputs::read_inputs_from_file, program::read_program_from_file}, NargoConfig, PackageOptions, }; @@ -243,9 +244,10 @@ fn profile_brillig_execution( let mut program_info = Vec::new(); for (package, program_artifact) in binary_packages.iter() { // Parse the initial witness values from Prover.toml or Prover.json - let (inputs_map, _) = read_inputs_from_file_any_format( + let (inputs_map, _) = read_inputs_from_file( &package.root_dir, prover_name, + Format::Toml, &program_artifact.abi, )?; let initial_witness = program_artifact.abi.encode(&inputs_map, None)?; From 4a9e140eaaa0ee2766e6bc7fb176964894e7e22e Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 23 Jan 2025 21:39:22 +0000 Subject: [PATCH 13/13] Add unit test --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 30 +++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index ec84e62589c..a8f0659f8db 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -964,6 +964,36 @@ mod test { assert_normalized_ssa_equals(ssa, expected); } + #[test] + fn do_not_remove_inc_rcs_for_arrays_in_terminator() { + 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 v0, v2 + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + + 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 v0, v2 + } + "; + + let ssa = ssa.dead_instruction_elimination(); + assert_normalized_ssa_equals(ssa, expected); + } + #[test] fn do_not_remove_inc_rc_if_used_as_call_arg() { // We do not want to remove inc_rc instructions on values