Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(nargo): Default expression width field in Nargo.toml #5505

Merged
merged 9 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ pub const NOIR_ARTIFACT_VERSION_STRING: &str =

#[derive(Args, Clone, Debug, Default)]
pub struct CompileOptions {
/// Override the expression width requested by the backend.
#[arg(long, value_parser = parse_expression_width, default_value = "4")]
pub expression_width: ExpressionWidth,
/// Specify the backend expression width that should be targeted
#[arg(long, value_parser = parse_expression_width)]
pub expression_width: Option<ExpressionWidth>,

/// Force a full recompilation.
#[arg(long = "force")]
Expand Down Expand Up @@ -113,7 +113,7 @@ pub struct CompileOptions {
pub show_artifact_paths: bool,
}

fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::Error> {
pub fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::Error> {
use std::io::{Error, ErrorKind};
let width = input
.parse::<usize>()
Expand Down
20 changes: 8 additions & 12 deletions compiler/wasm/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,9 @@ pub fn compile_program(
console_error_panic_hook::set_once();
let (crate_id, mut context) = prepare_context(entry_point, dependency_graph, file_source_map)?;

let compile_options = CompileOptions {
expression_width: ExpressionWidth::Bounded { width: 4 },
..CompileOptions::default()
};
let expression_width = ExpressionWidth::Bounded { width: 4 };
vezenovm marked this conversation as resolved.
Show resolved Hide resolved
let compile_options =
CompileOptions { expression_width: Some(expression_width), ..CompileOptions::default() };

let compiled_program =
noirc_driver::compile_main(&mut context, crate_id, &compile_options, None)
Expand All @@ -180,8 +179,7 @@ pub fn compile_program(
})?
.0;

let optimized_program =
nargo::ops::transform_program(compiled_program, compile_options.expression_width);
let optimized_program = nargo::ops::transform_program(compiled_program, expression_width);
let warnings = optimized_program.warnings.clone();

Ok(JsCompileProgramResult::new(optimized_program.into(), warnings))
Expand All @@ -196,10 +194,9 @@ pub fn compile_contract(
console_error_panic_hook::set_once();
let (crate_id, mut context) = prepare_context(entry_point, dependency_graph, file_source_map)?;

let compile_options = CompileOptions {
expression_width: ExpressionWidth::Bounded { width: 4 },
..CompileOptions::default()
};
let expression_width = ExpressionWidth::Bounded { width: 4 };
let compile_options =
CompileOptions { expression_width: Some(expression_width), ..CompileOptions::default() };

let compiled_contract =
noirc_driver::compile_contract(&mut context, crate_id, &compile_options)
Expand All @@ -212,8 +209,7 @@ pub fn compile_contract(
})?
.0;

let optimized_contract =
nargo::ops::transform_contract(compiled_contract, compile_options.expression_width);
let optimized_contract = nargo::ops::transform_contract(compiled_contract, expression_width);
let warnings = optimized_contract.warnings.clone();

Ok(JsCompileContractResult::new(optimized_contract.into(), warnings))
Expand Down
15 changes: 10 additions & 5 deletions compiler/wasm/src/compile_new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,10 @@ impl CompilerContext {
} else {
ExpressionWidth::Bounded { width: 4 }
};
let compile_options = CompileOptions { expression_width, ..CompileOptions::default() };
let compile_options = CompileOptions {
expression_width: Some(expression_width),
..CompileOptions::default()
};

let root_crate_id = *self.context.root_crate_id();
let compiled_program =
Expand All @@ -114,8 +117,7 @@ impl CompilerContext {
})?
.0;

let optimized_program =
nargo::ops::transform_program(compiled_program, compile_options.expression_width);
let optimized_program = nargo::ops::transform_program(compiled_program, expression_width);
let warnings = optimized_program.warnings.clone();

Ok(JsCompileProgramResult::new(optimized_program.into(), warnings))
Expand All @@ -130,7 +132,10 @@ impl CompilerContext {
} else {
ExpressionWidth::Bounded { width: 4 }
};
let compile_options = CompileOptions { expression_width, ..CompileOptions::default() };
let compile_options = CompileOptions {
expression_width: Some(expression_width),
..CompileOptions::default()
};

let root_crate_id = *self.context.root_crate_id();
let compiled_contract =
Expand All @@ -145,7 +150,7 @@ impl CompilerContext {
.0;

let optimized_contract =
nargo::ops::transform_contract(compiled_contract, compile_options.expression_width);
nargo::ops::transform_contract(compiled_contract, expression_width);
let warnings = optimized_contract.warnings.clone();

Ok(JsCompileContractResult::new(optimized_contract.into(), warnings))
Expand Down
1 change: 1 addition & 0 deletions docs/docs/getting_started/hello_noir/project_breakdown.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ The package section defines a number of fields including:
- `entry` (optional) - a relative filepath to use as the entry point into your package (overrides the default of `src/lib.nr` or `src/main.nr`)
- `backend` (optional)
- `license` (optional)
- `expression_width` (optional) - Sets the default backend expression width. This field will override the default backend expression width specified by the Noir compiler (currently set to width 4).

#### Dependencies section

Expand Down
vezenovm marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[package]
name = "poseidon_bn254_hash_width_3"
type = "bin"
authors = [""]
compiler_version = ">=0.31.0"
# Test usage of `expression_width` field
expression_width = "3"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
x1 = [1,2]
y1 = "0x115cc0f5e7d690413df64c6b9662e9cf2a3617f2743245519e19607a4417189a"
x2 = [1,2,3,4]
y2 = "0x299c867db6c1fdd79dcefa40e4510b9837e60ebb1ce0663dbaa525df65250465"
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
use std::hash::poseidon;

fn main(x1: [Field; 2], y1: pub Field, x2: [Field; 4], y2: pub Field) {
let hash1 = poseidon::bn254::hash_2(x1);
assert(hash1 == y1);

let hash2 = poseidon::bn254::hash_4(x2);
assert(hash2 == y2);
}
1 change: 1 addition & 0 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ pub(crate) fn resolve_workspace_for_source_path(
name: CrateName::from_str(parent_folder)
.map_err(|err| LspError::WorkspaceResolutionError(err.to_string()))?,
dependencies: BTreeMap::new(),
expression_width: None,
};
let workspace = Workspace {
root_dir: PathBuf::from(parent_folder),
Expand Down
2 changes: 2 additions & 0 deletions tooling/nargo/src/package.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{collections::BTreeMap, fmt::Display, path::PathBuf};

use acvm::acir::circuit::ExpressionWidth;
use noirc_frontend::graph::CrateName;

use crate::constants::PROVER_INPUT_FILE;
Expand Down Expand Up @@ -51,6 +52,7 @@ pub struct Package {
pub entry_path: PathBuf,
pub name: CrateName,
pub dependencies: BTreeMap<CrateName, Dependency>,
pub expression_width: Option<ExpressionWidth>,
}

impl Package {
Expand Down
30 changes: 27 additions & 3 deletions tooling/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
use std::path::Path;
use std::time::Duration;

use acvm::acir::circuit::ExpressionWidth;
use fm::FileManager;
use nargo::ops::{collect_errors, compile_contract, compile_program, report_errors};
use nargo::package::Package;
Expand Down Expand Up @@ -69,7 +70,7 @@
fn watch_workspace(workspace: &Workspace, compile_options: &CompileOptions) -> notify::Result<()> {
let (tx, rx) = std::sync::mpsc::channel();

// No specific tickrate, max debounce time 1 seconds

Check warning on line 73 in tooling/nargo_cli/src/cli/compile_cmd.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (tickrate)
let mut debouncer = new_debouncer(Duration::from_secs(1), None, tx)?;

// Add a path to be watched. All files and directories at that path and
Expand All @@ -77,7 +78,7 @@
debouncer.watcher().watch(&workspace.root_dir, RecursiveMode::Recursive)?;

let mut screen = std::io::stdout();
write!(screen, "{}", termion::cursor::Save).unwrap();

Check warning on line 81 in tooling/nargo_cli/src/cli/compile_cmd.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (termion)
screen.flush().unwrap();
let _ = compile_workspace_full(workspace, compile_options);
for res in rx {
Expand All @@ -98,7 +99,7 @@
});

if noir_files_modified {
write!(screen, "{}{}", termion::cursor::Restore, termion::clear::AfterCursor).unwrap();

Check warning on line 102 in tooling/nargo_cli/src/cli/compile_cmd.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (termion)

Check warning on line 102 in tooling/nargo_cli/src/cli/compile_cmd.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (termion)
screen.flush().unwrap();
let _ = compile_workspace_full(workspace, compile_options);
}
Expand Down Expand Up @@ -190,7 +191,11 @@
compile_options,
load_cached_program(package),
)?;
let program = nargo::ops::transform_program(program, compile_options.expression_width);

let target_width =
get_target_width(package.expression_width, compile_options.expression_width);
let program = nargo::ops::transform_program(program, target_width);

save_program_to_file(
&program.clone().into(),
&package.name,
Expand All @@ -216,8 +221,9 @@
.map(|package| {
let (contract, warnings) =
compile_contract(file_manager, parsed_files, package, compile_options)?;
let contract =
nargo::ops::transform_contract(contract, compile_options.expression_width);
let target_width =
get_target_width(package.expression_width, compile_options.expression_width);
let contract = nargo::ops::transform_contract(contract, target_width);
save_contract(contract, package, target_dir, compile_options.show_artifact_paths);
Ok(((), warnings))
})
Expand All @@ -243,3 +249,21 @@
println!("Saved contract artifact to: {}", artifact_path.display());
}
}

/// Default expression width used for Noir compilation.
/// The ACVM native type `ExpressionWidth` has its own default which should always be unbounded,
/// while we can sometimes expect the compilation target width to change.
/// Thus, we set it separately here rather than trying to alter the default derivation of the type.
const DEFAULT_EXPRESSION_WIDTH: ExpressionWidth = ExpressionWidth::Bounded { width: 4 };

/// If a target width was not specified in the CLI we can safely override the default.
pub(crate) fn get_target_width(
package_default_width: Option<ExpressionWidth>,
compile_options_width: Option<ExpressionWidth>,
) -> ExpressionWidth {
if let (Some(manifest_default_width), None) = (package_default_width, compile_options_width) {
manifest_default_width
} else {
compile_options_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH)
}
}
7 changes: 5 additions & 2 deletions tooling/nargo_cli/src/cli/debug_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use noirc_frontend::debug::DebugInstrumenter;
use noirc_frontend::graph::CrateName;
use noirc_frontend::hir::ParsedFiles;

use super::compile_cmd::get_target_width;
use super::fs::{inputs::read_inputs_from_file, witness::save_witness_to_dir};
use super::NargoConfig;
use crate::errors::CliError;
Expand Down Expand Up @@ -80,8 +81,10 @@ pub(crate) fn run(args: DebugCommand, config: NargoConfig) -> Result<(), CliErro
args.compile_options.clone(),
)?;

let compiled_program =
nargo::ops::transform_program(compiled_program, args.compile_options.expression_width);
let target_width =
get_target_width(package.expression_width, args.compile_options.expression_width);

let compiled_program = nargo::ops::transform_program(compiled_program, target_width);

run_async(package, compiled_program, &args.prover_name, &args.witness_name, target_dir)
}
Expand Down
12 changes: 6 additions & 6 deletions tooling/nargo_cli/src/cli/info_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ use serde::Serialize;
use crate::errors::CliError;

use super::{
compile_cmd::compile_workspace_full, fs::program::read_program_from_file, NargoConfig,
compile_cmd::{compile_workspace_full, get_target_width},
fs::program::read_program_from_file,
NargoConfig,
};

/// Provides detailed information on each of a program's function (represented by a single circuit)
Expand Down Expand Up @@ -84,11 +86,9 @@ pub(crate) fn run(args: InfoCommand, config: NargoConfig) -> Result<(), CliError
.into_iter()
.par_bridge()
.map(|(package, program)| {
count_opcodes_and_gates_in_program(
program,
&package,
args.compile_options.expression_width,
)
let target_width =
get_target_width(package.expression_width, args.compile_options.expression_width);
count_opcodes_and_gates_in_program(program, &package, target_width)
})
.collect();

Expand Down
1 change: 1 addition & 0 deletions tooling/nargo_cli/tests/stdlib-tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
entry_path: PathBuf::from("main.nr"),
name: "stdlib".parse().unwrap(),
dependencies: BTreeMap::new(),
expression_width: None,
};

let (mut context, dummy_crate_id) =
Expand Down Expand Up @@ -63,7 +64,7 @@
&CompileOptions::default(),
)
} else {
use noir_fuzzer::FuzzedExecutor;

Check warning on line 67 in tooling/nargo_cli/tests/stdlib-tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (fuzzer)
use proptest::test_runner::TestRunner;

let compiled_program = compile_no_check(
Expand All @@ -77,9 +78,9 @@
Ok(compiled_program) => {
let runner = TestRunner::default();

let fuzzer = FuzzedExecutor::new(compiled_program.into(), runner);

Check warning on line 81 in tooling/nargo_cli/tests/stdlib-tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (fuzzer)

let result = fuzzer.fuzz();

Check warning on line 83 in tooling/nargo_cli/tests/stdlib-tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (fuzzer)
if result.success {
TestStatus::Pass
} else {
Expand Down
1 change: 1 addition & 0 deletions tooling/nargo_toml/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ serde.workspace = true
thiserror.workspace = true
toml.workspace = true
url.workspace = true
noirc_driver.workspace = true
semver = "1.0.20"

[dev-dependencies]
3 changes: 3 additions & 0 deletions tooling/nargo_toml/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ pub enum ManifestError {

#[error("Cyclic package dependency found when processing {cycle}")]
CyclicDependency { cycle: String },

#[error("Failed to parse expression width with the following error: {0}")]
ParseExpressionWidth(String),
}

#[allow(clippy::enum_variant_names)]
Expand Down
28 changes: 28 additions & 0 deletions tooling/nargo_toml/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use nargo::{
package::{Dependency, Package, PackageType},
workspace::Workspace,
};
use noirc_driver::parse_expression_width;
use noirc_frontend::graph::CrateName;
use serde::Deserialize;

Expand Down Expand Up @@ -199,6 +200,16 @@ impl PackageConfig {
})?;
}

let expression_width = self
.package
.expression_width
.as_ref()
.map(|expression_width| {
parse_expression_width(expression_width)
.map_err(|err| ManifestError::ParseExpressionWidth(err.to_string()))
})
.map_or(Ok(None), |res| res.map(Some))?;

Ok(Package {
version: self.package.version.clone(),
compiler_required_version: self.package.compiler_version.clone(),
Expand All @@ -207,6 +218,7 @@ impl PackageConfig {
package_type,
name,
dependencies,
expression_width,
})
}
}
Expand Down Expand Up @@ -275,6 +287,7 @@ struct PackageMetadata {
// so you will not need to supply an ACIR and compiler version
compiler_version: Option<String>,
license: Option<String>,
expression_width: Option<String>,
}

#[derive(Debug, Deserialize, Clone)]
Expand Down Expand Up @@ -531,3 +544,18 @@ fn parse_workspace_default_member_toml() {
assert!(Config::try_from(String::from(src)).is_ok());
assert!(Config::try_from(src).is_ok());
}

#[test]
fn parse_package_expression_width_toml() {
let src = r#"
[package]
name = "test"
version = "0.1.0"
type = "bin"
authors = [""]
expression_width = 3
"#;

assert!(Config::try_from(String::from(src)).is_ok());
assert!(Config::try_from(src).is_ok());
}
Loading
Loading