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

feat: Update to acvm 0.22.0 #2363

Merged
merged 5 commits into from
Aug 21, 2023
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
453 changes: 237 additions & 216 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ edition = "2021"
rust-version = "1.66"

[workspace.dependencies]
acvm = "0.21.0"
acvm = "0.22.0"
arena = { path = "crates/arena" }
fm = { path = "crates/fm" }
iter-extended = { path = "crates/iter-extended" }
Expand Down
2 changes: 1 addition & 1 deletion crates/nargo_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ tokio = { version = "1.0", features = ["io-std"] }
tokio-util = { version = "0.7.8", features = ["compat"] }

# Backends
acvm-backend-barretenberg = { version = "0.10.0", default-features = false }
acvm-backend-barretenberg = { version = "0.11.0", default-features = false }

[dev-dependencies]
tempdir = "0.3.7"
Expand Down
28 changes: 6 additions & 22 deletions crates/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use acvm::acir::circuit::OpcodeLabel;
use acvm::{acir::circuit::Circuit, Backend};
use acvm::{acir::circuit::Circuit, compiler::AcirTransformationMap, Backend};
use iter_extended::try_vecmap;
use iter_extended::vecmap;
use nargo::artifacts::debug::DebugArtifact;
use nargo::package::Package;
use nargo::prepare_package;
Expand Down Expand Up @@ -171,27 +169,20 @@ pub(crate) fn compile_package<B: Backend>(
let result = compile_main(&mut context, crate_id, compile_options);
let mut program = report_errors(result, &context, compile_options.deny_warnings)?;
// Apply backend specific optimizations.
let (optimized_circuit, opcode_labels) = optimize_circuit(backend, program.circuit)
let (optimized_circuit, location_map) = optimize_circuit(backend, program.circuit)
.expect("Backend does not support an opcode that is in the IR");

// TODO(#2110): Why does this set `program.circuit` to `optimized_circuit` instead of the function taking ownership
// and requiring we use `optimized_circuit` everywhere after
program.circuit = optimized_circuit;
let opcode_ids = vecmap(opcode_labels, |label| match label {
OpcodeLabel::Unresolved => {
unreachable!("Compiled circuit opcodes must resolve to some index")
}
OpcodeLabel::Resolved(index) => index as usize,
});
program.debug.update_acir(opcode_ids);
program.debug.update_acir(location_map);

Ok((context, program))
}

pub(super) fn optimize_circuit<B: Backend>(
backend: &B,
circuit: Circuit,
) -> Result<(Circuit, Vec<OpcodeLabel>), CliError<B>> {
) -> Result<(Circuit, AcirTransformationMap), CliError<B>> {
let result = acvm::compiler::compile(circuit, backend.np_language(), |opcode| {
backend.supports_opcode(opcode)
})
Expand All @@ -205,16 +196,9 @@ pub(super) fn optimize_contract<B: Backend>(
contract: CompiledContract,
) -> Result<CompiledContract, CliError<B>> {
let functions = try_vecmap(contract.functions, |mut func| {
let (optimized_bytecode, opcode_labels) = optimize_circuit(backend, func.bytecode)?;
let opcode_ids = vecmap(opcode_labels, |label| match label {
OpcodeLabel::Unresolved => {
unreachable!("Compiled circuit opcodes must resolve to some index")
}
OpcodeLabel::Resolved(index) => index as usize,
});

let (optimized_bytecode, location_map) = optimize_circuit(backend, func.bytecode)?;
func.bytecode = optimized_bytecode;
func.debug.update_acir(opcode_ids);
func.debug.update_acir(location_map);
Ok::<_, CliError<B>>(func)
})?;

Expand Down
31 changes: 17 additions & 14 deletions crates/nargo_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use acvm::acir::circuit::OpcodeLabel;
use acvm::acir::circuit::OpcodeLocation;
use acvm::acir::{circuit::Circuit, native_types::WitnessMap};
use acvm::pwg::ErrorLocation;
use acvm::Backend;
use clap::Args;
use nargo::constants::PROVER_INPUT_FILE;
Expand Down Expand Up @@ -90,32 +91,34 @@ fn execute_package<B: Backend>(
Ok((return_value, solved_witness))
}

fn extract_unsatisfied_constraint_from_nargo_error(nargo_err: &NargoError) -> Option<usize> {
fn extract_unsatisfied_constraint_from_nargo_error(
nargo_err: &NargoError,
) -> Option<OpcodeLocation> {
let solving_err = match nargo_err {
nargo::NargoError::SolvingError(err) => err,
_ => return None,
};

match solving_err {
acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { opcode_label } => {
match opcode_label {
OpcodeLabel::Unresolved => {
unreachable!("Cannot resolve index for unsatisfied constraint")
}
OpcodeLabel::Resolved(opcode_index) => Some(*opcode_index as usize),
acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain {
opcode_location: error_location,
} => match error_location {
ErrorLocation::Unresolved => {
unreachable!("Cannot resolve index for unsatisfied constraint")
}
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
}
ErrorLocation::Resolved(opcode_location) => Some(*opcode_location),
},
_ => None,
}
}

fn report_unsatisfied_constraint_error(
opcode_idx: Option<usize>,
opcode_location: Option<OpcodeLocation>,
debug: &DebugInfo,
context: &Context,
) {
if let Some(opcode_index) = opcode_idx {
if let Some(locations) = debug.opcode_location(opcode_index) {
if let Some(opcode_location) = opcode_location {
if let Some(locations) = debug.opcode_location(&opcode_location) {
// The location of the error itself will be the location at the top
// of the call stack (the last item in the Vec).
if let Some(location) = locations.last() {
Expand All @@ -142,8 +145,8 @@ pub(crate) fn execute_program<B: Backend>(
Ok(solved_witness) => Ok(solved_witness),
Err(err) => {
if let Some((debug, context)) = debug_data {
let opcode_idx = extract_unsatisfied_constraint_from_nargo_error(&err);
report_unsatisfied_constraint_error(opcode_idx, &debug, &context);
let opcode_location = extract_unsatisfied_constraint_from_nargo_error(&err);
report_unsatisfied_constraint_error(opcode_location, &debug, &context);
}

Err(crate::errors::CliError::NargoError(err))
Expand Down
4 changes: 3 additions & 1 deletion crates/noirc_errors/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ edition.workspace = true
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
acvm.workspace = true
codespan-reporting.workspace = true
codespan.workspace = true
fm.workspace = true
chumsky.workspace = true
serde.workspace = true
serde.workspace = true
serde_with = "3.2.0"
44 changes: 26 additions & 18 deletions crates/noirc_errors/src/debug_info.rs
Original file line number Diff line number Diff line change
@@ -1,39 +1,47 @@
use acvm::acir::circuit::OpcodeLocation;
use acvm::compiler::AcirTransformationMap;

use serde_with::serde_as;
use serde_with::DisplayFromStr;
use std::collections::BTreeMap;

use crate::Location;
use serde::{Deserialize, Serialize};

#[serde_as]
#[derive(Default, Debug, Clone, Deserialize, Serialize)]
pub struct DebugInfo {
/// Map opcode index of an ACIR circuit into the source code location
pub locations: BTreeMap<usize, Vec<Location>>,
/// Serde does not support mapping keys being enums for json, so we indicate
/// that they should be serialized to/from strings.
#[serde_as(as = "BTreeMap<DisplayFromStr, _>")]
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
pub locations: BTreeMap<OpcodeLocation, Vec<Location>>,
}

impl DebugInfo {
pub fn new(locations: BTreeMap<usize, Vec<Location>>) -> Self {
pub fn new(locations: BTreeMap<OpcodeLocation, Vec<Location>>) -> Self {
DebugInfo { locations }
}

/// Updates the locations map when the circuit is modified
/// Updates the locations map when the [`Circuit`][acvm::acir::circuit::Circuit] is modified.
///
/// When the circuit is generated, the indices are 0,1,..,n
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
/// When the circuit is modified, the opcodes are eventually
/// mixed, removed, or with new ones. For instance 5,2,6,n+1,0,12,..
/// Since new opcodes (n+1 in the ex) don't have a location
/// we use the index of the old opcode that they replace.
/// This is the case during fallback or width 'optimization'
/// opcode_indices is this list of mixed indices
pub fn update_acir(&mut self, opcode_indices: Vec<usize>) {
let mut new_locations = BTreeMap::new();
for (i, idx) in opcode_indices.iter().enumerate() {
if self.locations.contains_key(idx) {
new_locations.insert(i, self.locations[idx].clone());
/// The [`OpcodeLocation`]s are generated with the ACIR, but passing the ACIR through a transformation step
/// renders the old `OpcodeLocation`s invalid. The AcirTransformationMap is able to map the old `OpcodeLocation` to the new ones.
/// Note: One old `OpcodeLocation` might have transformed into more than one new `OpcodeLocation`.
pub fn update_acir(&mut self, update_map: AcirTransformationMap) {
let mut new_locations_map = BTreeMap::new();

for (old_opcode_location, source_locations) in &self.locations {
let new_opcode_locations = update_map.new_locations(*old_opcode_location);
for new_opcode_location in new_opcode_locations {
new_locations_map.insert(new_opcode_location, source_locations.clone());
}
}
self.locations = new_locations;

self.locations = new_locations_map;
}

pub fn opcode_location(&self, idx: usize) -> Option<Vec<Location>> {
self.locations.get(&idx).cloned()
pub fn opcode_location(&self, loc: &OpcodeLocation) -> Option<Vec<Location>> {
self.locations.get(loc).cloned()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
brillig::{Brillig as AcvmBrillig, BrilligInputs, BrilligOutputs},
directives::LogInfo,
opcodes::{BlackBoxFuncCall, FunctionInput, Opcode as AcirOpcode},
OpcodeLocation,
},
native_types::Witness,
BlackBoxFunc,
Expand Down Expand Up @@ -44,9 +45,9 @@

/// All witness indices which are inputs to the main function
pub(crate) input_witnesses: Vec<Witness>,

Check warning on line 48 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (Correspondance)
/// Correspondance between an opcode index (in opcodes) and the source code call stack which generated it
pub(crate) locations: BTreeMap<usize, CallStack>,
pub(crate) locations: BTreeMap<OpcodeLocation, CallStack>,

/// Source code location of the current instruction being processed
/// None if we do not know the location
Expand All @@ -63,7 +64,8 @@
fn push_opcode(&mut self, opcode: AcirOpcode) {
self.opcodes.push(opcode);
if !self.call_stack.is_empty() {
self.locations.insert(self.opcodes.len() - 1, self.call_stack.clone());
self.locations
.insert(OpcodeLocation::Acir(self.opcodes.len() - 1), self.call_stack.clone());
}
}

Expand Down Expand Up @@ -343,8 +345,8 @@
(&*lhs_reduced * &*rhs_reduced).expect("Both expressions are reduced to be degree <= 1")
}

/// Signed division lhs / rhs

Check warning on line 348 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (euclidian)
/// We derive the signed division from the unsigned euclidian division.

Check warning on line 349 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (euclidian)
/// note that this is not euclidian division!
// if x is a signed integer, then sign(x)x >= 0
// so if a and b are signed integers, we can do the unsigned division:
Expand Down Expand Up @@ -745,7 +747,7 @@
) -> Result<Witness, RuntimeError> {
// Ensure that 2^{max_bits + 1} is less than the field size
//
// TODO: perhaps this should be a user error, instead of an assert

Check warning on line 750 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (Euclidian)
assert!(max_bits + 1 < FieldElement::max_num_bits());

// Compute : 2^{max_bits} + a - b
Expand Down
Loading