Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: Do not duplicate redundant Brillig debug metadata #5696

Merged
merged 16 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
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 Expand Up @@ -153,7 +153,7 @@

Opcode::BlackBoxFuncCall(g) => write!(f, "{g}"),
Opcode::Directive(Directive::ToLeRadix { a, b, radix: _ }) => {
write!(f, "DIR::TORADIX ")?;

Check warning on line 156 in acvm-repo/acir/src/circuit/opcodes.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (TORADIX)
write!(
f,
// TODO (Note): this assumes that the decomposed bits have contiguous witness indices
Expand Down Expand Up @@ -184,7 +184,7 @@
match databus {
BlockType::Memory => write!(f, "INIT ")?,
BlockType::CallData => write!(f, "INIT CALLDATA ")?,
BlockType::ReturnData => write!(f, "INIT RETURNDATA ")?,

Check warning on line 187 in acvm-repo/acir/src/circuit/opcodes.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (RETURNDATA)
}
write!(f, "(id: {}, len: {}) ", block_id.0, init.len())
}
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 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 @@ -56,7 +56,7 @@
RequiresForeignCall(ForeignCallWaitInfo<F>),

/// The ACVM has encountered a request for an ACIR [call][acir::circuit::Opcode]
/// to execute a separate ACVM instance. The result of the ACIR call must be passd back to the ACVM.

Check warning on line 59 in acvm-repo/acvm/src/pwg/mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (passd)
///
/// Once this is done, the ACVM can be restarted to solve the remaining opcodes.
RequiresAcirCall(AcirCallWaitInfo<F>),
Expand Down Expand Up @@ -132,6 +132,7 @@
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 @@
&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 @@

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 @@
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
Loading