Skip to content

Commit

Permalink
feat(nargo): Default expression width field in Nargo.toml (#5505)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #4961 

## Summary\*

This PR let's a developer override the default expression width target
with a field in the `Nargo.toml` manifest.

Due to the limitations of the `clap` macros I had to switch
`expression_width` on the `CompileOptions` struct to optional. I then
hardcoded a constant that specifies we want width 4 as Noir's
compilation default. We then settle on a target expression width by
checking the following:
1. Is there a user supplied expression width from the CLI flag? If there
is use that width.
2. Is there no user supplied width and an expression width specified in
the manifest? Use the expression width in the manifest.
3. Is there no user supplied width and no width specified in the
manifest? Use the hardcoded default value.

## Additional Context

## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <[email protected]>
  • Loading branch information
vezenovm and TomAFrench authored Jul 15, 2024
1 parent 13d1875 commit dea6b32
Show file tree
Hide file tree
Showing 18 changed files with 126 additions and 32 deletions.
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 };
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
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::io::Write;
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 @@ -190,7 +191,11 @@ fn compile_programs(
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 @@ fn compiled_contracts(
.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 @@ fn save_contract(
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 @@ fn run_stdlib_tests() {
entry_path: PathBuf::from("main.nr"),
name: "stdlib".parse().unwrap(),
dependencies: BTreeMap::new(),
expression_width: None,
};

let (mut context, dummy_crate_id) =
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

0 comments on commit dea6b32

Please sign in to comment.