Skip to content

Commit

Permalink
feat: remove query to backend to get expression width (#4975)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Progress on #4961 

## Summary\*

This PR removes the query to the backend in order to get the expression
width, for now we just have a blanket default of ~4~ 3 (Looks like we're
still waiting to update to a version of `bb` which uses 4). I've decided
to split the ability to specify a package-level override of this default
as I'd need to do some mucky stuff related to `compile_options`.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.
We don't have docs related to the expression width afaik.
# 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.
  • Loading branch information
TomAFrench authored May 7, 2024
1 parent c49d3a9 commit e5f356b
Show file tree
Hide file tree
Showing 22 changed files with 54 additions and 243 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

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

4 changes: 2 additions & 2 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ 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)]
pub expression_width: Option<ExpressionWidth>,
#[arg(long, value_parser = parse_expression_width, default_value = "3")]
pub expression_width: ExpressionWidth,

/// Force a full recompilation.
#[arg(long = "force")]
Expand Down
1 change: 0 additions & 1 deletion tooling/backend_interface/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ license.workspace = true
acvm.workspace = true
dirs.workspace = true
thiserror.workspace = true
serde.workspace = true
serde_json.workspace = true
bb_abstraction_leaks.workspace = true
tracing.workspace = true
Expand Down
62 changes: 0 additions & 62 deletions tooling/backend_interface/src/cli/info.rs

This file was deleted.

2 changes: 0 additions & 2 deletions tooling/backend_interface/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

mod contract;
mod gates;
mod info;
mod proof_as_fields;
mod prove;
mod verify;
Expand All @@ -12,7 +11,6 @@ mod write_vk;

pub(crate) use contract::ContractCommand;
pub(crate) use gates::GatesCommand;
pub(crate) use info::InfoCommand;
pub(crate) use proof_as_fields::ProofAsFieldsCommand;
pub(crate) use prove::ProveCommand;
pub(crate) use verify::VerifyCommand;
Expand Down
25 changes: 3 additions & 22 deletions tooling/backend_interface/src/proof_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@ use std::io::Write;
use std::path::Path;

use acvm::acir::{
circuit::{ExpressionWidth, Program},
circuit::Program,
native_types::{WitnessMap, WitnessStack},
};
use acvm::FieldElement;
use tempfile::tempdir;
use tracing::warn;

use crate::cli::{
GatesCommand, InfoCommand, ProofAsFieldsCommand, ProveCommand, VerifyCommand,
VkAsFieldsCommand, WriteVkCommand,
GatesCommand, ProofAsFieldsCommand, ProveCommand, VerifyCommand, VkAsFieldsCommand,
WriteVkCommand,
};
use crate::{Backend, BackendError};

Expand All @@ -33,25 +33,6 @@ impl Backend {
.run(binary_path)
}

pub fn get_backend_info(&self) -> Result<ExpressionWidth, BackendError> {
let binary_path = self.assert_binary_exists()?;
self.assert_correct_version()?;
InfoCommand { crs_path: self.crs_directory() }.run(binary_path)
}

/// If we cannot get a valid backend, returns `ExpressionWidth::Bound { width: 4 }``
/// The function also prints a message saying we could not find a backend
pub fn get_backend_info_or_default(&self) -> ExpressionWidth {
if let Ok(expression_width) = self.get_backend_info() {
expression_width
} else {
warn!(
"No valid backend found, ExpressionWidth defaulting to Bounded with a width of 4"
);
ExpressionWidth::Bounded { width: 4 }
}
}

#[tracing::instrument(level = "trace", skip_all)]
pub fn prove(
&self,
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use clap::{Parser, Subcommand};

mod contract_cmd;
mod gates_cmd;
mod info_cmd;
mod prove_cmd;
mod verify_cmd;
mod write_vk_cmd;
Expand All @@ -21,7 +20,6 @@ struct BackendCli {

#[derive(Subcommand, Clone, Debug)]
enum BackendCommand {
Info(info_cmd::InfoCommand),
Contract(contract_cmd::ContractCommand),
Gates(gates_cmd::GatesCommand),
Prove(prove_cmd::ProveCommand),
Expand All @@ -34,7 +32,6 @@ fn main() {
let BackendCli { command } = BackendCli::parse();

match command {
BackendCommand::Info(args) => info_cmd::run(args),
BackendCommand::Contract(args) => contract_cmd::run(args),
BackendCommand::Gates(args) => gates_cmd::run(args),
BackendCommand::Prove(args) => prove_cmd::run(args),
Expand Down
7 changes: 1 addition & 6 deletions tooling/nargo_cli/src/cli/check_cmd.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::backends::Backend;
use crate::errors::CliError;

use clap::Args;
Expand Down Expand Up @@ -42,11 +41,7 @@ pub(crate) struct CheckCommand {
compile_options: CompileOptions,
}

pub(crate) fn run(
_backend: &Backend,
args: CheckCommand,
config: NargoConfig,
) -> Result<(), CliError> {
pub(crate) fn run(args: CheckCommand, config: NargoConfig) -> Result<(), CliError> {
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
Expand Down
3 changes: 1 addition & 2 deletions tooling/nargo_cli/src/cli/codegen_verifier_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ pub(crate) fn run(
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

let expression_width = backend.get_backend_info()?;
let binary_packages = workspace.into_iter().filter(|package| package.is_binary());
for package in binary_packages {
let compilation_result = compile_program(
Expand All @@ -62,7 +61,7 @@ pub(crate) fn run(
args.compile_options.silence_warnings,
)?;

let program = nargo::ops::transform_program(program, expression_width);
let program = nargo::ops::transform_program(program, args.compile_options.expression_width);

// TODO(https://github.com/noir-lang/noir/issues/4428):
// We do not expect to have a smart contract verifier for a foldable program with multiple circuits.
Expand Down
17 changes: 3 additions & 14 deletions tooling/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use noirc_frontend::hir::ParsedFiles;
use notify::{EventKind, RecursiveMode, Watcher};
use notify_debouncer_full::new_debouncer;

use crate::backends::Backend;
use crate::errors::CliError;

use super::fs::program::only_acir;
Expand All @@ -47,11 +46,7 @@ pub(crate) struct CompileCommand {
watch: bool,
}

pub(crate) fn run(
backend: &Backend,
mut args: CompileCommand,
config: NargoConfig,
) -> Result<(), CliError> {
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 };
Expand All @@ -63,10 +58,6 @@ pub(crate) fn run(
Some(NOIR_ARTIFACT_VERSION_STRING.to_owned()),
)?;

if args.compile_options.expression_width.is_none() {
args.compile_options.expression_width = Some(backend.get_backend_info_or_default());
};

if args.watch {
watch_workspace(&workspace, &args.compile_options)
.map_err(|err| CliError::Generic(err.to_string()))?;
Expand Down Expand Up @@ -128,8 +119,6 @@ fn compile_workspace_full(
insert_all_files_for_workspace_into_file_manager(workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

let expression_width =
compile_options.expression_width.expect("expression width should have been set");
let compiled_workspace =
compile_workspace(&workspace_file_manager, &parsed_files, workspace, compile_options);

Expand All @@ -149,12 +138,12 @@ fn compile_workspace_full(
// Save build artifacts to disk.
let only_acir = compile_options.only_acir;
for (package, program) in binary_packages.into_iter().zip(compiled_programs) {
let program = nargo::ops::transform_program(program, expression_width);
let program = nargo::ops::transform_program(program, compile_options.expression_width);
save_program(program.clone(), &package, &workspace.target_directory_path(), only_acir);
}
let circuit_dir = workspace.target_directory_path();
for (package, contract) in contract_packages.into_iter().zip(compiled_contracts) {
let contract = nargo::ops::transform_contract(contract, expression_width);
let contract = nargo::ops::transform_contract(contract, compile_options.expression_width);
save_contract(contract, &package, &circuit_dir);
}

Expand Down
18 changes: 5 additions & 13 deletions tooling/nargo_cli/src/cli/dap_cmd.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use acvm::acir::circuit::ExpressionWidth;
use acvm::acir::native_types::WitnessMap;
use backend_interface::Backend;
use clap::Args;
use nargo::constants::PROVER_INPUT_FILE;
use nargo::workspace::Workspace;
Expand Down Expand Up @@ -29,8 +28,8 @@ use noir_debugger::errors::{DapError, LoadError};
#[derive(Debug, Clone, Args)]
pub(crate) struct DapCommand {
/// Override the expression width requested by the backend.
#[arg(long, value_parser = parse_expression_width)]
expression_width: Option<ExpressionWidth>,
#[arg(long, value_parser = parse_expression_width, default_value = "3")]
expression_width: ExpressionWidth,

#[clap(long)]
preflight_check: bool,
Expand Down Expand Up @@ -249,14 +248,7 @@ fn run_preflight_check(
Ok(())
}

pub(crate) fn run(
backend: &Backend,
args: DapCommand,
_config: NargoConfig,
) -> Result<(), CliError> {
let expression_width =
args.expression_width.unwrap_or_else(|| backend.get_backend_info_or_default());

pub(crate) fn run(args: DapCommand, _config: NargoConfig) -> Result<(), CliError> {
// When the --preflight-check flag is present, we run Noir's DAP server in "pre-flight mode", which test runs
// the DAP initialization code without actually starting the DAP server.
//
Expand All @@ -270,12 +262,12 @@ pub(crate) fn run(
// the DAP loop is established, which otherwise are considered "out of band" by the maintainers of the DAP spec.
// More details here: https://github.com/microsoft/vscode/issues/108138
if args.preflight_check {
return run_preflight_check(expression_width, args).map_err(CliError::DapError);
return run_preflight_check(args.expression_width, args).map_err(CliError::DapError);
}

let output = BufWriter::new(std::io::stdout());
let input = BufReader::new(std::io::stdin());
let server = Server::new(input, output);

loop_uninitialized_dap(server, expression_width).map_err(CliError::DapError)
loop_uninitialized_dap(server, args.expression_width).map_err(CliError::DapError)
}
14 changes: 3 additions & 11 deletions tooling/nargo_cli/src/cli/debug_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use noirc_frontend::hir::ParsedFiles;

use super::fs::{inputs::read_inputs_from_file, witness::save_witness_to_dir};
use super::NargoConfig;
use crate::backends::Backend;
use crate::errors::CliError;

/// Executes a circuit in debug mode
Expand Down Expand Up @@ -53,11 +52,7 @@ pub(crate) struct DebugCommand {
skip_instrumentation: Option<bool>,
}

pub(crate) fn run(
backend: &Backend,
args: DebugCommand,
config: NargoConfig,
) -> Result<(), CliError> {
pub(crate) fn run(args: DebugCommand, config: NargoConfig) -> Result<(), CliError> {
let acir_mode = args.acir_mode;
let skip_instrumentation = args.skip_instrumentation.unwrap_or(acir_mode);

Expand All @@ -69,10 +64,6 @@ pub(crate) fn run(
Some(NOIR_ARTIFACT_VERSION_STRING.to_string()),
)?;
let target_dir = &workspace.target_directory_path();
let expression_width = args
.compile_options
.expression_width
.unwrap_or_else(|| backend.get_backend_info_or_default());

let Some(package) = workspace.into_iter().find(|p| p.is_binary()) else {
println!(
Expand All @@ -89,7 +80,8 @@ pub(crate) fn run(
args.compile_options.clone(),
)?;

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

run_async(package, compiled_program, &args.prover_name, &args.witness_name, target_dir)
}
Expand Down
Loading

0 comments on commit e5f356b

Please sign in to comment.