From 565e2a4087b5912e06516a28265a90d73acd7ed2 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 15 Dec 2023 08:53:26 +0000 Subject: [PATCH 1/5] feat: speed up transformation of debug messages --- acvm-repo/acvm/src/compiler/mod.rs | 42 ++++++++++++++++++++++++- compiler/noirc_errors/src/debug_info.rs | 30 +++++++++++++++--- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 4abf94a2e78..81c3c9ff4ec 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -1,3 +1,5 @@ +use std::collections::BTreeMap; + use acir::{ circuit::{opcodes::UnsupportedMemoryOpcode, Circuit, Opcode, OpcodeLocation}, BlackBoxFunc, @@ -33,7 +35,45 @@ pub struct AcirTransformationMap { } impl AcirTransformationMap { - pub fn new_locations( + /// Returns a `BTreeMap` which maps an ACIR index in the untransformed [`Circuit`] to the set of ACIR indices + /// in the new [`Circuit`] corresponding to the opcodes. + pub fn get_opcode_mapping(&self) -> BTreeMap> { + let mut new_opcode_to_old_map: BTreeMap> = BTreeMap::new(); + let mut index = 0; + let mut old_index = 0; + + let mut temp: Vec = Vec::new(); + while old_index <= *self.acir_opcode_positions.last().unwrap() { + let val = self.acir_opcode_positions[index]; + match old_index.cmp(&val) { + std::cmp::Ordering::Less => { + new_opcode_to_old_map.insert(old_index, temp); + temp = Vec::new(); + old_index = val; + } + std::cmp::Ordering::Equal => { + temp.push(index); + index += 1; + if index == self.acir_opcode_positions.len() { + new_opcode_to_old_map.insert(old_index, temp); + break; + } + } + std::cmp::Ordering::Greater => { + // We assume that `self.acir_opcodes_positions` is sorted. For this assumption to be broken + // we would require the situation where a circuit `[opcode_1, opcode_2]` is optimized such that + // the new opcodes generated from `opcode_1` would be executed after the opcodes from `opcode_2`. + // + // There is no reason for this to occur so we can discount it. + unreachable!("`old_index` cannot exceed `val`") + } + } + } + + new_opcode_to_old_map + } + + fn new_locations( &self, old_location: OpcodeLocation, ) -> impl Iterator + '_ { diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 3ae5c193e39..9307379a545 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -1,6 +1,7 @@ use acvm::acir::circuit::OpcodeLocation; use acvm::compiler::AcirTransformationMap; +use acvm::compiler::reverse_mapping; use serde_with::serde_as; use serde_with::DisplayFromStr; use std::collections::BTreeMap; @@ -37,15 +38,36 @@ impl DebugInfo { /// /// 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`. + /// Note: One old `OpcodeLocation` might have transformed into more than one new `OpcodeLocation`, or removed entirely. pub fn update_acir(&mut self, update_map: AcirTransformationMap) { log::trace!("Start debug info update"); let old_locations = mem::take(&mut self.locations); + // `update_map` maps as `update_map.get(new_acir_index) = old_acir_index` which is many to one. + // We want a mapping which is `reverse_mapping_tree.get(old_acir_index) = new_acir_indices`, which is one to many. + let reverse_mapping_tree = update_map.get_opcode_mapping(); + for (old_opcode_location, source_locations) in old_locations { - update_map.new_locations(old_opcode_location).for_each(|new_opcode_location| { - self.locations.insert(new_opcode_location, source_locations.clone()); - }); + let old_acir_index = match old_opcode_location { + OpcodeLocation::Acir(index) => index, + OpcodeLocation::Brillig { acir_index, .. } => acir_index, + }; + + if let Some(new_acir_indices) = reverse_mapping_tree.get(&old_acir_index) { + // Update the `OpcodeLocation`s with the new acir index and write them back into the `locations` map. + new_acir_indices + .iter() + .map(|&new_acir_index| match old_opcode_location { + OpcodeLocation::Acir(_) => OpcodeLocation::Acir(new_acir_index), + OpcodeLocation::Brillig { brillig_index, .. } => { + // brillig indices are unaffected by optimizations. + OpcodeLocation::Brillig { acir_index: new_acir_index, brillig_index } + } + }) + .for_each(|new_opcode_location| { + self.locations.insert(new_opcode_location, source_locations.clone()); + }); + } } log::trace!("Finish debug info update"); } From 065708b269418d380f03a3e898acf5e2305655c0 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 15 Dec 2023 09:29:32 +0000 Subject: [PATCH 2/5] Update compiler/noirc_errors/src/debug_info.rs --- compiler/noirc_errors/src/debug_info.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_errors/src/debug_info.rs b/compiler/noirc_errors/src/debug_info.rs index 9307379a545..fc41f432c2c 100644 --- a/compiler/noirc_errors/src/debug_info.rs +++ b/compiler/noirc_errors/src/debug_info.rs @@ -1,7 +1,6 @@ use acvm::acir::circuit::OpcodeLocation; use acvm::compiler::AcirTransformationMap; -use acvm::compiler::reverse_mapping; use serde_with::serde_as; use serde_with::DisplayFromStr; use std::collections::BTreeMap; From 1598cc7df9b4866b341ecc49418cce126689430f Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 15 Dec 2023 09:49:50 +0000 Subject: [PATCH 3/5] fix: handle empty circuits --- acvm-repo/acvm/src/compiler/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 81c3c9ff4ec..3e00d9f01f4 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -38,12 +38,17 @@ impl AcirTransformationMap { /// Returns a `BTreeMap` which maps an ACIR index in the untransformed [`Circuit`] to the set of ACIR indices /// in the new [`Circuit`] corresponding to the opcodes. pub fn get_opcode_mapping(&self) -> BTreeMap> { + if self.acir_opcode_positions.is_empty() { + return BTreeMap::new(); + }; + let mut new_opcode_to_old_map: BTreeMap> = BTreeMap::new(); let mut index = 0; let mut old_index = 0; let mut temp: Vec = Vec::new(); while old_index <= *self.acir_opcode_positions.last().unwrap() { + println!("{old_index}, {index}"); let val = self.acir_opcode_positions[index]; match old_index.cmp(&val) { std::cmp::Ordering::Less => { From 0caddcac9992dce94665494d21ec66dd24533fbf Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 15 Dec 2023 09:50:57 +0000 Subject: [PATCH 4/5] chore: move empty circuit tests to correct folder --- .../brillig_set_slice_of_slice/Nargo.toml | 0 .../brillig_set_slice_of_slice/src/main.nr | 0 .../regression_3635/Nargo.toml | 0 .../regression_3635/src/main.nr | 0 test_programs/execution_success/regression_3635/Prover.toml | 0 5 files changed, 0 insertions(+), 0 deletions(-) rename test_programs/{execution_success => compile_success_empty}/brillig_set_slice_of_slice/Nargo.toml (100%) rename test_programs/{execution_success => compile_success_empty}/brillig_set_slice_of_slice/src/main.nr (100%) rename test_programs/{execution_success => compile_success_empty}/regression_3635/Nargo.toml (100%) rename test_programs/{execution_success => compile_success_empty}/regression_3635/src/main.nr (100%) delete mode 100644 test_programs/execution_success/regression_3635/Prover.toml diff --git a/test_programs/execution_success/brillig_set_slice_of_slice/Nargo.toml b/test_programs/compile_success_empty/brillig_set_slice_of_slice/Nargo.toml similarity index 100% rename from test_programs/execution_success/brillig_set_slice_of_slice/Nargo.toml rename to test_programs/compile_success_empty/brillig_set_slice_of_slice/Nargo.toml diff --git a/test_programs/execution_success/brillig_set_slice_of_slice/src/main.nr b/test_programs/compile_success_empty/brillig_set_slice_of_slice/src/main.nr similarity index 100% rename from test_programs/execution_success/brillig_set_slice_of_slice/src/main.nr rename to test_programs/compile_success_empty/brillig_set_slice_of_slice/src/main.nr diff --git a/test_programs/execution_success/regression_3635/Nargo.toml b/test_programs/compile_success_empty/regression_3635/Nargo.toml similarity index 100% rename from test_programs/execution_success/regression_3635/Nargo.toml rename to test_programs/compile_success_empty/regression_3635/Nargo.toml diff --git a/test_programs/execution_success/regression_3635/src/main.nr b/test_programs/compile_success_empty/regression_3635/src/main.nr similarity index 100% rename from test_programs/execution_success/regression_3635/src/main.nr rename to test_programs/compile_success_empty/regression_3635/src/main.nr diff --git a/test_programs/execution_success/regression_3635/Prover.toml b/test_programs/execution_success/regression_3635/Prover.toml deleted file mode 100644 index e69de29bb2d..00000000000 From 0bf88357d376e81019492c219d30dba607bd0ef2 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 15 Dec 2023 09:51:35 +0000 Subject: [PATCH 5/5] chore: remove print --- acvm-repo/acvm/src/compiler/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 3e00d9f01f4..ab7bc8083eb 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -48,7 +48,6 @@ impl AcirTransformationMap { let mut temp: Vec = Vec::new(); while old_index <= *self.acir_opcode_positions.last().unwrap() { - println!("{old_index}, {index}"); let val = self.acir_opcode_positions[index]; match old_index.cmp(&val) { std::cmp::Ordering::Less => {