From ccd20d55735efc7f832c756ab677ace2c6962ff7 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 18 Feb 2025 23:04:47 +0000 Subject: [PATCH 1/8] remove redundant slice access check from brillig --- .../src/brillig/brillig_gen/brillig_block.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 3be8ac47def..524fb50e06c 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -767,7 +767,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let index_variable = self.convert_ssa_single_addr_value(*index, dfg); - if !dfg.is_safe_index(*index, *array) { + // Slice access checks are generated separately against the slice's dynamic length field. + if !dfg.is_safe_index(*index, *array) && matches!(dfg.type_of_value(*array), Type::Array(..)) { self.validate_array_index(array_variable, index_variable); } @@ -795,7 +796,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { dfg, ); - if !dfg.is_safe_index(*index, *array) { + // Slice access checks are generated separately against the slice's dynamic length field. + if !dfg.is_safe_index(*index, *array) && matches!(dfg.type_of_value(*array), Type::Array(..)) { self.validate_array_index(source_variable, index_register); } From cf0ee3da9457e97bb96ff53112ff16a137407000 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 18 Feb 2025 23:15:43 +0000 Subject: [PATCH 2/8] cargo fmt --- .../src/brillig/brillig_gen/brillig_block.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 524fb50e06c..285c56af30b 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -768,7 +768,9 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let index_variable = self.convert_ssa_single_addr_value(*index, dfg); // Slice access checks are generated separately against the slice's dynamic length field. - if !dfg.is_safe_index(*index, *array) && matches!(dfg.type_of_value(*array), Type::Array(..)) { + if !dfg.is_safe_index(*index, *array) + && matches!(dfg.type_of_value(*array), Type::Array(..)) + { self.validate_array_index(array_variable, index_variable); } @@ -797,7 +799,9 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { ); // Slice access checks are generated separately against the slice's dynamic length field. - if !dfg.is_safe_index(*index, *array) && matches!(dfg.type_of_value(*array), Type::Array(..)) { + if !dfg.is_safe_index(*index, *array) + && matches!(dfg.type_of_value(*array), Type::Array(..)) + { self.validate_array_index(source_variable, index_register); } From 45a584025661662afdc1bde7fe420add4e26ed7d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 19 Feb 2025 00:44:53 +0000 Subject: [PATCH 3/8] some docs on profiling commands and initial error reporting --- Cargo.lock | 1 + .../execution_success/1_mul/src/main.nr | 2 +- tooling/profiler/Cargo.toml | 1 + .../src/cli/execution_flamegraph_cmd.rs | 53 +++++++++++++++++-- .../profiler/src/cli/gates_flamegraph_cmd.rs | 4 +- tooling/profiler/src/cli/mod.rs | 4 +- .../src/cli/opcodes_flamegraph_cmd.rs | 1 + tooling/profiler/src/errors.rs | 15 ++++++ tooling/profiler/src/main.rs | 3 +- 9 files changed, 77 insertions(+), 7 deletions(-) create mode 100644 tooling/profiler/src/errors.rs diff --git a/Cargo.lock b/Cargo.lock index a522ccb3bed..2f4c54f39a7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3254,6 +3254,7 @@ dependencies = [ "serde", "serde_json", "tempfile", + "thiserror", "tracing-appender", "tracing-subscriber", ] diff --git a/test_programs/execution_success/1_mul/src/main.nr b/test_programs/execution_success/1_mul/src/main.nr index 8f4032dbd75..36590aec359 100644 --- a/test_programs/execution_success/1_mul/src/main.nr +++ b/test_programs/execution_success/1_mul/src/main.nr @@ -5,5 +5,5 @@ fn main(mut x: u32, y: u32, z: u32) { x *= x; //144 x *= x; //20736 x *= x; //429 981 696 - assert(x == z); + assert(x == z + 10); } diff --git a/tooling/profiler/Cargo.toml b/tooling/profiler/Cargo.toml index 798a21ea0d6..a51adf41988 100644 --- a/tooling/profiler/Cargo.toml +++ b/tooling/profiler/Cargo.toml @@ -34,6 +34,7 @@ nargo.workspace = true noirc_errors.workspace = true noirc_abi.workspace = true noirc_evaluator.workspace = true +thiserror.workspace = true # Logs tracing-subscriber.workspace = true diff --git a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs index dbb2a2c38bc..ddb33d0c186 100644 --- a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs @@ -1,11 +1,15 @@ use std::path::{Path, PathBuf}; +use acir::circuit::Opcode; use acir::circuit::OpcodeLocation; use clap::Args; use color_eyre::eyre::{self, Context}; +use nargo::errors::try_to_diagnose_runtime_error; use nargo::foreign_calls::DefaultForeignCallBuilder; use nargo::PrintOutput; +use noirc_artifacts::program::ProgramArtifact; +use crate::errors::{report_error, CliError}; use crate::flamegraph::{BrilligExecutionSample, FlamegraphGenerator, InfernoFlamegraphGenerator}; use crate::fs::{read_inputs_from_file, read_program_from_file}; use crate::opcode_formatter::format_brillig_opcode; @@ -13,6 +17,7 @@ use bn254_blackbox_solver::Bn254BlackBoxSolver; use noirc_abi::input_parser::Format; use noirc_artifacts::debug::DebugArtifact; +/// Generates a flamegraph mapping unconstrained Noir execution to source code. #[derive(Debug, Clone, Args)] pub(crate) struct ExecutionFlamegraphCommand { /// The path to the artifact JSON file @@ -54,17 +59,40 @@ fn run_with_generator( let program = read_program_from_file(artifact_path).context("Error reading program from file")?; + is_brillig_entry_point(&program)?; + let (inputs_map, _) = read_inputs_from_file(prover_toml_path, Format::Toml, &program.abi)?; let initial_witness = program.abi.encode(&inputs_map, None)?; - println!("Executing"); - let (_, mut profiling_samples) = nargo::ops::execute_program_with_profiling( + println!("Executing..."); + + let solved_witness_stack_err = nargo::ops::execute_program_with_profiling( &program.bytecode, initial_witness, &Bn254BlackBoxSolver(pedantic_solving), &mut DefaultForeignCallBuilder::default().with_output(PrintOutput::Stdout).build(), - )?; + ); + let mut profiling_samples = match solved_witness_stack_err { + Ok((_, profiling_samples)) => profiling_samples, + Err(err) => { + let debug_artifact = DebugArtifact { + debug_symbols: program.debug_symbols.debug_infos.clone(), + file_map: program.file_map.clone(), + }; + + if let Some(diagnostic) = try_to_diagnose_runtime_error( + &err, + &program.abi, + &program.debug_symbols.debug_infos, + ) { + diagnostic.report(&debug_artifact, false); + } + + return Err(CliError::Generic.into()); + } + }; + println!("Executed"); println!("Collecting {} samples", profiling_samples.len()); @@ -104,3 +132,22 @@ fn run_with_generator( Ok(()) } + +fn is_brillig_entry_point(artifact: &ProgramArtifact) -> Result<(), CliError> { + let err_msg = "Command only supports fully unconstrained Noir programs e.g. `unconstrained fn main() { .. }".to_owned(); + let program = &artifact.bytecode; + if program.functions.len() != 1 || program.unconstrained_functions.len() != 1 { + return report_error(err_msg); + } + + let main_function = &program.functions[0]; + let Opcode::BrilligCall { id, .. } = main_function.opcodes[0] else { + return report_error(err_msg); + }; + + if id.as_usize() != 0 { + return report_error(err_msg); + } + + Ok(()) +} diff --git a/tooling/profiler/src/cli/gates_flamegraph_cmd.rs b/tooling/profiler/src/cli/gates_flamegraph_cmd.rs index e68a8cd5bd2..23ea15b402a 100644 --- a/tooling/profiler/src/cli/gates_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/gates_flamegraph_cmd.rs @@ -11,13 +11,14 @@ use crate::fs::read_program_from_file; use crate::gates_provider::{BackendGatesProvider, GatesProvider}; use crate::opcode_formatter::format_acir_opcode; +/// Generates a flamegraph mapping backend opcodes to their associated locations in the source code. #[derive(Debug, Clone, Args)] pub(crate) struct GatesFlamegraphCommand { /// The path to the artifact JSON file #[clap(long, short)] artifact_path: String, - /// Path to the noir backend binary + /// Path to the Noir backend binary #[clap(long, short)] backend_path: String, @@ -25,6 +26,7 @@ pub(crate) struct GatesFlamegraphCommand { #[clap(long, short = 'g', default_value = "gates")] backend_gates_command: String, + /// Optional arguments for the backend gates command #[arg(trailing_var_arg = true, allow_hyphen_values = true)] backend_extra_args: Vec, diff --git a/tooling/profiler/src/cli/mod.rs b/tooling/profiler/src/cli/mod.rs index 80c6bceb3ce..b91dd6990aa 100644 --- a/tooling/profiler/src/cli/mod.rs +++ b/tooling/profiler/src/cli/mod.rs @@ -32,5 +32,7 @@ pub(crate) fn start_cli() -> eyre::Result<()> { ProfilerCommand::Gates(args) => gates_flamegraph_cmd::run(args), ProfilerCommand::Opcodes(args) => opcodes_flamegraph_cmd::run(args), ProfilerCommand::ExecutionOpcodes(args) => execution_flamegraph_cmd::run(args), - } + }?; + + Ok(()) } diff --git a/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs b/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs index 4e271c9f838..90d5da08c1c 100644 --- a/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/opcodes_flamegraph_cmd.rs @@ -11,6 +11,7 @@ use crate::flamegraph::{CompilationSample, FlamegraphGenerator, InfernoFlamegrap use crate::fs::read_program_from_file; use crate::opcode_formatter::{format_acir_opcode, format_brillig_opcode}; +/// Generates a flamegraph mapping ACIR opcodes to their associated locations in the source code. #[derive(Debug, Clone, Args)] pub(crate) struct OpcodesFlamegraphCommand { /// The path to the artifact JSON file diff --git a/tooling/profiler/src/errors.rs b/tooling/profiler/src/errors.rs new file mode 100644 index 00000000000..a7140a59807 --- /dev/null +++ b/tooling/profiler/src/errors.rs @@ -0,0 +1,15 @@ +use fm::FileMap; +use noirc_errors::{CustomDiagnostic, Span}; +use thiserror::Error; + +#[derive(Debug, Error)] +pub(crate) enum CliError { + #[error("Failed to run profiler command")] + Generic, +} + +pub(crate) fn report_error(message: String) -> Result<(), CliError> { + let error = CustomDiagnostic::simple_error(message.clone(), String::new(), Span::default()); + noirc_errors::reporter::report(&FileMap::default(), &error, None, false); + Err(CliError::Generic) +} diff --git a/tooling/profiler/src/main.rs b/tooling/profiler/src/main.rs index b0b42e6ee41..a49c24a40b2 100644 --- a/tooling/profiler/src/main.rs +++ b/tooling/profiler/src/main.rs @@ -4,6 +4,7 @@ #![cfg_attr(not(test), warn(unused_crate_dependencies, unused_extern_crates))] mod cli; +mod errors; mod flamegraph; mod fs; mod gates_provider; @@ -33,7 +34,7 @@ fn main() { } if let Err(report) = cli::start_cli() { - eprintln!("{report:?}"); + eprintln!("{report}"); std::process::exit(1); } } From 1c2b4f21ea4be251d162e3eb4ae4f523da09a068 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 19 Feb 2025 00:52:36 +0000 Subject: [PATCH 4/8] update package description --- tooling/profiler/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/profiler/Cargo.toml b/tooling/profiler/Cargo.toml index a51adf41988..cdd014f6d65 100644 --- a/tooling/profiler/Cargo.toml +++ b/tooling/profiler/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "noir_profiler" -description = "Profiler for noir circuits" +description = "Profiler for Noir circuits" version.workspace = true authors.workspace = true edition.workspace = true From 1302f67c0a8949f0d6fd4eb024268219aa4e0b91 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 19 Feb 2025 11:26:34 -0500 Subject: [PATCH 5/8] Update tooling/profiler/src/cli/execution_flamegraph_cmd.rs Co-authored-by: Michael J Klein --- tooling/profiler/src/cli/execution_flamegraph_cmd.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs index ddb33d0c186..aca78893d4f 100644 --- a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs @@ -133,7 +133,7 @@ fn run_with_generator( Ok(()) } -fn is_brillig_entry_point(artifact: &ProgramArtifact) -> Result<(), CliError> { +fn ensure_brillig_entry_point(artifact: &ProgramArtifact) -> Result<(), CliError> { let err_msg = "Command only supports fully unconstrained Noir programs e.g. `unconstrained fn main() { .. }".to_owned(); let program = &artifact.bytecode; if program.functions.len() != 1 || program.unconstrained_functions.len() != 1 { From af9da2d4d4e5d77d4898550d0c0c6179cf255771 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 19 Feb 2025 11:26:51 -0500 Subject: [PATCH 6/8] Update tooling/profiler/src/cli/execution_flamegraph_cmd.rs --- tooling/profiler/src/cli/execution_flamegraph_cmd.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs index aca78893d4f..43a5ca4680f 100644 --- a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs @@ -59,7 +59,7 @@ fn run_with_generator( let program = read_program_from_file(artifact_path).context("Error reading program from file")?; - is_brillig_entry_point(&program)?; + ensure_brillig_entry_point(&program)?; let (inputs_map, _) = read_inputs_from_file(prover_toml_path, Format::Toml, &program.abi)?; From ed6b7687184c3c2b16827a76af794e0c584d58f0 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 19 Feb 2025 11:30:42 -0500 Subject: [PATCH 7/8] Update tooling/profiler/src/errors.rs --- tooling/profiler/src/errors.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tooling/profiler/src/errors.rs b/tooling/profiler/src/errors.rs index a7140a59807..623f888c2b0 100644 --- a/tooling/profiler/src/errors.rs +++ b/tooling/profiler/src/errors.rs @@ -8,6 +8,7 @@ pub(crate) enum CliError { Generic, } +/// Report an error from the CLI that is not reliant on a stack trace. pub(crate) fn report_error(message: String) -> Result<(), CliError> { let error = CustomDiagnostic::simple_error(message.clone(), String::new(), Span::default()); noirc_errors::reporter::report(&FileMap::default(), &error, None, false); From e2971e7cd095d38485993e0b7bc27fd07e221f9d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 19 Feb 2025 16:36:49 +0000 Subject: [PATCH 8/8] cargo fmt --- tooling/profiler/src/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/profiler/src/errors.rs b/tooling/profiler/src/errors.rs index 623f888c2b0..c49b0b335dd 100644 --- a/tooling/profiler/src/errors.rs +++ b/tooling/profiler/src/errors.rs @@ -8,7 +8,7 @@ pub(crate) enum CliError { Generic, } -/// Report an error from the CLI that is not reliant on a stack trace. +/// Report an error from the CLI that is not reliant on a stack trace. pub(crate) fn report_error(message: String) -> Result<(), CliError> { let error = CustomDiagnostic::simple_error(message.clone(), String::new(), Span::default()); noirc_errors::reporter::report(&FileMap::default(), &error, None, false);