From fc4b3589b42c866dc971058f89af0cd39a58745d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 21:39:17 +0000 Subject: [PATCH 1/9] Test that the optimization is idempotent on all test programs --- tooling/nargo_cli/src/cli/compile_cmd.rs | 117 +++++++++++++++++++++-- 1 file changed, 107 insertions(+), 10 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index ff6009981c7..e42bd5d299c 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -8,7 +8,9 @@ use nargo::ops::{collect_errors, compile_contract, compile_program, report_error use nargo::package::{CrateName, Package}; use nargo::workspace::Workspace; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; -use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; +use nargo_toml::{ + get_package_manifest, resolve_workspace_from_toml, ManifestError, PackageSelection, +}; use noirc_driver::DEFAULT_EXPRESSION_WIDTH; use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; use noirc_driver::{CompilationResult, CompileOptions, CompiledContract}; @@ -44,16 +46,11 @@ pub(crate) struct CompileCommand { } pub(crate) fn run(args: CompileCommand, config: NargoConfig) -> Result<(), CliError> { - let toml_path = get_package_manifest(&config.program_dir)?; let default_selection = if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll }; let selection = args.package.map_or(default_selection, PackageSelection::Selected); - let workspace = resolve_workspace_from_toml( - &toml_path, - selection, - Some(NOIR_ARTIFACT_VERSION_STRING.to_owned()), - )?; + let workspace = read_workspace(&config.program_dir, selection)?; if args.watch { watch_workspace(&workspace, &args.compile_options) @@ -65,6 +62,22 @@ pub(crate) fn run(args: CompileCommand, config: NargoConfig) -> Result<(), CliEr Ok(()) } +/// Read a given program directory into a workspace. +fn read_workspace( + program_dir: &Path, + selection: PackageSelection, +) -> Result { + let toml_path = get_package_manifest(program_dir)?; + + let workspace = resolve_workspace_from_toml( + &toml_path, + selection, + Some(NOIR_ARTIFACT_VERSION_STRING.to_owned()), + )?; + + Ok(workspace) +} + /// Continuously recompile the workspace on any Noir file change event. fn watch_workspace(workspace: &Workspace, compile_options: &CompileOptions) -> notify::Result<()> { let (tx, rx) = std::sync::mpsc::channel(); @@ -109,15 +122,21 @@ fn watch_workspace(workspace: &Workspace, compile_options: &CompileOptions) -> n Ok(()) } +/// Parse all files in the workspace. +fn parse_workspace(workspace: &Workspace) -> (FileManager, ParsedFiles) { + let mut file_manager = workspace.new_file_manager(); + insert_all_files_for_workspace_into_file_manager(workspace, &mut file_manager); + let parsed_files = parse_all(&file_manager); + (file_manager, parsed_files) +} + /// Parse and compile the entire workspace, then report errors. /// This is the main entry point used by all other commands that need compilation. pub(super) fn compile_workspace_full( workspace: &Workspace, compile_options: &CompileOptions, ) -> Result<(), CliError> { - let mut workspace_file_manager = workspace.new_file_manager(); - insert_all_files_for_workspace_into_file_manager(workspace, &mut workspace_file_manager); - let parsed_files = parse_all(&workspace_file_manager); + let (workspace_file_manager, parsed_files) = parse_workspace(workspace); let compiled_workspace = compile_workspace(&workspace_file_manager, &parsed_files, workspace, compile_options); @@ -296,3 +315,81 @@ pub(crate) fn get_target_width( compile_options_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH) } } + +#[cfg(test)] +mod tests { + use std::path::{Path, PathBuf}; + + use acvm::acir::circuit::ExpressionWidth; + use nargo::ops::compile_program; + use nargo_toml::PackageSelection; + use noirc_driver::CompileOptions; + + use crate::cli::compile_cmd::{parse_workspace, read_workspace}; + + /// Try to find the directory that Cargo sets when it is running; otherwise fallback to assuming the CWD + /// is the root of the repository and append the crate path + fn test_programs_dir() -> PathBuf { + let root_dir = match std::env::var("CARGO_MANIFEST_DIR") { + Ok(dir) => PathBuf::from(dir).parent().unwrap().parent().unwrap().to_path_buf(), + Err(_) => std::env::current_dir().unwrap(), + }; + root_dir.join("test_programs") + } + + /// Collect the test programs under a sub-directory. + fn read_test_program_dirs( + test_programs_dir: &Path, + test_sub_dir: &str, + ) -> impl Iterator { + let test_case_dir = test_programs_dir.join(test_sub_dir); + std::fs::read_dir(test_case_dir) + .unwrap() + .flatten() + .filter(|c| c.path().is_dir()) + .map(|c| c.path()) + .into_iter() + } + + /// Check that `nargo::ops::transform_program` is idempotent by compiling the + /// test programs and running them through the optimizer twice. + /// + /// This test is here purely because of the convenience of having access to + /// the utility functions to process workspaces. + #[test] + fn test_transform_program_is_idempotent() { + let test_workspaces = read_test_program_dirs(&test_programs_dir(), "execution_success") + .filter_map(|dir| read_workspace(&dir, PackageSelection::DefaultOrAll).ok()) + .collect::>(); + + assert!(!test_workspaces.is_empty(), "should find some test workspaces"); + + for workspace in test_workspaces { + let (file_manager, parsed_files) = parse_workspace(&workspace); + let binary_packages = workspace.into_iter().filter(|package| package.is_binary()); + + for package in binary_packages { + let (program, _warnings) = compile_program( + &file_manager, + &parsed_files, + &workspace, + package, + &CompileOptions::default(), + None, + ) + .expect("failed to compile"); + + let program = nargo::ops::transform_program(program, ExpressionWidth::default()); + let program_hash_1 = fxhash::hash64(&program); + let program = nargo::ops::transform_program(program, ExpressionWidth::default()); + let program_hash_2 = fxhash::hash64(&program); + + assert_eq!( + program_hash_1, program_hash_2, + "optimization not idempotent for test program '{}'", + package.name + ) + } + } + } +} From b443c0535c1ca7fe2ddf12319bb8e8d3e5d3ffc6 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 21:47:46 +0000 Subject: [PATCH 2/9] Process test programs in parallel --- tooling/nargo_cli/src/cli/compile_cmd.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index e42bd5d299c..ed1a77ed4da 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -169,7 +169,7 @@ fn compile_workspace( let program_warnings_or_errors: CompilationResult<()> = compile_programs(file_manager, parsed_files, workspace, &binary_packages, compile_options); - let contract_warnings_or_errors: CompilationResult<()> = compiled_contracts( + let contract_warnings_or_errors: CompilationResult<()> = compile_contracts( file_manager, parsed_files, &contract_packages, @@ -263,7 +263,7 @@ fn compile_programs( } /// Compile the given contracts in the workspace. -fn compiled_contracts( +fn compile_contracts( file_manager: &FileManager, parsed_files: &ParsedFiles, contract_packages: &[Package], @@ -324,6 +324,7 @@ mod tests { use nargo::ops::compile_program; use nargo_toml::PackageSelection; use noirc_driver::CompileOptions; + use rayon::prelude::*; use crate::cli::compile_cmd::{parse_workspace, read_workspace}; @@ -364,7 +365,7 @@ mod tests { assert!(!test_workspaces.is_empty(), "should find some test workspaces"); - for workspace in test_workspaces { + test_workspaces.par_iter().for_each(|workspace| { let (file_manager, parsed_files) = parse_workspace(&workspace); let binary_packages = workspace.into_iter().filter(|package| package.is_binary()); @@ -388,8 +389,8 @@ mod tests { program_hash_1, program_hash_2, "optimization not idempotent for test program '{}'", package.name - ) + ); } - } + }); } } From d5ae0b1f64d829af9d88e7f47d1768cbd352a538 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 22:13:33 +0000 Subject: [PATCH 3/9] Don't include the hash in the panic --- tooling/nargo_cli/src/cli/compile_cmd.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index ed1a77ed4da..142ac319a4a 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -385,8 +385,8 @@ mod tests { let program = nargo::ops::transform_program(program, ExpressionWidth::default()); let program_hash_2 = fxhash::hash64(&program); - assert_eq!( - program_hash_1, program_hash_2, + assert!( + program_hash_1 == program_hash_2, "optimization not idempotent for test program '{}'", package.name ); From f249be4953cb87c5f67be21f948d9570755f3862 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 22:20:23 +0000 Subject: [PATCH 4/9] Fix clippy --- tooling/nargo_cli/src/cli/compile_cmd.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 142ac319a4a..36d40c282e0 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -323,7 +323,7 @@ mod tests { use acvm::acir::circuit::ExpressionWidth; use nargo::ops::compile_program; use nargo_toml::PackageSelection; - use noirc_driver::CompileOptions; + use noirc_driver::{CompileOptions, DEFAULT_EXPRESSION_WIDTH}; use rayon::prelude::*; use crate::cli::compile_cmd::{parse_workspace, read_workspace}; @@ -349,7 +349,6 @@ mod tests { .flatten() .filter(|c| c.path().is_dir()) .map(|c| c.path()) - .into_iter() } /// Check that `nargo::ops::transform_program` is idempotent by compiling the @@ -366,23 +365,26 @@ mod tests { assert!(!test_workspaces.is_empty(), "should find some test workspaces"); test_workspaces.par_iter().for_each(|workspace| { - let (file_manager, parsed_files) = parse_workspace(&workspace); + let (file_manager, parsed_files) = parse_workspace(workspace); let binary_packages = workspace.into_iter().filter(|package| package.is_binary()); for package in binary_packages { let (program, _warnings) = compile_program( &file_manager, &parsed_files, - &workspace, + workspace, package, &CompileOptions::default(), None, ) .expect("failed to compile"); - let program = nargo::ops::transform_program(program, ExpressionWidth::default()); + // let width = DEFAULT_EXPRESSION_WIDTH; + let width = ExpressionWidth::default(); + + let program = nargo::ops::transform_program(program, width); let program_hash_1 = fxhash::hash64(&program); - let program = nargo::ops::transform_program(program, ExpressionWidth::default()); + let program = nargo::ops::transform_program(program, width); let program_hash_2 = fxhash::hash64(&program); assert!( From 0671486591e3bc04dec2dd7a89bda2c4b240caa7 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 22:22:40 +0000 Subject: [PATCH 5/9] Use target width --- tooling/nargo_cli/src/cli/compile_cmd.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 36d40c282e0..3136cd46dda 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -320,13 +320,12 @@ pub(crate) fn get_target_width( mod tests { use std::path::{Path, PathBuf}; - use acvm::acir::circuit::ExpressionWidth; use nargo::ops::compile_program; use nargo_toml::PackageSelection; - use noirc_driver::{CompileOptions, DEFAULT_EXPRESSION_WIDTH}; + use noirc_driver::CompileOptions; use rayon::prelude::*; - use crate::cli::compile_cmd::{parse_workspace, read_workspace}; + use crate::cli::compile_cmd::{get_target_width, parse_workspace, read_workspace}; /// Try to find the directory that Cargo sets when it is running; otherwise fallback to assuming the CWD /// is the root of the repository and append the crate path @@ -379,8 +378,7 @@ mod tests { ) .expect("failed to compile"); - // let width = DEFAULT_EXPRESSION_WIDTH; - let width = ExpressionWidth::default(); + let width = get_target_width(package.expression_width, None); let program = nargo::ops::transform_program(program, width); let program_hash_1 = fxhash::hash64(&program); From 6f4ceba686147393f63b9f53c4f8a50a5d8b9984 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 23:05:21 +0000 Subject: [PATCH 6/9] Add filter option and verbose output --- tooling/nargo_cli/src/cli/compile_cmd.rs | 116 ++++++++++++++++------- tooling/nargo_toml/src/lib.rs | 2 +- 2 files changed, 85 insertions(+), 33 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 3136cd46dda..0d85ef2d194 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -318,11 +318,15 @@ pub(crate) fn get_target_width( #[cfg(test)] mod tests { - use std::path::{Path, PathBuf}; + use std::{ + path::{Path, PathBuf}, + str::FromStr, + }; + use clap::Parser; use nargo::ops::compile_program; use nargo_toml::PackageSelection; - use noirc_driver::CompileOptions; + use noirc_driver::{CompileOptions, CrateName}; use rayon::prelude::*; use crate::cli::compile_cmd::{get_target_width, parse_workspace, read_workspace}; @@ -350,6 +354,29 @@ mod tests { .map(|c| c.path()) } + #[derive(Parser, Debug)] + #[command(ignore_errors = true)] + struct Options { + /// Test name to filter for. + /// + /// For example: + /// ```text + /// cargo test -p nargo_cli -- test_transform_program_is_idempotent slice_loop + /// ``` + args: Vec, + } + + impl Options { + fn package_selection(&self) -> PackageSelection { + match self.args.as_slice() { + [_test_name, test_program] => { + PackageSelection::Selected(CrateName::from_str(test_program).unwrap()) + } + _ => PackageSelection::DefaultOrAll, + } + } + } + /// Check that `nargo::ops::transform_program` is idempotent by compiling the /// test programs and running them through the optimizer twice. /// @@ -357,40 +384,65 @@ mod tests { /// the utility functions to process workspaces. #[test] fn test_transform_program_is_idempotent() { + let opts = Options::parse(); + + let sel = opts.package_selection(); + let verbose = matches!(sel, PackageSelection::Selected(_)); + let test_workspaces = read_test_program_dirs(&test_programs_dir(), "execution_success") - .filter_map(|dir| read_workspace(&dir, PackageSelection::DefaultOrAll).ok()) + .filter_map(|dir| read_workspace(&dir, sel.clone()).ok()) .collect::>(); assert!(!test_workspaces.is_empty(), "should find some test workspaces"); - test_workspaces.par_iter().for_each(|workspace| { - let (file_manager, parsed_files) = parse_workspace(workspace); - let binary_packages = workspace.into_iter().filter(|package| package.is_binary()); - - for package in binary_packages { - let (program, _warnings) = compile_program( - &file_manager, - &parsed_files, - workspace, - package, - &CompileOptions::default(), - None, - ) - .expect("failed to compile"); - - let width = get_target_width(package.expression_width, None); - - let program = nargo::ops::transform_program(program, width); - let program_hash_1 = fxhash::hash64(&program); - let program = nargo::ops::transform_program(program, width); - let program_hash_2 = fxhash::hash64(&program); - - assert!( - program_hash_1 == program_hash_2, - "optimization not idempotent for test program '{}'", - package.name - ); - } - }); + test_workspaces + .par_iter() + //.filter(|workspace| workspace.members.iter().any(|p| p.name == test_workspace_name)) + .for_each(|workspace| { + let (file_manager, parsed_files) = parse_workspace(workspace); + let binary_packages = workspace.into_iter().filter(|package| package.is_binary()); + + for package in binary_packages { + let (program_0, _warnings) = compile_program( + &file_manager, + &parsed_files, + workspace, + package, + &CompileOptions::default(), + None, + ) + .expect("failed to compile"); + + let width = get_target_width(package.expression_width, None); + + let program_1 = nargo::ops::transform_program(program_0, width); + let program_2 = nargo::ops::transform_program(program_1.clone(), width); + + if verbose { + // Compare where the most likely difference is. + assert_eq!( + program_1.program, program_2.program, + "optimization not idempotent for test program '{}'", + package.name + ); + + // Compare the whole content. + similar_asserts::assert_eq!( + serde_json::to_string_pretty(&program_1).unwrap(), + serde_json::to_string_pretty(&program_2).unwrap(), + "optimization not idempotent for test program '{}'", + package.name + ); + } else { + // Just compare hashes, which would just state that the program failed. + // Then we can use the filter option to zoom in one one to see why. + assert!( + fxhash::hash64(&program_1) == fxhash::hash64(&program_2), + "optimization not idempotent for test program '{}'", + package.name + ); + } + } + }); } } diff --git a/tooling/nargo_toml/src/lib.rs b/tooling/nargo_toml/src/lib.rs index c0d8c7997fd..c1990dab4a6 100644 --- a/tooling/nargo_toml/src/lib.rs +++ b/tooling/nargo_toml/src/lib.rs @@ -469,7 +469,7 @@ fn resolve_package_from_toml( result } -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Clone)] pub enum PackageSelection { Selected(CrateName), DefaultOrAll, From 73e11c629cac3b1308d1f48ea2db74accf800f97 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 3 Dec 2024 23:14:46 +0000 Subject: [PATCH 7/9] Remove leftover filter --- tooling/nargo_cli/src/cli/compile_cmd.rs | 98 ++++++++++++------------ 1 file changed, 48 insertions(+), 50 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 0d85ef2d194..3317cd34e85 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -331,8 +331,9 @@ mod tests { use crate::cli::compile_cmd::{get_target_width, parse_workspace, read_workspace}; - /// Try to find the directory that Cargo sets when it is running; otherwise fallback to assuming the CWD - /// is the root of the repository and append the crate path + /// Try to find the directory that Cargo sets when it is running; + /// otherwise fallback to assuming the CWD is the root of the repository + /// and append the crate path. fn test_programs_dir() -> PathBuf { let root_dir = match std::env::var("CARGO_MANIFEST_DIR") { Ok(dir) => PathBuf::from(dir).parent().unwrap().parent().unwrap().to_path_buf(), @@ -395,54 +396,51 @@ mod tests { assert!(!test_workspaces.is_empty(), "should find some test workspaces"); - test_workspaces - .par_iter() - //.filter(|workspace| workspace.members.iter().any(|p| p.name == test_workspace_name)) - .for_each(|workspace| { - let (file_manager, parsed_files) = parse_workspace(workspace); - let binary_packages = workspace.into_iter().filter(|package| package.is_binary()); - - for package in binary_packages { - let (program_0, _warnings) = compile_program( - &file_manager, - &parsed_files, - workspace, - package, - &CompileOptions::default(), - None, - ) - .expect("failed to compile"); - - let width = get_target_width(package.expression_width, None); - - let program_1 = nargo::ops::transform_program(program_0, width); - let program_2 = nargo::ops::transform_program(program_1.clone(), width); - - if verbose { - // Compare where the most likely difference is. - assert_eq!( - program_1.program, program_2.program, - "optimization not idempotent for test program '{}'", - package.name - ); - - // Compare the whole content. - similar_asserts::assert_eq!( - serde_json::to_string_pretty(&program_1).unwrap(), - serde_json::to_string_pretty(&program_2).unwrap(), - "optimization not idempotent for test program '{}'", - package.name - ); - } else { - // Just compare hashes, which would just state that the program failed. - // Then we can use the filter option to zoom in one one to see why. - assert!( - fxhash::hash64(&program_1) == fxhash::hash64(&program_2), - "optimization not idempotent for test program '{}'", - package.name - ); - } + test_workspaces.par_iter().for_each(|workspace| { + let (file_manager, parsed_files) = parse_workspace(workspace); + let binary_packages = workspace.into_iter().filter(|package| package.is_binary()); + + for package in binary_packages { + let (program_0, _warnings) = compile_program( + &file_manager, + &parsed_files, + workspace, + package, + &CompileOptions::default(), + None, + ) + .expect("failed to compile"); + + let width = get_target_width(package.expression_width, None); + + let program_1 = nargo::ops::transform_program(program_0, width); + let program_2 = nargo::ops::transform_program(program_1.clone(), width); + + if verbose { + // Compare where the most likely difference is. + assert_eq!( + program_1.program, program_2.program, + "optimization not idempotent for test program '{}'", + package.name + ); + + // Compare the whole content. + similar_asserts::assert_eq!( + serde_json::to_string_pretty(&program_1).unwrap(), + serde_json::to_string_pretty(&program_2).unwrap(), + "optimization not idempotent for test program '{}'", + package.name + ); + } else { + // Just compare hashes, which would just state that the program failed. + // Then we can use the filter option to zoom in one one to see why. + assert!( + fxhash::hash64(&program_1) == fxhash::hash64(&program_2), + "optimization not idempotent for test program '{}'", + package.name + ); } - }); + } + }); } } From 82b38bd33016776fc3a84b692adee033a4f2023d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 5 Dec 2024 17:17:59 +0000 Subject: [PATCH 8/9] fix: Make `nargo::ops::transform_program` idempotent (#6695) --- Cargo.lock | 1 + acvm-repo/acvm/Cargo.toml | 2 +- acvm-repo/acvm/src/compiler/mod.rs | 41 ++- acvm-repo/acvm/src/compiler/optimizers/mod.rs | 2 +- .../acvm/src/compiler/transformers/mod.rs | 332 +++++++++++++++++- tooling/nargo_cli/src/cli/compile_cmd.rs | 6 + 6 files changed, 366 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2f19ed704b2..f414a126495 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -48,6 +48,7 @@ dependencies = [ "ark-bn254", "bn254_blackbox_solver", "brillig_vm", + "fxhash", "indexmap 1.9.3", "num-bigint", "proptest", diff --git a/acvm-repo/acvm/Cargo.toml b/acvm-repo/acvm/Cargo.toml index e513ae4e727..ba01ac8ec16 100644 --- a/acvm-repo/acvm/Cargo.toml +++ b/acvm-repo/acvm/Cargo.toml @@ -17,7 +17,7 @@ workspace = true thiserror.workspace = true tracing.workspace = true serde.workspace = true - +fxhash.workspace = true acir.workspace = true brillig_vm.workspace = true acvm_blackbox_solver.workspace = true diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 8829f77e50b..e32c0665c0f 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -16,6 +16,10 @@ pub use simulator::CircuitSimulator; use transformers::transform_internal; pub use transformers::{transform, MIN_EXPRESSION_WIDTH}; +/// We need multiple passes to stabilize the output. +/// The value was determined by running tests. +const MAX_OPTIMIZER_PASSES: usize = 3; + /// This module moves and decomposes acir opcodes. The transformation map allows consumers of this module to map /// metadata they had about the opcodes to the new opcode structure generated after the transformation. #[derive(Debug)] @@ -28,9 +32,9 @@ impl AcirTransformationMap { /// Builds a map from a vector of pointers to the old acir opcodes. /// The index of the vector is the new opcode index. /// The value of the vector is the old opcode index pointed. - fn new(acir_opcode_positions: Vec) -> Self { + fn new(acir_opcode_positions: &[usize]) -> Self { let mut old_indices_to_new_indices = HashMap::with_capacity(acir_opcode_positions.len()); - for (new_index, old_index) in acir_opcode_positions.into_iter().enumerate() { + for (new_index, old_index) in acir_opcode_positions.iter().copied().enumerate() { old_indices_to_new_indices.entry(old_index).or_insert_with(Vec::new).push(new_index); } AcirTransformationMap { old_indices_to_new_indices } @@ -72,17 +76,42 @@ fn transform_assert_messages( } /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`]. +/// +/// Runs multiple passes until the output stabilizes. pub fn compile( acir: Circuit, expression_width: ExpressionWidth, ) -> (Circuit, AcirTransformationMap) { - let (acir, acir_opcode_positions) = optimize_internal(acir); + let mut pass = 0; + let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); + let mut prev_acir = acir; + + // For most test programs it would be enough to only loop `transform_internal`, + // but some of them don't stabilize unless we also repeat the backend agnostic optimizations. + let (mut acir, acir_opcode_positions) = loop { + let (acir, acir_opcode_positions) = optimize_internal(prev_acir); + + // Stop if we have already done at least one transform and an extra optimization changed nothing. + if pass > 0 && prev_opcodes_hash == fxhash::hash64(&acir.opcodes) { + break (acir, acir_opcode_positions); + } - let (mut acir, acir_opcode_positions) = - transform_internal(acir, expression_width, acir_opcode_positions); + let (acir, acir_opcode_positions) = + transform_internal(acir, expression_width, acir_opcode_positions); + + let opcodes_hash = fxhash::hash64(&acir.opcodes); + + // Stop if the output hasn't change in this loop or we went too long. + if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash { + break (acir, acir_opcode_positions); + } - let transformation_map = AcirTransformationMap::new(acir_opcode_positions); + pass += 1; + prev_acir = acir; + prev_opcodes_hash = opcodes_hash; + }; + let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); (acir, transformation_map) diff --git a/acvm-repo/acvm/src/compiler/optimizers/mod.rs b/acvm-repo/acvm/src/compiler/optimizers/mod.rs index 1947a80dc35..f86bf500998 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/mod.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/mod.rs @@ -23,7 +23,7 @@ use super::{transform_assert_messages, AcirTransformationMap}; pub fn optimize(acir: Circuit) -> (Circuit, AcirTransformationMap) { let (mut acir, new_opcode_positions) = optimize_internal(acir); - let transformation_map = AcirTransformationMap::new(new_opcode_positions); + let transformation_map = AcirTransformationMap::new(&new_opcode_positions); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); diff --git a/acvm-repo/acvm/src/compiler/transformers/mod.rs b/acvm-repo/acvm/src/compiler/transformers/mod.rs index c9ce4ac7895..e7e8f885710 100644 --- a/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -1,5 +1,10 @@ use acir::{ - circuit::{brillig::BrilligOutputs, Circuit, ExpressionWidth, Opcode}, + circuit::{ + self, + brillig::{BrilligInputs, BrilligOutputs}, + opcodes::{BlackBoxFuncCall, FunctionInput, MemOp}, + Circuit, ExpressionWidth, Opcode, + }, native_types::{Expression, Witness}, AcirField, }; @@ -12,6 +17,7 @@ pub use csat::MIN_EXPRESSION_WIDTH; use super::{ optimizers::MergeExpressionsOptimizer, transform_assert_messages, AcirTransformationMap, + MAX_OPTIMIZER_PASSES, }; /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`]. @@ -26,7 +32,7 @@ pub fn transform( let (mut acir, acir_opcode_positions) = transform_internal(acir, expression_width, acir_opcode_positions); - let transformation_map = AcirTransformationMap::new(acir_opcode_positions); + let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map); @@ -36,9 +42,52 @@ pub fn transform( /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`]. /// /// Accepts an injected `acir_opcode_positions` to allow transformations to be applied directly after optimizations. +/// +/// Does multiple passes until the output stabilizes. #[tracing::instrument(level = "trace", name = "transform_acir", skip(acir, acir_opcode_positions))] pub(super) fn transform_internal( - acir: Circuit, + mut acir: Circuit, + expression_width: ExpressionWidth, + mut acir_opcode_positions: Vec, +) -> (Circuit, Vec) { + // Allow multiple passes until we have stable output. + let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); + + // For most test programs it would be enough to loop here, but some of them + // don't stabilize unless we also repeat the backend agnostic optimizations. + for _ in 0..MAX_OPTIMIZER_PASSES { + let (new_acir, new_acir_opcode_positions) = + transform_internal_once(acir, expression_width, acir_opcode_positions); + + acir = new_acir; + acir_opcode_positions = new_acir_opcode_positions; + + let new_opcodes_hash = fxhash::hash64(&acir.opcodes); + + if new_opcodes_hash == prev_opcodes_hash { + break; + } + prev_opcodes_hash = new_opcodes_hash; + } + // After the elimination of intermediate variables the `current_witness_index` is potentially higher than it needs to be, + // which would cause gaps if we ran the optimization a second time, making it look like new variables were added. + acir.current_witness_index = max_witness(&acir).witness_index(); + + (acir, acir_opcode_positions) +} + +/// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`]. +/// +/// Accepts an injected `acir_opcode_positions` to allow transformations to be applied directly after optimizations. +/// +/// Does a single optimization pass. +#[tracing::instrument( + level = "trace", + name = "transform_acir_once", + skip(acir, acir_opcode_positions) +)] +fn transform_internal_once( + mut acir: Circuit, expression_width: ExpressionWidth, acir_opcode_positions: Vec, ) -> (Circuit, Vec) { @@ -79,8 +128,6 @@ pub(super) fn transform_internal( &mut next_witness_index, ); - // Update next_witness counter - next_witness_index += (intermediate_variables.len() - len) as u32; let mut new_opcodes = Vec::new(); for (g, (norm, w)) in intermediate_variables.iter().skip(len) { // de-normalize @@ -150,23 +197,288 @@ pub(super) fn transform_internal( let current_witness_index = next_witness_index - 1; - let acir = Circuit { + acir = Circuit { current_witness_index, expression_width, opcodes: transformed_opcodes, // The transformer does not add new public inputs ..acir }; + let mut merge_optimizer = MergeExpressionsOptimizer::new(); + let (opcodes, new_acir_opcode_positions) = merge_optimizer.eliminate_intermediate_variable(&acir, new_acir_opcode_positions); - // n.b. we do not update current_witness_index after the eliminate_intermediate_variable pass, the real index could be less. - let acir = Circuit { - current_witness_index, - expression_width, + + // n.b. if we do not update current_witness_index after the eliminate_intermediate_variable pass, the real index could be less. + acir = Circuit { opcodes, // The optimizer does not add new public inputs ..acir }; + (acir, new_acir_opcode_positions) } + +/// Find the witness with the highest ID in the circuit. +fn max_witness(circuit: &Circuit) -> Witness { + let mut witnesses = WitnessFolder::new(Witness::default(), |state, witness| { + *state = witness.max(*state); + }); + witnesses.fold_circuit(circuit); + witnesses.into_state() +} + +/// Fold all witnesses in a circuit. +struct WitnessFolder { + state: S, + accumulate: A, +} + +impl WitnessFolder +where + A: Fn(&mut S, Witness), +{ + /// Create the folder with some initial state and an accumulator function. + fn new(init: S, accumulate: A) -> Self { + Self { state: init, accumulate } + } + + /// Take the accumulated state. + fn into_state(self) -> S { + self.state + } + + /// Add all witnesses from the circuit. + fn fold_circuit(&mut self, circuit: &Circuit) { + self.fold_many(circuit.private_parameters.iter()); + self.fold_many(circuit.public_parameters.0.iter()); + self.fold_many(circuit.return_values.0.iter()); + for opcode in &circuit.opcodes { + self.fold_opcode(opcode); + } + } + + /// Fold a witness into the state. + fn fold(&mut self, witness: Witness) { + (self.accumulate)(&mut self.state, witness); + } + + /// Fold many witnesses into the state. + fn fold_many<'w, I: Iterator>(&mut self, witnesses: I) { + for w in witnesses { + self.fold(*w); + } + } + + /// Add witnesses from the opcode. + fn fold_opcode(&mut self, opcode: &Opcode) { + match opcode { + Opcode::AssertZero(expr) => { + self.fold_expr(expr); + } + Opcode::BlackBoxFuncCall(call) => self.fold_blackbox(call), + Opcode::MemoryOp { block_id: _, op, predicate } => { + let MemOp { operation, index, value } = op; + self.fold_expr(operation); + self.fold_expr(index); + self.fold_expr(value); + if let Some(pred) = predicate { + self.fold_expr(pred); + } + } + Opcode::MemoryInit { block_id: _, init, block_type: _ } => { + for w in init { + self.fold(*w); + } + } + // We keep the display for a BrilligCall and circuit Call separate as they + // are distinct in their functionality and we should maintain this separation for debugging. + Opcode::BrilligCall { id: _, inputs, outputs, predicate } => { + if let Some(pred) = predicate { + self.fold_expr(pred); + } + self.fold_brillig_inputs(inputs); + self.fold_brillig_outputs(outputs); + } + Opcode::Call { id: _, inputs, outputs, predicate } => { + if let Some(pred) = predicate { + self.fold_expr(pred); + } + self.fold_many(inputs.iter()); + self.fold_many(outputs.iter()); + } + } + } + + fn fold_expr(&mut self, expr: &Expression) { + for i in &expr.mul_terms { + self.fold(i.1); + self.fold(i.2); + } + for i in &expr.linear_combinations { + self.fold(i.1); + } + } + + fn fold_brillig_inputs(&mut self, inputs: &[BrilligInputs]) { + for input in inputs { + match input { + BrilligInputs::Single(expr) => { + self.fold_expr(expr); + } + BrilligInputs::Array(exprs) => { + for expr in exprs { + self.fold_expr(expr); + } + } + BrilligInputs::MemoryArray(_) => {} + } + } + } + + fn fold_brillig_outputs(&mut self, outputs: &[BrilligOutputs]) { + for output in outputs { + match output { + BrilligOutputs::Simple(w) => { + self.fold(*w); + } + BrilligOutputs::Array(ws) => self.fold_many(ws.iter()), + } + } + } + + fn fold_blackbox(&mut self, call: &BlackBoxFuncCall) { + match call { + BlackBoxFuncCall::AES128Encrypt { inputs, iv, key, outputs } => { + self.fold_function_inputs(inputs.as_slice()); + self.fold_function_inputs(iv.as_slice()); + self.fold_function_inputs(key.as_slice()); + self.fold_many(outputs.iter()); + } + BlackBoxFuncCall::AND { lhs, rhs, output } => { + self.fold_function_input(lhs); + self.fold_function_input(rhs); + self.fold(*output); + } + BlackBoxFuncCall::XOR { lhs, rhs, output } => { + self.fold_function_input(lhs); + self.fold_function_input(rhs); + self.fold(*output); + } + BlackBoxFuncCall::RANGE { input } => { + self.fold_function_input(input); + } + BlackBoxFuncCall::Blake2s { inputs, outputs } => { + self.fold_function_inputs(inputs.as_slice()); + self.fold_many(outputs.iter()); + } + BlackBoxFuncCall::Blake3 { inputs, outputs } => { + self.fold_function_inputs(inputs.as_slice()); + self.fold_many(outputs.iter()); + } + BlackBoxFuncCall::SchnorrVerify { + public_key_x, + public_key_y, + signature, + message, + output, + } => { + self.fold_function_input(public_key_x); + self.fold_function_input(public_key_y); + self.fold_function_inputs(signature.as_slice()); + self.fold_function_inputs(message.as_slice()); + self.fold(*output); + } + BlackBoxFuncCall::EcdsaSecp256k1 { + public_key_x, + public_key_y, + signature, + hashed_message, + output, + } => { + self.fold_function_inputs(public_key_x.as_slice()); + self.fold_function_inputs(public_key_y.as_slice()); + self.fold_function_inputs(signature.as_slice()); + self.fold_function_inputs(hashed_message.as_slice()); + self.fold(*output); + } + BlackBoxFuncCall::EcdsaSecp256r1 { + public_key_x, + public_key_y, + signature, + hashed_message, + output, + } => { + self.fold_function_inputs(public_key_x.as_slice()); + self.fold_function_inputs(public_key_y.as_slice()); + self.fold_function_inputs(signature.as_slice()); + self.fold_function_inputs(hashed_message.as_slice()); + self.fold(*output); + } + BlackBoxFuncCall::MultiScalarMul { points, scalars, outputs } => { + self.fold_function_inputs(points.as_slice()); + self.fold_function_inputs(scalars.as_slice()); + let (x, y, i) = outputs; + self.fold(*x); + self.fold(*y); + self.fold(*i); + } + BlackBoxFuncCall::EmbeddedCurveAdd { input1, input2, outputs } => { + self.fold_function_inputs(input1.as_slice()); + self.fold_function_inputs(input2.as_slice()); + let (x, y, i) = outputs; + self.fold(*x); + self.fold(*y); + self.fold(*i); + } + BlackBoxFuncCall::Keccakf1600 { inputs, outputs } => { + self.fold_function_inputs(inputs.as_slice()); + self.fold_many(outputs.iter()); + } + BlackBoxFuncCall::RecursiveAggregation { + verification_key, + proof, + public_inputs, + key_hash, + proof_type: _, + } => { + self.fold_function_inputs(verification_key.as_slice()); + self.fold_function_inputs(proof.as_slice()); + self.fold_function_inputs(public_inputs.as_slice()); + self.fold_function_input(key_hash); + } + BlackBoxFuncCall::BigIntAdd { .. } + | BlackBoxFuncCall::BigIntSub { .. } + | BlackBoxFuncCall::BigIntMul { .. } + | BlackBoxFuncCall::BigIntDiv { .. } => {} + BlackBoxFuncCall::BigIntFromLeBytes { inputs, modulus: _, output: _ } => { + self.fold_function_inputs(inputs.as_slice()); + } + BlackBoxFuncCall::BigIntToLeBytes { input: _, outputs } => { + self.fold_many(outputs.iter()); + } + BlackBoxFuncCall::Poseidon2Permutation { inputs, outputs, len: _ } => { + self.fold_function_inputs(inputs.as_slice()); + self.fold_many(outputs.iter()); + } + BlackBoxFuncCall::Sha256Compression { inputs, hash_values, outputs } => { + self.fold_function_inputs(inputs.as_slice()); + self.fold_function_inputs(hash_values.as_slice()); + self.fold_many(outputs.iter()); + } + } + } + + fn fold_function_input(&mut self, input: &FunctionInput) { + if let circuit::opcodes::ConstantOrWitnessEnum::Witness(witness) = input.input() { + self.fold(witness); + } + } + + fn fold_function_inputs(&mut self, inputs: &[FunctionInput]) { + for input in inputs { + self.fold_function_input(input); + } + } +} diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 3317cd34e85..f134374f89e 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -418,6 +418,12 @@ mod tests { if verbose { // Compare where the most likely difference is. + similar_asserts::assert_eq!( + format!("{}", program_1.program), + format!("{}", program_2.program), + "optimization not idempotent for test program '{}'", + package.name + ); assert_eq!( program_1.program, program_2.program, "optimization not idempotent for test program '{}'", From 274bdd14240936faf2938b09715be5ab70cdfa2f Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 9 Dec 2024 12:00:41 +0000 Subject: [PATCH 9/9] Update acvm-repo/acvm/src/compiler/transformers/mod.rs --- acvm-repo/acvm/src/compiler/transformers/mod.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/transformers/mod.rs b/acvm-repo/acvm/src/compiler/transformers/mod.rs index e7e8f885710..a499aec1b30 100644 --- a/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -377,19 +377,6 @@ where self.fold_function_inputs(inputs.as_slice()); self.fold_many(outputs.iter()); } - BlackBoxFuncCall::SchnorrVerify { - public_key_x, - public_key_y, - signature, - message, - output, - } => { - self.fold_function_input(public_key_x); - self.fold_function_input(public_key_y); - self.fold_function_inputs(signature.as_slice()); - self.fold_function_inputs(message.as_slice()); - self.fold(*output); - } BlackBoxFuncCall::EcdsaSecp256k1 { public_key_x, public_key_y,