Skip to content

Commit

Permalink
fix: Do not duplicate redundant Brillig debug metadata (#5696)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #5521 

## Summary\*

This PR adds a new `brillig_locations` map to our generated ACIR that
maps a brillig function id to an opcode locations map
`BTreeMap<OpcodeLocation, CallStack>`. If we have called that Brillig
function id before we do not add its debug locations.

For the same test as in the issue I ran `du -sh ./target` and got a size
of 2.3M. This is a >400x reduction in size of the artifact. Compilation
also takes a matter of a couple seconds now rather than 10 minutes.

## Additional Context

The call stacks are the same as expected. For example, under
`execution_failure/fold_nested_brillig_assert_fail` we have the
following call stack:
<img width="871" alt="Screenshot 2024-08-08 at 10 33 28 AM"
src="https://github.com/user-attachments/assets/716d5e53-dc7b-4873-950f-2e1b8fd6c5b9">


## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] 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: jfecher <jake@aztecprotocol.com>
  • Loading branch information
vezenovm and jfecher authored Aug 8, 2024
1 parent 2882eae commit e4f7dbe
Show file tree
Hide file tree
Showing 17 changed files with 202 additions and 71 deletions.
19 changes: 19 additions & 0 deletions acvm-repo/acir/src/circuit/brillig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,22 @@ pub enum BrilligOutputs {
pub struct BrilligBytecode<F> {
pub bytecode: Vec<BrilligOpcode<F>>,
}

/// Id for the function being called.
#[derive(
Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Hash, Copy, Default, PartialOrd, Ord,
)]
#[serde(transparent)]
pub struct BrilligFunctionId(pub u32);

impl BrilligFunctionId {
pub fn as_usize(&self) -> usize {
self.0 as usize
}
}

impl std::fmt::Display for BrilligFunctionId {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0)
}
}
4 changes: 2 additions & 2 deletions acvm-repo/acir/src/circuit/opcodes.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{
brillig::{BrilligInputs, BrilligOutputs},
brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs},
directives::Directive,
};
use crate::native_types::{Expression, Witness};
Expand Down Expand Up @@ -111,7 +111,7 @@ pub enum Opcode<F> {
BrilligCall {
/// Id for the function being called. It is the responsibility of the executor
/// to fetch the appropriate Brillig bytecode from this id.
id: u32,
id: BrilligFunctionId,
/// Inputs to the function call
inputs: Vec<BrilligInputs<F>>,
/// Outputs to the function call
Expand Down
6 changes: 3 additions & 3 deletions acvm-repo/acir/tests/test_program_serialization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::collections::BTreeSet;

use acir::{
circuit::{
brillig::{BrilligBytecode, BrilligInputs, BrilligOutputs},
brillig::{BrilligBytecode, BrilligFunctionId, BrilligInputs, BrilligOutputs},
opcodes::{BlackBoxFuncCall, BlockId, FunctionInput, MemOp},
Circuit, Opcode, Program, PublicInputs,
},
Expand Down Expand Up @@ -181,7 +181,7 @@ fn simple_brillig_foreign_call() {
};

let opcodes = vec![Opcode::BrilligCall {
id: 0,
id: BrilligFunctionId(0),
inputs: vec![
BrilligInputs::Single(w_input.into()), // Input Register 0,
],
Expand Down Expand Up @@ -272,7 +272,7 @@ fn complex_brillig_foreign_call() {
};

let opcodes = vec![Opcode::BrilligCall {
id: 0,
id: BrilligFunctionId(0),
inputs: vec![
// Input 0,1,2
BrilligInputs::Array(vec![
Expand Down
15 changes: 12 additions & 3 deletions acvm-repo/acvm/src/pwg/brillig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::collections::HashMap;
use acir::{
brillig::{ForeignCallParam, ForeignCallResult, Opcode as BrilligOpcode},
circuit::{
brillig::{BrilligInputs, BrilligOutputs},
brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs},
opcodes::BlockId,
ErrorSelector, OpcodeLocation, RawAssertionPayload, ResolvedAssertionPayload,
STRING_ERROR_SELECTOR,
Expand All @@ -29,6 +29,10 @@ pub enum BrilligSolverStatus<F> {
pub struct BrilligSolver<'b, F, B: BlackBoxFunctionSolver<F>> {
vm: VM<'b, F, B>,
acir_index: usize,
/// This id references which Brillig function within the main ACIR program we are solving.
/// This is used for appropriately resolving errors as the ACIR program artifacts
/// set up their Brillig debug metadata by function id.
function_id: BrilligFunctionId,
}

impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
Expand Down Expand Up @@ -61,10 +65,11 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
brillig_bytecode: &'b [BrilligOpcode<F>],
bb_solver: &'b B,
acir_index: usize,
brillig_function_id: BrilligFunctionId,
) -> Result<Self, OpcodeResolutionError<F>> {
let vm =
Self::setup_brillig_vm(initial_witness, memory, inputs, brillig_bytecode, bb_solver)?;
Ok(Self { vm, acir_index })
Ok(Self { vm, acir_index, function_id: brillig_function_id })
}

fn setup_brillig_vm(
Expand Down Expand Up @@ -182,7 +187,11 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
}
};

Err(OpcodeResolutionError::BrilligFunctionFailed { payload, call_stack })
Err(OpcodeResolutionError::BrilligFunctionFailed {
function_id: self.function_id,
payload,
call_stack,
})
}
VMStatus::ForeignCallWait { function, inputs } => {
Ok(BrilligSolverStatus::ForeignCallWait(ForeignCallWaitInfo { function, inputs }))
Expand Down
11 changes: 7 additions & 4 deletions acvm-repo/acvm/src/pwg/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::collections::HashMap;
use acir::{
brillig::ForeignCallResult,
circuit::{
brillig::BrilligBytecode,
brillig::{BrilligBytecode, BrilligFunctionId},
opcodes::{BlockId, ConstantOrWitnessEnum, FunctionInput},
AssertionPayload, ErrorSelector, ExpressionOrMemory, Opcode, OpcodeLocation,
RawAssertionPayload, ResolvedAssertionPayload, STRING_ERROR_SELECTOR,
Expand Down Expand Up @@ -132,6 +132,7 @@ pub enum OpcodeResolutionError<F> {
BlackBoxFunctionFailed(BlackBoxFunc, String),
#[error("Failed to solve brillig function")]
BrilligFunctionFailed {
function_id: BrilligFunctionId,
call_stack: Vec<OpcodeLocation>,
payload: Option<ResolvedAssertionPayload<F>>,
},
Expand Down Expand Up @@ -475,9 +476,10 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver<F>> ACVM<'a, F, B> {
&self.witness_map,
&self.block_solvers,
inputs,
&self.unconstrained_functions[*id as usize].bytecode,
&self.unconstrained_functions[id.as_usize()].bytecode,
self.backend,
self.instruction_pointer,
*id,
)?,
};

Expand All @@ -502,7 +504,7 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver<F>> ACVM<'a, F, B> {

fn map_brillig_error(&self, mut err: OpcodeResolutionError<F>) -> OpcodeResolutionError<F> {
match &mut err {
OpcodeResolutionError::BrilligFunctionFailed { call_stack, payload } => {
OpcodeResolutionError::BrilligFunctionFailed { call_stack, payload, .. } => {
// Some brillig errors have static strings as payloads, we can resolve them here
let last_location =
call_stack.last().expect("Call stacks should have at least one item");
Expand Down Expand Up @@ -546,9 +548,10 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver<F>> ACVM<'a, F, B> {
witness,
&self.block_solvers,
inputs,
&self.unconstrained_functions[*id as usize].bytecode,
&self.unconstrained_functions[id.as_usize()].bytecode,
self.backend,
self.instruction_pointer,
*id,
);
match solver {
Ok(solver) => StepResult::IntoBrillig(solver),
Expand Down
13 changes: 7 additions & 6 deletions acvm-repo/acvm/tests/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use acir::{
acir_field::GenericFieldElement,
brillig::{BinaryFieldOp, HeapArray, MemoryAddress, Opcode as BrilligOpcode, ValueOrArray},
circuit::{
brillig::{BrilligBytecode, BrilligInputs, BrilligOutputs},
brillig::{BrilligBytecode, BrilligFunctionId, BrilligInputs, BrilligOutputs},
opcodes::{BlackBoxFuncCall, BlockId, BlockType, FunctionInput, MemOp},
Opcode, OpcodeLocation,
},
Expand Down Expand Up @@ -82,7 +82,7 @@ fn inversion_brillig_oracle_equivalence() {

let opcodes = vec![
Opcode::BrilligCall {
id: 0,
id: BrilligFunctionId(0),
inputs: vec![
BrilligInputs::Single(Expression {
// Input Register 0
Expand Down Expand Up @@ -211,7 +211,7 @@ fn double_inversion_brillig_oracle() {

let opcodes = vec![
Opcode::BrilligCall {
id: 0,
id: BrilligFunctionId(0),
inputs: vec![
BrilligInputs::Single(Expression {
// Input Register 0
Expand Down Expand Up @@ -406,7 +406,7 @@ fn oracle_dependent_execution() {
let opcodes = vec![
Opcode::AssertZero(equality_check),
Opcode::BrilligCall {
id: 0,
id: BrilligFunctionId(0),
inputs: vec![
BrilligInputs::Single(w_x.into()), // Input Register 0
BrilligInputs::Single(Expression::default()), // Input Register 1
Expand Down Expand Up @@ -514,7 +514,7 @@ fn brillig_oracle_predicate() {
};

let opcodes = vec![Opcode::BrilligCall {
id: 0,
id: BrilligFunctionId(0),
inputs: vec![
BrilligInputs::Single(Expression {
mul_terms: vec![],
Expand Down Expand Up @@ -650,7 +650,7 @@ fn unsatisfied_opcode_resolved_brillig() {

let opcodes = vec![
Opcode::BrilligCall {
id: 0,
id: BrilligFunctionId(0),
inputs: vec![
BrilligInputs::Single(Expression {
mul_terms: vec![],
Expand All @@ -675,6 +675,7 @@ fn unsatisfied_opcode_resolved_brillig() {
assert_eq!(
solver_status,
ACVMStatus::Failure(OpcodeResolutionError::BrilligFunctionFailed {
function_id: BrilligFunctionId(0),
payload: None,
call_stack: vec![OpcodeLocation::Brillig { acir_index: 0, brillig_index: 3 }]
}),
Expand Down
6 changes: 5 additions & 1 deletion compiler/noirc_errors/src/debug_info.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use acvm::acir::circuit::brillig::BrilligFunctionId;
use acvm::acir::circuit::OpcodeLocation;
use acvm::compiler::AcirTransformationMap;

Expand Down Expand Up @@ -97,6 +98,8 @@ pub struct DebugInfo {
/// that they should be serialized to/from strings.
#[serde_as(as = "BTreeMap<DisplayFromStr, _>")]
pub locations: BTreeMap<OpcodeLocation, Vec<Location>>,
#[serde_as(as = "BTreeMap<_, BTreeMap<DisplayFromStr, _>>")]
pub brillig_locations: BTreeMap<BrilligFunctionId, BTreeMap<OpcodeLocation, Vec<Location>>>,
pub variables: DebugVariables,
pub functions: DebugFunctions,
pub types: DebugTypes,
Expand All @@ -113,11 +116,12 @@ pub struct OpCodesCount {
impl DebugInfo {
pub fn new(
locations: BTreeMap<OpcodeLocation, Vec<Location>>,
brillig_locations: BTreeMap<BrilligFunctionId, BTreeMap<OpcodeLocation, Vec<Location>>>,
variables: DebugVariables,
functions: DebugFunctions,
types: DebugTypes,
) -> Self {
Self { locations, variables, functions, types }
Self { locations, brillig_locations, variables, functions, types }
}

/// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified.
Expand Down
15 changes: 14 additions & 1 deletion compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ fn convert_generated_acir_into_circuit(
let GeneratedAcir {
return_witnesses,
locations,
brillig_locations,
input_witnesses,
assertion_payloads: assert_messages,
warnings,
Expand Down Expand Up @@ -292,7 +293,19 @@ fn convert_generated_acir_into_circuit(
.map(|(index, locations)| (index, locations.into_iter().collect()))
.collect();

let mut debug_info = DebugInfo::new(locations, debug_variables, debug_functions, debug_types);
let brillig_locations = brillig_locations
.into_iter()
.map(|(function_index, locations)| {
let locations = locations
.into_iter()
.map(|(index, locations)| (index, locations.into_iter().collect()))
.collect();
(function_index, locations)
})
.collect();

let mut debug_info =
DebugInfo::new(locations, brillig_locations, debug_variables, debug_functions, debug_types);

// Perform any ACIR-level optimizations
let (optimized_circuit, transformation_map) = acvm::compiler::optimize(circuit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::ssa::acir_gen::{AcirDynamicArray, AcirValue};
use crate::ssa::ir::dfg::CallStack;
use crate::ssa::ir::types::Type as SsaType;
use crate::ssa::ir::{instruction::Endian, types::NumericType};
use acvm::acir::circuit::brillig::{BrilligInputs, BrilligOutputs};
use acvm::acir::circuit::brillig::{BrilligFunctionId, BrilligInputs, BrilligOutputs};
use acvm::acir::circuit::opcodes::{BlockId, BlockType, MemOp};
use acvm::acir::circuit::{AssertionPayload, ExpressionOrMemory, ExpressionWidth, Opcode};
use acvm::blackbox_solver;
Expand Down Expand Up @@ -1612,7 +1612,7 @@ impl<F: AcirField> AcirContext<F> {
outputs: Vec<AcirType>,
attempt_execution: bool,
unsafe_return_values: bool,
brillig_function_index: u32,
brillig_function_index: BrilligFunctionId,
brillig_stdlib_func: Option<BrilligStdlibFunc>,
) -> Result<Vec<AcirValue>, RuntimeError> {
let predicate = self.var_to_expression(predicate)?;
Expand Down
Loading

0 comments on commit e4f7dbe

Please sign in to comment.