From fc4b3589b42c866dc971058f89af0cd39a58745d Mon Sep 17 00:00:00 2001
From: Akosh Farkash <aakoshh@gmail.com>
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<Workspace, ManifestError> {
+ 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<Item = PathBuf> {
+ 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::<Vec<_>>();
+
+ 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 <aakoshh@gmail.com>
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 <aakoshh@gmail.com>
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 <aakoshh@gmail.com>
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 <aakoshh@gmail.com>
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 <aakoshh@gmail.com>
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<String>,
+ }
+
+ 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::<Vec<_>>();
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 <aakoshh@gmail.com>
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 <aakoshh@gmail.com>
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<usize>) -> 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<F: Clone>(
}
/// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`].
+///
+/// Runs multiple passes until the output stabilizes.
pub fn compile<F: AcirField>(
acir: Circuit<F>,
expression_width: ExpressionWidth,
) -> (Circuit<F>, 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<F: AcirField>(acir: Circuit<F>) -> (Circuit<F>, 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<F: AcirField>(
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<F: AcirField>(
/// 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<F: AcirField>(
- acir: Circuit<F>,
+ mut acir: Circuit<F>,
+ expression_width: ExpressionWidth,
+ mut acir_opcode_positions: Vec<usize>,
+) -> (Circuit<F>, Vec<usize>) {
+ // 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<F: AcirField>(
+ mut acir: Circuit<F>,
expression_width: ExpressionWidth,
acir_opcode_positions: Vec<usize>,
) -> (Circuit<F>, Vec<usize>) {
@@ -79,8 +128,6 @@ pub(super) fn transform_internal<F: AcirField>(
&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<F: AcirField>(
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<F: AcirField>(circuit: &Circuit<F>) -> 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<S, A> {
+ state: S,
+ accumulate: A,
+}
+
+impl<S, A> WitnessFolder<S, A>
+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<F: AcirField>(&mut self, circuit: &Circuit<F>) {
+ 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<Item = &'w Witness>>(&mut self, witnesses: I) {
+ for w in witnesses {
+ self.fold(*w);
+ }
+ }
+
+ /// Add witnesses from the opcode.
+ fn fold_opcode<F: AcirField>(&mut self, opcode: &Opcode<F>) {
+ 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<F: AcirField>(&mut self, expr: &Expression<F>) {
+ 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<F: AcirField>(&mut self, inputs: &[BrilligInputs<F>]) {
+ 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<F: AcirField>(&mut self, call: &BlackBoxFuncCall<F>) {
+ 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<F: AcirField>(&mut self, input: &FunctionInput<F>) {
+ if let circuit::opcodes::ConstantOrWitnessEnum::Witness(witness) = input.input() {
+ self.fold(witness);
+ }
+ }
+
+ fn fold_function_inputs<F: AcirField>(&mut self, inputs: &[FunctionInput<F>]) {
+ 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,