From bc4b3235c0a7b4b11188b00d268708f5b8727afa Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 29 Oct 2024 04:18:33 +0000 Subject: [PATCH 1/8] add procedure information debug info --- acvm-repo/acir/src/circuit/brillig.rs | 64 +++++++++++++++++++ compiler/noirc_driver/src/lib.rs | 5 ++ compiler/noirc_errors/Cargo.toml | 1 + compiler/noirc_errors/src/debug_info.rs | 7 +- .../brillig/brillig_gen/brillig_directive.rs | 6 +- .../src/brillig/brillig_ir/artifact.rs | 17 +++-- .../src/brillig/brillig_ir/instructions.rs | 2 +- .../src/brillig/brillig_ir/procedures/mod.rs | 31 ++++----- compiler/noirc_evaluator/src/brillig/mod.rs | 6 +- compiler/noirc_evaluator/src/ssa.rs | 21 +++++- .../ssa/acir_gen/acir_ir/generated_acir.rs | 22 ++++++- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 34 +++++++--- tooling/debugger/src/source_code_printer.rs | 1 + tooling/nargo_cli/src/cli/compile_cmd.rs | 1 - tooling/noirc_artifacts/src/debug.rs | 1 + tooling/profiler/src/flamegraph.rs | 21 +++++- tooling/profiler/src/opcode_formatter.rs | 4 +- 17 files changed, 202 insertions(+), 42 deletions(-) diff --git a/acvm-repo/acir/src/circuit/brillig.rs b/acvm-repo/acir/src/circuit/brillig.rs index a9714ce29b2..dc7e78fd0a1 100644 --- a/acvm-repo/acir/src/circuit/brillig.rs +++ b/acvm-repo/acir/src/circuit/brillig.rs @@ -1,7 +1,10 @@ +use std::str::FromStr; + use super::opcodes::BlockId; use crate::native_types::{Expression, Witness}; use brillig::Opcode as BrilligOpcode; use serde::{Deserialize, Serialize}; +use thiserror::Error; /// Inputs for the Brillig VM. These are the initial inputs /// that the Brillig VM will use to start. @@ -46,3 +49,64 @@ impl std::fmt::Display for BrilligFunctionId { write!(f, "{}", self.0) } } + +/// Procedures are a set of complex operations that are common in the noir language. +/// Extracting them to reusable procedures allows us to reduce the size of the generated Brillig. +/// Procedures receive their arguments on scratch space to avoid stack dumping&restoring. +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] +pub enum ProcedureId { + ArrayCopy, + ArrayReverse, + VectorCopy, + MemCopy, + PrepareVectorPush(bool), + VectorPop(bool), + PrepareVectorInsert, + VectorRemove, + CheckMaxStackDepth, +} + +impl std::fmt::Display for ProcedureId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ProcedureId::ArrayCopy => write!(f, "ArrayCopy"), + ProcedureId::ArrayReverse => write!(f, "ArrayReverse"), + ProcedureId::VectorCopy => write!(f, "VectorCopy"), + ProcedureId::MemCopy => write!(f, "MemCopy"), + ProcedureId::PrepareVectorPush(flag) => write!(f, "PrepareVectorPush({flag})"), + ProcedureId::VectorPop(flag) => write!(f, "VectorPop({flag})"), + ProcedureId::PrepareVectorInsert => write!(f, "PrepareVectorInsert"), + ProcedureId::VectorRemove => write!(f, "VectorRemove"), + ProcedureId::CheckMaxStackDepth => write!(f, "CheckMaxStackDepth"), + } + } +} +#[derive(Error, Debug)] +pub enum ProcedureIdFromStrError { + #[error("Invalid procedure id string: {0}")] + InvalidProcedureIdString(String), +} + +/// The implementation of display and FromStr allows serializing and deserializing a ProcedureId to a string. +/// This is useful when used as key in a map that has to be serialized to JSON/TOML, for example when mapping an opcode to its metadata. +impl FromStr for ProcedureId { + type Err = ProcedureIdFromStrError; + + fn from_str(s: &str) -> Result { + let res = match s { + "ArrayCopy" => ProcedureId::ArrayCopy, + "ArrayReverse" => ProcedureId::ArrayReverse, + "VectorCopy" => ProcedureId::VectorCopy, + "MemCopy" => ProcedureId::MemCopy, + "PrepareVectorPush(true)" => ProcedureId::PrepareVectorPush(true), + "PrepareVectorPush(false)" => ProcedureId::PrepareVectorPush(false), + "VectorPop(true)" => ProcedureId::VectorPop(true), + "VectorPop(false)" => ProcedureId::VectorPop(false), + "PrepareVectorInsert" => ProcedureId::PrepareVectorInsert, + "VectorRemove" => ProcedureId::VectorRemove, + "CheckMaxStackDepth" => ProcedureId::CheckMaxStackDepth, + _ => return Err(ProcedureIdFromStrError::InvalidProcedureIdString(s.to_string())), + }; + Ok(res) + } +} diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index c4e2491a5fe..45b10910678 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -131,6 +131,10 @@ pub struct CompileOptions { /// A less aggressive inliner should generate smaller programs #[arg(long, hide = true, allow_hyphen_values = true, default_value_t = i64::MAX)] pub inliner_aggressiveness: i64, + + /// Gather extra debug information + #[arg(long, hide = true)] + pub profile: bool, } pub fn parse_expression_width(input: &str) -> Result { @@ -590,6 +594,7 @@ pub fn compile_no_check( emit_ssa: if options.emit_ssa { Some(context.package_build_path.clone()) } else { None }, skip_underconstrained_check: options.skip_underconstrained_check, inliner_aggressiveness: options.inliner_aggressiveness, + profiling_active: options.profile, }; let SsaProgramArtifact { program, debug, warnings, names, brillig_names, error_types, .. } = diff --git a/compiler/noirc_errors/Cargo.toml b/compiler/noirc_errors/Cargo.toml index a6927eb647a..ddc30a12bb9 100644 --- a/compiler/noirc_errors/Cargo.toml +++ b/compiler/noirc_errors/Cargo.toml @@ -17,6 +17,7 @@ codespan-reporting.workspace = true codespan.workspace = true fm.workspace = true noirc_printable_type.workspace = true +# noirc_evaluator.workspace = true serde.workspace = true serde_with = "3.2.0" tracing.workspace = true diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index b480d20fde4..9d4d7e70702 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -1,4 +1,5 @@ use acvm::acir::circuit::brillig::BrilligFunctionId; +use acvm::acir::circuit::brillig::ProcedureId; use acvm::acir::circuit::BrilligOpcodeLocation; use acvm::acir::circuit::OpcodeLocation; use acvm::compiler::AcirTransformationMap; @@ -104,6 +105,9 @@ pub struct DebugInfo { pub variables: DebugVariables, pub functions: DebugFunctions, pub types: DebugTypes, + /// This a map per brillig function representing the range of opcodes where a procedure is activated. + #[serde_as(as = "BTreeMap<_, BTreeMap>")] + pub brillig_procedure_locs: BTreeMap>, } /// Holds OpCodes Counts for Acir and Brillig Opcodes @@ -124,8 +128,9 @@ impl DebugInfo { variables: DebugVariables, functions: DebugFunctions, types: DebugTypes, + brillig_procedure_locs: BTreeMap>, ) -> Self { - Self { locations, brillig_locations, variables, functions, types } + Self { locations, brillig_locations, variables, functions, types, brillig_procedure_locs } } /// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified. diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs index f066d967e0d..49e259e0ce5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs @@ -67,9 +67,8 @@ pub(crate) fn directive_invert() -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 1 }, ], - assert_messages: Default::default(), - locations: Default::default(), name: "directive_invert".to_string(), + ..Default::default() } } @@ -132,8 +131,7 @@ pub(crate) fn directive_quotient() -> GeneratedBrillig { }, BrilligOpcode::Stop { return_data_offset: 0, return_data_size: 2 }, ], - assert_messages: Default::default(), - locations: Default::default(), name: "directive_integer_quotient".to_string(), + ..Default::default() } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index d0bd91e185c..9e594e68f3e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -1,7 +1,6 @@ -use acvm::acir::brillig::Opcode as BrilligOpcode; +use acvm::acir::{brillig::Opcode as BrilligOpcode, circuit::brillig::ProcedureId}; use std::collections::{BTreeMap, HashMap}; -use crate::brillig::brillig_ir::procedures::ProcedureId; use crate::ssa::ir::{basic_block::BasicBlockId, dfg::CallStack, function::FunctionId}; /// Represents a parameter or a return value of an entry point function. @@ -24,6 +23,7 @@ pub(crate) struct GeneratedBrillig { pub(crate) locations: BTreeMap, pub(crate) assert_messages: BTreeMap, pub(crate) name: String, + pub(crate) procedure_locations: HashMap, } #[derive(Default, Debug, Clone)] @@ -48,11 +48,19 @@ pub(crate) struct BrilligArtifact { /// TODO: and have an enum which indicates whether the jump is internal or external unresolved_external_call_labels: Vec<(JumpInstructionPosition, Label)>, /// Maps the opcodes that are associated with a callstack to it. - locations: BTreeMap, + pub(crate) locations: BTreeMap, /// The current call stack. All opcodes that are pushed will be associated with this call stack. - call_stack: CallStack, + pub(crate) call_stack: CallStack, /// Name of the function, only used for debugging purposes. pub(crate) name: String, + + /// This field contains the given procedure id if this artifact originates from as procedure + pub(crate) procedure: Option, + /// Procedure ID mapped to the range of its opcode locations + /// This is created as artifacts are linked together and allows us to determine + /// which opcodes originate from reusable procedures.s + /// The range is inclusive for both start and end opcode locations. + pub(crate) procedure_locations: HashMap, } /// A pointer to a location in the opcode. @@ -149,6 +157,7 @@ impl BrilligArtifact { locations: self.locations, assert_messages: self.assert_messages, name: self.name, + procedure_locations: self.procedure_locations, } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs index 1ac672687f3..f77c1b7377f 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs @@ -4,6 +4,7 @@ use acvm::{ BinaryFieldOp, BinaryIntOp, BitSize, BlackBoxOp, HeapArray, HeapValueType, MemoryAddress, Opcode as BrilligOpcode, ValueOrArray, }, + circuit::brillig::ProcedureId, AcirField, }, FieldElement, @@ -15,7 +16,6 @@ use super::{ artifact::{Label, UnresolvedJumpLocation}, brillig_variable::SingleAddrVariable, debug_show::DebugToString, - procedures::ProcedureId, registers::RegisterAllocator, BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index 0ee6fe49435..820d3278c19 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -8,6 +8,7 @@ mod vector_copy; mod vector_pop; mod vector_remove; +use acvm::acir::circuit::brillig::ProcedureId; use array_copy::compile_array_copy_procedure; use array_reverse::compile_array_reverse_procedure; use check_max_stack_depth::compile_check_max_stack_depth_procedure; @@ -26,21 +27,21 @@ use super::{ BrilligContext, }; -/// Procedures are a set of complex operations that are common in the noir language. -/// Extracting them to reusable procedures allows us to reduce the size of the generated Brillig. -/// Procedures receive their arguments on scratch space to avoid stack dumping&restoring. -#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] -pub(crate) enum ProcedureId { - ArrayCopy, - ArrayReverse, - VectorCopy, - MemCopy, - PrepareVectorPush(bool), - VectorPop(bool), - PrepareVectorInsert, - VectorRemove, - CheckMaxStackDepth, -} +// /// Procedures are a set of complex operations that are common in the noir language. +// /// Extracting them to reusable procedures allows us to reduce the size of the generated Brillig. +// /// Procedures receive their arguments on scratch space to avoid stack dumping&restoring. +// #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] +// pub enum ProcedureId { +// ArrayCopy, +// ArrayReverse, +// VectorCopy, +// MemCopy, +// PrepareVectorPush(bool), +// VectorPop(bool), +// PrepareVectorInsert, +// VectorRemove, +// CheckMaxStackDepth, +// } pub(crate) fn compile_procedure( procedure_id: ProcedureId, diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index f1da76669cd..21b439ac9ff 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -43,7 +43,11 @@ impl Brillig { self.ssa_function_to_brillig.get(&function_id).map(Cow::Borrowed) } // Procedures are compiled as needed - LabelType::Procedure(procedure_id) => Some(Cow::Owned(compile_procedure(procedure_id))), + LabelType::Procedure(procedure_id) => { + let mut artifact = compile_procedure(procedure_id); + artifact.procedure = Some(procedure_id); + Some(Cow::Owned(artifact)) + } _ => unreachable!("ICE: Expected a function or procedure label"), } } diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index ea41b0cfb32..29095ae5faa 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -70,6 +70,9 @@ pub struct SsaEvaluatorOptions { /// The higher the value, the more inlined brillig functions will be. pub inliner_aggressiveness: i64, + + /// Collect extra debug information for more informative profiling. + pub profiling_active: bool, } pub(crate) struct ArtifactsAndWarnings(Artifacts, Vec); @@ -144,7 +147,7 @@ pub(crate) fn optimize_into_acir( }); let artifacts = time("SSA to ACIR", options.print_codegen_timings, || { - ssa.into_acir(&brillig, options.expression_width) + ssa.into_acir(&brillig, options.expression_width, options.profiling_active) })?; Ok(ArtifactsAndWarnings(artifacts, ssa_level_warnings)) } @@ -291,6 +294,7 @@ fn convert_generated_acir_into_circuit( assertion_payloads: assert_messages, warnings, name, + brillig_procedure_locs, .. } = generated_acir; @@ -328,8 +332,19 @@ fn convert_generated_acir_into_circuit( }) .collect(); - let mut debug_info = - DebugInfo::new(locations, brillig_locations, debug_variables, debug_functions, debug_types); + let brillig_procedure_locs = if brillig_procedure_locs.is_empty() { + BTreeMap::default() + } else { + brillig_procedure_locs + }; + let mut debug_info = DebugInfo::new( + locations, + brillig_locations, + debug_variables, + debug_functions, + debug_types, + brillig_procedure_locs, + ); // Perform any ACIR-level optimizations let (optimized_circuit, transformation_map) = acvm::compiler::optimize(circuit); diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 01fcaef9042..b01fe382ec1 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -9,7 +9,7 @@ use crate::{ }; use acvm::acir::{ circuit::{ - brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs}, + brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs, ProcedureId}, opcodes::{BlackBoxFuncCall, FunctionInput, Opcode as AcirOpcode}, AssertionPayload, BrilligOpcodeLocation, OpcodeLocation, }, @@ -20,6 +20,7 @@ use acvm::{ acir::AcirField, acir::{circuit::directives::Directive, native_types::Expression}, }; + use iter_extended::vecmap; use num_bigint::BigUint; @@ -72,6 +73,15 @@ pub(crate) struct GeneratedAcir { /// As to avoid passing the ACIR gen shared context into each individual ACIR /// we can instead keep this map and resolve the Brillig calls at the end of code generation. pub(crate) brillig_stdlib_func_locations: BTreeMap, + + /// Flag that determines whether we should gather extra information for profiling + /// such as opcode range of Brillig procedures. + pub(crate) profiling_active: bool, + + /// Brillig function id -> Brillig procedure locations map + /// This maps allows a profiler to determine which Brillig opcodes + /// originated from a reusable procedure. + pub(crate) brillig_procedure_locs: BTreeMap, } /// Correspondence between an opcode index (in opcodes) and the source code call stack which generated it @@ -79,6 +89,8 @@ pub(crate) type OpcodeToLocationsMap = BTreeMap; pub(crate) type BrilligOpcodeToLocationsMap = BTreeMap; +pub(crate) type BrilligProcedureRangeMap = BTreeMap; + #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] pub(crate) enum BrilligStdlibFunc { Inverse, @@ -591,6 +603,14 @@ impl GeneratedAcir { return; } + for (procedure_id, (start_index, end_index)) in generated_brillig.procedure_locations.iter() + { + self.brillig_procedure_locs + .entry(brillig_function_index) + .or_default() + .insert(*procedure_id, (*start_index, *end_index)); + } + for (brillig_index, call_stack) in generated_brillig.locations.iter() { self.brillig_locations .entry(brillig_function_index) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a5c51392114..be6f90a0e9f 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -195,6 +195,9 @@ struct Context<'a> { /// Contains state that is generated and also used across ACIR functions shared_context: &'a mut SharedContext, + + /// Flag to determine whether compilation should collect extra debug information for profiling. + profiling_active: bool, } #[derive(Clone)] @@ -292,12 +295,13 @@ impl Ssa { self, brillig: &Brillig, expression_width: ExpressionWidth, + profiling_active: bool, ) -> Result { let mut acirs = Vec::new(); // TODO: can we parallelize this? let mut shared_context = SharedContext::default(); for function in self.functions.values() { - let context = Context::new(&mut shared_context, expression_width); + let context = Context::new(&mut shared_context, expression_width, profiling_active); if let Some(mut generated_acir) = context.convert_ssa_function(&self, function, brillig)? { @@ -349,6 +353,7 @@ impl<'a> Context<'a> { fn new( shared_context: &'a mut SharedContext, expression_width: ExpressionWidth, + profiling_active: bool, ) -> Context<'a> { let mut acir_context = AcirContext::default(); acir_context.set_expression_width(expression_width); @@ -365,6 +370,7 @@ impl<'a> Context<'a> { max_block_id: 0, data_bus: DataBus::default(), shared_context, + profiling_active, } } @@ -1003,6 +1009,18 @@ impl<'a> Context<'a> { } }; entry_point.link_with(artifact); + // Insert the range of opcode locations occupied by a procedure + if self.profiling_active { + if let Some(procedure_id) = artifact.procedure { + let num_opcodes = entry_point.byte_code.len(); + let previous_num_opcodes = + entry_point.byte_code.len() - artifact.byte_code.len(); + // We subtract one as to keep the range inclusive on both ends + entry_point + .procedure_locations + .insert(procedure_id, (previous_num_opcodes, num_opcodes - 1)); + } + } } // Generate the final bytecode Ok(entry_point.finish()) @@ -3020,7 +3038,7 @@ mod test { let ssa = builder.finish(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), ExpressionWidth::default(), false) .expect("Should compile manually written SSA into ACIR"); // Expected result: // main f0 @@ -3125,7 +3143,7 @@ mod test { let ssa = builder.finish(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), ExpressionWidth::default(), false) .expect("Should compile manually written SSA into ACIR"); // The expected result should look very similar to the above test expect that the input witnesses of the `Call` // opcodes will be different. The changes can discerned from the checks below. @@ -3225,7 +3243,7 @@ mod test { let ssa = builder.finish(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), ExpressionWidth::default(), false) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 3, "Should have three ACIR functions"); @@ -3349,7 +3367,7 @@ mod test { let brillig = ssa.to_brillig(false); let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, ExpressionWidth::default(), false) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3413,7 +3431,7 @@ mod test { // The Brillig bytecode we insert for the stdlib is hardcoded so we do not need to provide any // Brillig artifacts to the ACIR gen pass. let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default()) + .into_acir(&Brillig::default(), ExpressionWidth::default(), false) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3487,7 +3505,7 @@ mod test { println!("{}", ssa); let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, ExpressionWidth::default(), false) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3575,7 +3593,7 @@ mod test { println!("{}", ssa); let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default()) + .into_acir(&brillig, ExpressionWidth::default(), false) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions"); diff --git a/tooling/debugger/src/source_code_printer.rs b/tooling/debugger/src/source_code_printer.rs index d1c82ad96ba..b3682c9016f 100644 --- a/tooling/debugger/src/source_code_printer.rs +++ b/tooling/debugger/src/source_code_printer.rs @@ -275,6 +275,7 @@ mod tests { BTreeMap::default(), BTreeMap::default(), BTreeMap::default(), + BTreeMap::default(), )]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 0ad07c91ff4..18fb407d413 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -192,7 +192,6 @@ fn compile_programs( 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.into(), &package.name, workspace.target_directory_path()); Ok(((), warnings)) diff --git a/tooling/noirc_artifacts/src/debug.rs b/tooling/noirc_artifacts/src/debug.rs index 8e2add70ae7..5c47f1f2652 100644 --- a/tooling/noirc_artifacts/src/debug.rs +++ b/tooling/noirc_artifacts/src/debug.rs @@ -264,6 +264,7 @@ mod tests { BTreeMap::default(), BTreeMap::default(), BTreeMap::default(), + BTreeMap::default(), )]; let debug_artifact = DebugArtifact::new(debug_symbols, &fm); diff --git a/tooling/profiler/src/flamegraph.rs b/tooling/profiler/src/flamegraph.rs index a72168a20af..39b7aa6d814 100644 --- a/tooling/profiler/src/flamegraph.rs +++ b/tooling/profiler/src/flamegraph.rs @@ -24,7 +24,7 @@ pub(crate) struct Sample { pub(crate) brillig_function_id: Option, } -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub(crate) struct FoldedStackItem { pub(crate) total_samples: usize, pub(crate) nested_items: BTreeMap, @@ -111,6 +111,7 @@ fn generate_folded_sorted_lines<'files, F: AcirField>( if let Some(opcode) = &sample.opcode { location_names.push(format_opcode(opcode)); } + add_locations_to_folded_stack_items(&mut folded_stack_items, location_names, sample.count); } @@ -123,10 +124,19 @@ fn find_callsite_labels<'files>( brillig_function_id: Option, files: &'files impl Files<'files, FileId = fm::FileId>, ) -> Vec { + let mut procedure_id = None; let source_locations = debug_symbols.opcode_location(opcode_location).unwrap_or_else(|| { if let (Some(brillig_function_id), Some(brillig_location)) = (brillig_function_id, opcode_location.to_brillig_location()) { + let procedure_locs = debug_symbols.brillig_procedure_locs.get(&brillig_function_id); + if let Some(procedure_locs) = procedure_locs { + for (procedure, range) in procedure_locs.iter() { + if brillig_location.0 >= range.0 && brillig_location.0 <= range.1 { + procedure_id = Some(*procedure); + } + } + } let brillig_locations = debug_symbols.brillig_locations.get(&brillig_function_id); if let Some(brillig_locations) = brillig_locations { brillig_locations.get(&brillig_location).cloned().unwrap_or_default() @@ -137,10 +147,16 @@ fn find_callsite_labels<'files>( vec![] } }); - let callsite_labels: Vec<_> = source_locations + + let mut callsite_labels: Vec<_> = source_locations .into_iter() .map(|location| location_to_callsite_label(location, files)) .collect(); + + if let Some(procedure_id) = procedure_id { + callsite_labels.push(format!("procedure::{}", procedure_id)); + } + callsite_labels } @@ -317,6 +333,7 @@ mod tests { BTreeMap::default(), BTreeMap::default(), BTreeMap::default(), + BTreeMap::default(), ); let samples: Vec> = vec![ diff --git a/tooling/profiler/src/opcode_formatter.rs b/tooling/profiler/src/opcode_formatter.rs index 68057b6d86f..b044066bdca 100644 --- a/tooling/profiler/src/opcode_formatter.rs +++ b/tooling/profiler/src/opcode_formatter.rs @@ -138,7 +138,9 @@ fn format_brillig_opcode_kind(opcode: &BrilligOpcode) -> String { pub(crate) fn format_opcode(opcode: &AcirOrBrilligOpcode) -> String { match opcode { - AcirOrBrilligOpcode::Acir(opcode) => format!("acir::{}", format_acir_opcode_kind(opcode)), + AcirOrBrilligOpcode::Acir(opcode) => { + format!("acir::{}", format_acir_opcode_kind(opcode)) + } AcirOrBrilligOpcode::Brillig(opcode) => { format!("brillig::{}", format_brillig_opcode_kind(opcode)) } From 698a4f2425dbebe18e70b6917fd5c2dec45bb774 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 29 Oct 2024 04:28:28 +0000 Subject: [PATCH 2/8] general cleanup --- compiler/noirc_errors/Cargo.toml | 1 - .../src/brillig/brillig_ir/artifact.rs | 4 ++-- .../src/brillig/brillig_ir/procedures/mod.rs | 16 ---------------- compiler/noirc_evaluator/src/ssa.rs | 5 ----- tooling/profiler/src/opcode_formatter.rs | 4 +--- 5 files changed, 3 insertions(+), 27 deletions(-) diff --git a/compiler/noirc_errors/Cargo.toml b/compiler/noirc_errors/Cargo.toml index ddc30a12bb9..a6927eb647a 100644 --- a/compiler/noirc_errors/Cargo.toml +++ b/compiler/noirc_errors/Cargo.toml @@ -17,7 +17,6 @@ codespan-reporting.workspace = true codespan.workspace = true fm.workspace = true noirc_printable_type.workspace = true -# noirc_evaluator.workspace = true serde.workspace = true serde_with = "3.2.0" tracing.workspace = true diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 9e594e68f3e..bb1048bc865 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -48,9 +48,9 @@ pub(crate) struct BrilligArtifact { /// TODO: and have an enum which indicates whether the jump is internal or external unresolved_external_call_labels: Vec<(JumpInstructionPosition, Label)>, /// Maps the opcodes that are associated with a callstack to it. - pub(crate) locations: BTreeMap, + locations: BTreeMap, /// The current call stack. All opcodes that are pushed will be associated with this call stack. - pub(crate) call_stack: CallStack, + call_stack: CallStack, /// Name of the function, only used for debugging purposes. pub(crate) name: String, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index 820d3278c19..d78364dcc96 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -27,22 +27,6 @@ use super::{ BrilligContext, }; -// /// Procedures are a set of complex operations that are common in the noir language. -// /// Extracting them to reusable procedures allows us to reduce the size of the generated Brillig. -// /// Procedures receive their arguments on scratch space to avoid stack dumping&restoring. -// #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] -// pub enum ProcedureId { -// ArrayCopy, -// ArrayReverse, -// VectorCopy, -// MemCopy, -// PrepareVectorPush(bool), -// VectorPop(bool), -// PrepareVectorInsert, -// VectorRemove, -// CheckMaxStackDepth, -// } - pub(crate) fn compile_procedure( procedure_id: ProcedureId, ) -> BrilligArtifact { diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 29095ae5faa..4fa4474fb4b 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -332,11 +332,6 @@ fn convert_generated_acir_into_circuit( }) .collect(); - let brillig_procedure_locs = if brillig_procedure_locs.is_empty() { - BTreeMap::default() - } else { - brillig_procedure_locs - }; let mut debug_info = DebugInfo::new( locations, brillig_locations, diff --git a/tooling/profiler/src/opcode_formatter.rs b/tooling/profiler/src/opcode_formatter.rs index b044066bdca..68057b6d86f 100644 --- a/tooling/profiler/src/opcode_formatter.rs +++ b/tooling/profiler/src/opcode_formatter.rs @@ -138,9 +138,7 @@ fn format_brillig_opcode_kind(opcode: &BrilligOpcode) -> String { pub(crate) fn format_opcode(opcode: &AcirOrBrilligOpcode) -> String { match opcode { - AcirOrBrilligOpcode::Acir(opcode) => { - format!("acir::{}", format_acir_opcode_kind(opcode)) - } + AcirOrBrilligOpcode::Acir(opcode) => format!("acir::{}", format_acir_opcode_kind(opcode)), AcirOrBrilligOpcode::Brillig(opcode) => { format!("brillig::{}", format_brillig_opcode_kind(opcode)) } From c22b3e6debcfb673e71827eab803bba3f7c49689 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 29 Oct 2024 15:08:36 +0000 Subject: [PATCH 3/8] remove profiling active for brillig procs --- compiler/noirc_driver/src/lib.rs | 5 --- compiler/noirc_evaluator/src/ssa.rs | 5 +-- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 39 +++++++------------ 3 files changed, 16 insertions(+), 33 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 45b10910678..c4e2491a5fe 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -131,10 +131,6 @@ pub struct CompileOptions { /// A less aggressive inliner should generate smaller programs #[arg(long, hide = true, allow_hyphen_values = true, default_value_t = i64::MAX)] pub inliner_aggressiveness: i64, - - /// Gather extra debug information - #[arg(long, hide = true)] - pub profile: bool, } pub fn parse_expression_width(input: &str) -> Result { @@ -594,7 +590,6 @@ pub fn compile_no_check( emit_ssa: if options.emit_ssa { Some(context.package_build_path.clone()) } else { None }, skip_underconstrained_check: options.skip_underconstrained_check, inliner_aggressiveness: options.inliner_aggressiveness, - profiling_active: options.profile, }; let SsaProgramArtifact { program, debug, warnings, names, brillig_names, error_types, .. } = diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 4fa4474fb4b..c993ec291dd 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -70,9 +70,6 @@ pub struct SsaEvaluatorOptions { /// The higher the value, the more inlined brillig functions will be. pub inliner_aggressiveness: i64, - - /// Collect extra debug information for more informative profiling. - pub profiling_active: bool, } pub(crate) struct ArtifactsAndWarnings(Artifacts, Vec); @@ -147,7 +144,7 @@ pub(crate) fn optimize_into_acir( }); let artifacts = time("SSA to ACIR", options.print_codegen_timings, || { - ssa.into_acir(&brillig, options.expression_width, options.profiling_active) + ssa.into_acir(&brillig, options.expression_width) })?; Ok(ArtifactsAndWarnings(artifacts, ssa_level_warnings)) } diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index be6f90a0e9f..0fbff149fed 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -195,9 +195,6 @@ struct Context<'a> { /// Contains state that is generated and also used across ACIR functions shared_context: &'a mut SharedContext, - - /// Flag to determine whether compilation should collect extra debug information for profiling. - profiling_active: bool, } #[derive(Clone)] @@ -295,13 +292,12 @@ impl Ssa { self, brillig: &Brillig, expression_width: ExpressionWidth, - profiling_active: bool, ) -> Result { let mut acirs = Vec::new(); // TODO: can we parallelize this? let mut shared_context = SharedContext::default(); for function in self.functions.values() { - let context = Context::new(&mut shared_context, expression_width, profiling_active); + let context = Context::new(&mut shared_context, expression_width); if let Some(mut generated_acir) = context.convert_ssa_function(&self, function, brillig)? { @@ -353,7 +349,6 @@ impl<'a> Context<'a> { fn new( shared_context: &'a mut SharedContext, expression_width: ExpressionWidth, - profiling_active: bool, ) -> Context<'a> { let mut acir_context = AcirContext::default(); acir_context.set_expression_width(expression_width); @@ -370,7 +365,6 @@ impl<'a> Context<'a> { max_block_id: 0, data_bus: DataBus::default(), shared_context, - profiling_active, } } @@ -1010,16 +1004,13 @@ impl<'a> Context<'a> { }; entry_point.link_with(artifact); // Insert the range of opcode locations occupied by a procedure - if self.profiling_active { - if let Some(procedure_id) = artifact.procedure { - let num_opcodes = entry_point.byte_code.len(); - let previous_num_opcodes = - entry_point.byte_code.len() - artifact.byte_code.len(); - // We subtract one as to keep the range inclusive on both ends - entry_point - .procedure_locations - .insert(procedure_id, (previous_num_opcodes, num_opcodes - 1)); - } + if let Some(procedure_id) = artifact.procedure { + let num_opcodes = entry_point.byte_code.len(); + let previous_num_opcodes = entry_point.byte_code.len() - artifact.byte_code.len(); + // We subtract one as to keep the range inclusive on both ends + entry_point + .procedure_locations + .insert(procedure_id, (previous_num_opcodes, num_opcodes - 1)); } } // Generate the final bytecode @@ -3038,7 +3029,7 @@ mod test { let ssa = builder.finish(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default(), false) + .into_acir(&Brillig::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); // Expected result: // main f0 @@ -3143,7 +3134,7 @@ mod test { let ssa = builder.finish(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default(), false) + .into_acir(&Brillig::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); // The expected result should look very similar to the above test expect that the input witnesses of the `Call` // opcodes will be different. The changes can discerned from the checks below. @@ -3243,7 +3234,7 @@ mod test { let ssa = builder.finish(); let (acir_functions, _, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default(), false) + .into_acir(&Brillig::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 3, "Should have three ACIR functions"); @@ -3367,7 +3358,7 @@ mod test { let brillig = ssa.to_brillig(false); let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default(), false) + .into_acir(&brillig, ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3431,7 +3422,7 @@ mod test { // The Brillig bytecode we insert for the stdlib is hardcoded so we do not need to provide any // Brillig artifacts to the ACIR gen pass. let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&Brillig::default(), ExpressionWidth::default(), false) + .into_acir(&Brillig::default(), ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3505,7 +3496,7 @@ mod test { println!("{}", ssa); let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default(), false) + .into_acir(&brillig, ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 1, "Should only have a `main` ACIR function"); @@ -3593,7 +3584,7 @@ mod test { println!("{}", ssa); let (acir_functions, brillig_functions, _, _) = ssa - .into_acir(&brillig, ExpressionWidth::default(), false) + .into_acir(&brillig, ExpressionWidth::default()) .expect("Should compile manually written SSA into ACIR"); assert_eq!(acir_functions.len(), 2, "Should only have two ACIR functions"); From d3fea17bd99beef6e74538f9d301a01732932381 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 29 Oct 2024 18:50:39 +0000 Subject: [PATCH 4/8] separate debug mapping for procedure ids as to leave ProcedureId in the compiler --- Cargo.lock | 1 + acvm-repo/acir/src/circuit/brillig.rs | 66 ----------- compiler/noirc_errors/src/debug_info.rs | 14 ++- .../src/brillig/brillig_ir/artifact.rs | 4 +- .../src/brillig/brillig_ir/instructions.rs | 2 +- .../src/brillig/brillig_ir/procedures/mod.rs | 108 +++++++++++++++++- compiler/noirc_evaluator/src/brillig/mod.rs | 2 + .../ssa/acir_gen/acir_ir/generated_acir.rs | 11 +- tooling/profiler/Cargo.toml | 1 + tooling/profiler/src/flamegraph.rs | 3 +- 10 files changed, 131 insertions(+), 81 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3add28d3939..75f6040a12d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2776,6 +2776,7 @@ dependencies = [ "noirc_artifacts", "noirc_driver", "noirc_errors", + "noirc_evaluator", "serde", "serde_json", "tempfile", diff --git a/acvm-repo/acir/src/circuit/brillig.rs b/acvm-repo/acir/src/circuit/brillig.rs index 6f92ff51782..a9714ce29b2 100644 --- a/acvm-repo/acir/src/circuit/brillig.rs +++ b/acvm-repo/acir/src/circuit/brillig.rs @@ -1,10 +1,7 @@ -use std::str::FromStr; - use super::opcodes::BlockId; use crate::native_types::{Expression, Witness}; use brillig::Opcode as BrilligOpcode; use serde::{Deserialize, Serialize}; -use thiserror::Error; /// Inputs for the Brillig VM. These are the initial inputs /// that the Brillig VM will use to start. @@ -49,66 +46,3 @@ impl std::fmt::Display for BrilligFunctionId { write!(f, "{}", self.0) } } - -/// Procedures are a set of complex operations that are common in the noir language. -/// Extracting them to reusable procedures allows us to reduce the size of the generated Brillig. -/// Procedures receive their arguments on scratch space to avoid stack dumping&restoring. -#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] -pub enum ProcedureId { - ArrayCopy, - ArrayReverse, - VectorCopy, - MemCopy, - PrepareVectorPush(bool), - VectorPopFront, - VectorPopBack, - PrepareVectorInsert, - VectorRemove, - CheckMaxStackDepth, -} - -impl std::fmt::Display for ProcedureId { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - ProcedureId::ArrayCopy => write!(f, "ArrayCopy"), - ProcedureId::ArrayReverse => write!(f, "ArrayReverse"), - ProcedureId::VectorCopy => write!(f, "VectorCopy"), - ProcedureId::MemCopy => write!(f, "MemCopy"), - ProcedureId::PrepareVectorPush(flag) => write!(f, "PrepareVectorPush({flag})"), - ProcedureId::VectorPopFront => write!(f, "VectorPopFront"), - ProcedureId::VectorPopBack => write!(f, "VectorPopBack"), - ProcedureId::PrepareVectorInsert => write!(f, "PrepareVectorInsert"), - ProcedureId::VectorRemove => write!(f, "VectorRemove"), - ProcedureId::CheckMaxStackDepth => write!(f, "CheckMaxStackDepth"), - } - } -} -#[derive(Error, Debug)] -pub enum ProcedureIdFromStrError { - #[error("Invalid procedure id string: {0}")] - InvalidProcedureIdString(String), -} - -/// The implementation of display and FromStr allows serializing and deserializing a ProcedureId to a string. -/// This is useful when used as key in a map that has to be serialized to JSON/TOML, for example when mapping an opcode to its metadata. -impl FromStr for ProcedureId { - type Err = ProcedureIdFromStrError; - - fn from_str(s: &str) -> Result { - let res = match s { - "ArrayCopy" => ProcedureId::ArrayCopy, - "ArrayReverse" => ProcedureId::ArrayReverse, - "VectorCopy" => ProcedureId::VectorCopy, - "MemCopy" => ProcedureId::MemCopy, - "PrepareVectorPush(true)" => ProcedureId::PrepareVectorPush(true), - "PrepareVectorPush(false)" => ProcedureId::PrepareVectorPush(false), - "VectorPopFront" => ProcedureId::VectorPopFront, - "VectorPopBack" => ProcedureId::VectorPopBack, - "PrepareVectorInsert" => ProcedureId::PrepareVectorInsert, - "VectorRemove" => ProcedureId::VectorRemove, - "CheckMaxStackDepth" => ProcedureId::CheckMaxStackDepth, - _ => return Err(ProcedureIdFromStrError::InvalidProcedureIdString(s.to_string())), - }; - Ok(res) - } -} diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 9d4d7e70702..fed536d34db 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -1,5 +1,4 @@ use acvm::acir::circuit::brillig::BrilligFunctionId; -use acvm::acir::circuit::brillig::ProcedureId; use acvm::acir::circuit::BrilligOpcodeLocation; use acvm::acir::circuit::OpcodeLocation; use acvm::compiler::AcirTransformationMap; @@ -49,6 +48,9 @@ pub type DebugVariables = BTreeMap; pub type DebugFunctions = BTreeMap; pub type DebugTypes = BTreeMap; +#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord, Deserialize, Serialize)] +pub struct ProcedureDebugId(pub u32); + #[derive(Default, Debug, Clone, Deserialize, Serialize)] pub struct ProgramDebugInfo { pub debug_infos: Vec, @@ -106,8 +108,9 @@ pub struct DebugInfo { pub functions: DebugFunctions, pub types: DebugTypes, /// This a map per brillig function representing the range of opcodes where a procedure is activated. - #[serde_as(as = "BTreeMap<_, BTreeMap>")] - pub brillig_procedure_locs: BTreeMap>, + // #[serde_as(as = "BTreeMap<_, BTreeMap>")] + pub brillig_procedure_locs: + BTreeMap>, } /// Holds OpCodes Counts for Acir and Brillig Opcodes @@ -128,7 +131,10 @@ impl DebugInfo { variables: DebugVariables, functions: DebugFunctions, types: DebugTypes, - brillig_procedure_locs: BTreeMap>, + brillig_procedure_locs: BTreeMap< + BrilligFunctionId, + BTreeMap, + >, ) -> Self { Self { locations, brillig_locations, variables, functions, types, brillig_procedure_locs } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index bb1048bc865..276456fc40d 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -1,8 +1,10 @@ -use acvm::acir::{brillig::Opcode as BrilligOpcode, circuit::brillig::ProcedureId}; +use acvm::acir::brillig::Opcode as BrilligOpcode; use std::collections::{BTreeMap, HashMap}; use crate::ssa::ir::{basic_block::BasicBlockId, dfg::CallStack, function::FunctionId}; +use super::procedures::ProcedureId; + /// Represents a parameter or a return value of an entry point function. #[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)] pub(crate) enum BrilligParameter { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs index f77c1b7377f..1ac672687f3 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs @@ -4,7 +4,6 @@ use acvm::{ BinaryFieldOp, BinaryIntOp, BitSize, BlackBoxOp, HeapArray, HeapValueType, MemoryAddress, Opcode as BrilligOpcode, ValueOrArray, }, - circuit::brillig::ProcedureId, AcirField, }, FieldElement, @@ -16,6 +15,7 @@ use super::{ artifact::{Label, UnresolvedJumpLocation}, brillig_variable::SingleAddrVariable, debug_show::DebugToString, + procedures::ProcedureId, registers::RegisterAllocator, BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index e7f8ae9c647..de3cd8ed4b0 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -9,13 +9,14 @@ mod vector_pop_back; mod vector_pop_front; mod vector_remove; -use acvm::acir::circuit::brillig::ProcedureId; use array_copy::compile_array_copy_procedure; use array_reverse::compile_array_reverse_procedure; use check_max_stack_depth::compile_check_max_stack_depth_procedure; use mem_copy::compile_mem_copy_procedure; +use noirc_errors::debug_info::ProcedureDebugId; use prepare_vector_insert::compile_prepare_vector_insert_procedure; use prepare_vector_push::compile_prepare_vector_push_procedure; +use serde::{Deserialize, Serialize}; use vector_copy::compile_vector_copy_procedure; use vector_pop_back::compile_vector_pop_back_procedure; use vector_pop_front::compile_vector_pop_front_procedure; @@ -29,6 +30,111 @@ use super::{ BrilligContext, }; +/// Procedures are a set of complex operations that are common in the noir language. +/// Extracting them to reusable procedures allows us to reduce the size of the generated Brillig. +/// Procedures receive their arguments on scratch space to avoid stack dumping&restoring. +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, PartialOrd, Ord, Deserialize, Serialize)] +pub enum ProcedureId { + ArrayCopy, + ArrayReverse, + VectorCopy, + MemCopy, + PrepareVectorPush(bool), + VectorPopFront, + VectorPopBack, + PrepareVectorInsert, + VectorRemove, + CheckMaxStackDepth, +} + +impl ProcedureId { + pub(crate) fn to_debug_id(self) -> ProcedureDebugId { + ProcedureDebugId(match self { + ProcedureId::ArrayCopy => 0, + ProcedureId::ArrayReverse => 1, + ProcedureId::VectorCopy => 2, + ProcedureId::MemCopy => 3, + ProcedureId::PrepareVectorPush(flag) => { + if flag { + 4 + } else { + 5 + } + } + ProcedureId::VectorPopFront => 6, + ProcedureId::VectorPopBack => 7, + ProcedureId::PrepareVectorInsert => 8, + ProcedureId::VectorRemove => 9, + ProcedureId::CheckMaxStackDepth => 10, + }) + } + + pub fn from_debug_id(debug_id: ProcedureDebugId) -> Self { + let inner = debug_id.0; + match inner { + 0 => ProcedureId::ArrayCopy, + 1 => ProcedureId::ArrayReverse, + 2 => ProcedureId::VectorCopy, + 3 => ProcedureId::MemCopy, + 4 => ProcedureId::PrepareVectorPush(true), + 5 => ProcedureId::PrepareVectorPush(false), + 6 => ProcedureId::VectorPopFront, + 7 => ProcedureId::VectorPopBack, + 8 => ProcedureId::PrepareVectorInsert, + 9 => ProcedureId::VectorRemove, + 10 => ProcedureId::CheckMaxStackDepth, + _ => panic!("Unsupported procedure debug ID of {inner} was supplied"), + } + } +} + +impl std::fmt::Display for ProcedureId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ProcedureId::ArrayCopy => write!(f, "ArrayCopy"), + ProcedureId::ArrayReverse => write!(f, "ArrayReverse"), + ProcedureId::VectorCopy => write!(f, "VectorCopy"), + ProcedureId::MemCopy => write!(f, "MemCopy"), + ProcedureId::PrepareVectorPush(flag) => write!(f, "PrepareVectorPush({flag})"), + ProcedureId::VectorPopFront => write!(f, "VectorPopFront"), + ProcedureId::VectorPopBack => write!(f, "VectorPopBack"), + ProcedureId::PrepareVectorInsert => write!(f, "PrepareVectorInsert"), + ProcedureId::VectorRemove => write!(f, "VectorRemove"), + ProcedureId::CheckMaxStackDepth => write!(f, "CheckMaxStackDepth"), + } + } +} + +// #[derive(Error, Debug)] +// pub enum ProcedureIdFromStrError { +// #[error("Invalid procedure id string: {0}")] +// InvalidProcedureIdString(String), +// } + +// /// The implementation of display and FromStr allows serializing and deserializing a ProcedureId to a string. +// /// This is useful when used as key in a map that has to be serialized to JSON/TOML, for example when mapping an opcode to its metadata. +// impl FromStr for ProcedureId { +// type Err = ProcedureIdFromStrError; + +// fn from_str(s: &str) -> Result { +// let res = match s { +// "ArrayCopy" => ProcedureId::ArrayCopy, +// "ArrayReverse" => ProcedureId::ArrayReverse, +// "VectorCopy" => ProcedureId::VectorCopy, +// "MemCopy" => ProcedureId::MemCopy, +// "PrepareVectorPush(true)" => ProcedureId::PrepareVectorPush(true), +// "PrepareVectorPush(false)" => ProcedureId::PrepareVectorPush(false), +// "VectorPopFront" => ProcedureId::VectorPopFront, +// "VectorPopBack" => ProcedureId::VectorPopBack, +// "PrepareVectorInsert" => ProcedureId::PrepareVectorInsert, +// "VectorRemove" => ProcedureId::VectorRemove, +// "CheckMaxStackDepth" => ProcedureId::CheckMaxStackDepth, +// _ => return Err(ProcedureIdFromStrError::InvalidProcedureIdString(s.to_string())), +// }; +// Ok(res) +// } +// } + pub(crate) fn compile_procedure( procedure_id: ProcedureId, ) -> BrilligArtifact { diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 21b439ac9ff..6f4ee7f7c61 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -18,6 +18,8 @@ use crate::ssa::{ use fxhash::FxHashMap as HashMap; use std::{borrow::Cow, collections::BTreeSet}; +pub use self::brillig_ir::procedures::ProcedureId; + /// Context structure for the brillig pass. /// It stores brillig-related data required for brillig generation. #[derive(Default)] diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index b01fe382ec1..9a12fbe2441 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -9,7 +9,7 @@ use crate::{ }; use acvm::acir::{ circuit::{ - brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs, ProcedureId}, + brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs}, opcodes::{BlackBoxFuncCall, FunctionInput, Opcode as AcirOpcode}, AssertionPayload, BrilligOpcodeLocation, OpcodeLocation, }, @@ -22,6 +22,7 @@ use acvm::{ }; use iter_extended::vecmap; +use noirc_errors::debug_info::ProcedureDebugId; use num_bigint::BigUint; /// Brillig calls such as for the Brillig std lib are resolved only after code generation is finished. @@ -74,10 +75,6 @@ pub(crate) struct GeneratedAcir { /// we can instead keep this map and resolve the Brillig calls at the end of code generation. pub(crate) brillig_stdlib_func_locations: BTreeMap, - /// Flag that determines whether we should gather extra information for profiling - /// such as opcode range of Brillig procedures. - pub(crate) profiling_active: bool, - /// Brillig function id -> Brillig procedure locations map /// This maps allows a profiler to determine which Brillig opcodes /// originated from a reusable procedure. @@ -89,7 +86,7 @@ pub(crate) type OpcodeToLocationsMap = BTreeMap; pub(crate) type BrilligOpcodeToLocationsMap = BTreeMap; -pub(crate) type BrilligProcedureRangeMap = BTreeMap; +pub(crate) type BrilligProcedureRangeMap = BTreeMap; #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq)] pub(crate) enum BrilligStdlibFunc { @@ -608,7 +605,7 @@ impl GeneratedAcir { self.brillig_procedure_locs .entry(brillig_function_index) .or_default() - .insert(*procedure_id, (*start_index, *end_index)); + .insert(procedure_id.to_debug_id(), (*start_index, *end_index)); } for (brillig_index, call_stack) in generated_brillig.locations.iter() { diff --git a/tooling/profiler/Cargo.toml b/tooling/profiler/Cargo.toml index c76b4c73e65..604208b5a54 100644 --- a/tooling/profiler/Cargo.toml +++ b/tooling/profiler/Cargo.toml @@ -33,6 +33,7 @@ acir.workspace = true nargo.workspace = true noirc_errors.workspace = true noirc_abi.workspace = true +noirc_evaluator.workspace = true # Logs tracing-subscriber.workspace = true diff --git a/tooling/profiler/src/flamegraph.rs b/tooling/profiler/src/flamegraph.rs index 39b7aa6d814..0b1ab8c3faa 100644 --- a/tooling/profiler/src/flamegraph.rs +++ b/tooling/profiler/src/flamegraph.rs @@ -11,6 +11,7 @@ use inferno::flamegraph::{from_lines, Options, TextTruncateDirection}; use noirc_errors::debug_info::DebugInfo; use noirc_errors::reporter::line_and_column_from_span; use noirc_errors::Location; +use noirc_evaluator::brillig::ProcedureId; use crate::opcode_formatter::AcirOrBrilligOpcode; @@ -154,7 +155,7 @@ fn find_callsite_labels<'files>( .collect(); if let Some(procedure_id) = procedure_id { - callsite_labels.push(format!("procedure::{}", procedure_id)); + callsite_labels.push(format!("procedure::{}", ProcedureId::from_debug_id(procedure_id))); } callsite_labels From 03f0b60c1cff852ca26a1c14a7994623e117b098 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 29 Oct 2024 18:51:13 +0000 Subject: [PATCH 5/8] remove old comments --- compiler/noirc_errors/src/debug_info.rs | 1 - .../src/brillig/brillig_ir/procedures/mod.rs | 30 ------------------- 2 files changed, 31 deletions(-) diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index fed536d34db..9267722f5a2 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -108,7 +108,6 @@ pub struct DebugInfo { pub functions: DebugFunctions, pub types: DebugTypes, /// This a map per brillig function representing the range of opcodes where a procedure is activated. - // #[serde_as(as = "BTreeMap<_, BTreeMap>")] pub brillig_procedure_locs: BTreeMap>, } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index de3cd8ed4b0..4fe4b25c1bb 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -105,36 +105,6 @@ impl std::fmt::Display for ProcedureId { } } -// #[derive(Error, Debug)] -// pub enum ProcedureIdFromStrError { -// #[error("Invalid procedure id string: {0}")] -// InvalidProcedureIdString(String), -// } - -// /// The implementation of display and FromStr allows serializing and deserializing a ProcedureId to a string. -// /// This is useful when used as key in a map that has to be serialized to JSON/TOML, for example when mapping an opcode to its metadata. -// impl FromStr for ProcedureId { -// type Err = ProcedureIdFromStrError; - -// fn from_str(s: &str) -> Result { -// let res = match s { -// "ArrayCopy" => ProcedureId::ArrayCopy, -// "ArrayReverse" => ProcedureId::ArrayReverse, -// "VectorCopy" => ProcedureId::VectorCopy, -// "MemCopy" => ProcedureId::MemCopy, -// "PrepareVectorPush(true)" => ProcedureId::PrepareVectorPush(true), -// "PrepareVectorPush(false)" => ProcedureId::PrepareVectorPush(false), -// "VectorPopFront" => ProcedureId::VectorPopFront, -// "VectorPopBack" => ProcedureId::VectorPopBack, -// "PrepareVectorInsert" => ProcedureId::PrepareVectorInsert, -// "VectorRemove" => ProcedureId::VectorRemove, -// "CheckMaxStackDepth" => ProcedureId::CheckMaxStackDepth, -// _ => return Err(ProcedureIdFromStrError::InvalidProcedureIdString(s.to_string())), -// }; -// Ok(res) -// } -// } - pub(crate) fn compile_procedure( procedure_id: ProcedureId, ) -> BrilligArtifact { From b291fd8b01730cdb9c5a53604b79d45f7e7e2739 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 29 Oct 2024 18:53:38 +0000 Subject: [PATCH 6/8] cleanup --- tooling/profiler/src/flamegraph.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tooling/profiler/src/flamegraph.rs b/tooling/profiler/src/flamegraph.rs index 0b1ab8c3faa..7882ac903ef 100644 --- a/tooling/profiler/src/flamegraph.rs +++ b/tooling/profiler/src/flamegraph.rs @@ -25,7 +25,7 @@ pub(crate) struct Sample { pub(crate) brillig_function_id: Option, } -#[derive(Debug, Default, Clone)] +#[derive(Debug, Default)] pub(crate) struct FoldedStackItem { pub(crate) total_samples: usize, pub(crate) nested_items: BTreeMap, @@ -135,6 +135,7 @@ fn find_callsite_labels<'files>( for (procedure, range) in procedure_locs.iter() { if brillig_location.0 >= range.0 && brillig_location.0 <= range.1 { procedure_id = Some(*procedure); + break; } } } From 9c11a22f5adb41d01270a58ac2ce7daabc4996ef Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 30 Oct 2024 13:56:51 -0400 Subject: [PATCH 7/8] Update compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs Co-authored-by: Ary Borenszweig --- .../src/brillig/brillig_ir/procedures/mod.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index 4fe4b25c1bb..4b4d9359d5e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -54,13 +54,8 @@ impl ProcedureId { ProcedureId::ArrayReverse => 1, ProcedureId::VectorCopy => 2, ProcedureId::MemCopy => 3, - ProcedureId::PrepareVectorPush(flag) => { - if flag { - 4 - } else { - 5 - } - } + ProcedureId::PrepareVectorPush(true) => 4, + ProcedureId::PrepareVectorPush(false) => 5, ProcedureId::VectorPopFront => 6, ProcedureId::VectorPopBack => 7, ProcedureId::PrepareVectorInsert => 8, From 14967c9b937fe899a2640683b9915fdd740f24fc Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 30 Oct 2024 18:05:19 +0000 Subject: [PATCH 8/8] set procedure on artifact in constructor --- compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 11 +++++++++-- .../src/brillig/brillig_ir/procedures/mod.rs | 2 +- compiler/noirc_evaluator/src/brillig/mod.rs | 6 +----- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 0c6097b8f6d..3633700a795 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -36,6 +36,8 @@ use acvm::{ }; use debug_show::DebugShow; +use super::ProcedureId; + /// The Brillig VM does not apply a limit to the memory address space, /// As a convention, we take use 32 bits. This means that we assume that /// memory has 2^32 memory slots. @@ -111,9 +113,14 @@ impl BrilligContext { /// Special brillig context to codegen compiler intrinsic shared procedures impl BrilligContext { - pub(crate) fn new_for_procedure(enable_debug_trace: bool) -> BrilligContext { + pub(crate) fn new_for_procedure( + enable_debug_trace: bool, + procedure_id: ProcedureId, + ) -> BrilligContext { + let mut obj = BrilligArtifact::default(); + obj.procedure = Some(procedure_id); BrilligContext { - obj: BrilligArtifact::default(), + obj, registers: ScratchSpace::new(), context_label: Label::entrypoint(), current_section: 0, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index 4b4d9359d5e..e17b4ac9359 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -103,7 +103,7 @@ impl std::fmt::Display for ProcedureId { pub(crate) fn compile_procedure( procedure_id: ProcedureId, ) -> BrilligArtifact { - let mut brillig_context = BrilligContext::new_for_procedure(false); + let mut brillig_context = BrilligContext::new_for_procedure(false, procedure_id); brillig_context.enter_context(Label::procedure(procedure_id)); match procedure_id { diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 6f4ee7f7c61..1b61ae1a864 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -45,11 +45,7 @@ impl Brillig { self.ssa_function_to_brillig.get(&function_id).map(Cow::Borrowed) } // Procedures are compiled as needed - LabelType::Procedure(procedure_id) => { - let mut artifact = compile_procedure(procedure_id); - artifact.procedure = Some(procedure_id); - Some(Cow::Owned(artifact)) - } + LabelType::Procedure(procedure_id) => Some(Cow::Owned(compile_procedure(procedure_id))), _ => unreachable!("ICE: Expected a function or procedure label"), } }