From f4745d4578a4aec526b3440707e5fdcb0452811f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 6 Jan 2025 11:24:57 +0000 Subject: [PATCH 01/11] feat(test): Enable the test fuzzer for Wasm (#6835) --- Cargo.lock | 34 +++------- Cargo.toml | 8 ++- tooling/fuzzer/src/dictionary/mod.rs | 3 + tooling/fuzzer/src/lib.rs | 18 ++--- tooling/fuzzer/src/strategies/int.rs | 12 ++-- tooling/fuzzer/src/strategies/mod.rs | 30 ++++---- tooling/fuzzer/src/strategies/uint.rs | 28 +++++--- tooling/nargo/Cargo.toml | 6 +- tooling/nargo/src/ops/test.rs | 98 ++++++++++++--------------- 9 files changed, 113 insertions(+), 124 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index eb6d4327faf..6235fbf4f8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -654,18 +654,18 @@ dependencies = [ [[package]] name = "bit-set" -version = "0.5.3" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" dependencies = [ "bit-vec", ] [[package]] name = "bit-vec" -version = "0.6.3" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" [[package]] name = "bitflags" @@ -2775,12 +2775,6 @@ version = "0.2.162" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "18d287de67fe55fd7e1581fe933d965a5a9477b38e949cfa9f8574ef01506398" -[[package]] -name = "libm" -version = "0.2.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8355be11b20d696c8f18f6cc018c4e372165b1fa8126cef092399c9951984ffa" - [[package]] name = "libredox" version = "0.0.2" @@ -3247,7 +3241,7 @@ dependencies = [ "num-bigint", "num-traits", "proptest", - "proptest-derive 0.4.0", + "proptest-derive", "serde", "serde_json", "strum", @@ -3374,7 +3368,7 @@ dependencies = [ "num-traits", "petgraph", "proptest", - "proptest-derive 0.5.0", + "proptest-derive", "rangemap", "rustc-hash 1.1.0", "serde", @@ -3486,7 +3480,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "071dfc062690e90b734c0b2273ce72ad0ffa95f0c74596bc250dcfd960262841" dependencies = [ "autocfg", - "libm", ] [[package]] @@ -3868,9 +3861,9 @@ dependencies = [ [[package]] name = "proptest" -version = "1.5.0" +version = "1.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4c2511913b88df1637da85cc8d96ec8e43a3f8bb8ccb71ee1ac240d6f3df58d" +checksum = "14cae93065090804185d3b75f0bf93b8eeda30c7a9b4a33d3bdb3988d6229e50" dependencies = [ "bit-set", "bit-vec", @@ -3886,17 +3879,6 @@ dependencies = [ "unarray", ] -[[package]] -name = "proptest-derive" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9cf16337405ca084e9c78985114633b6827711d22b9e6ef6c6c0d665eb3f0b6e" -dependencies = [ - "proc-macro2", - "quote", - "syn 1.0.109", -] - [[package]] name = "proptest-derive" version = "0.5.0" diff --git a/Cargo.toml b/Cargo.toml index 73937667c14..8d48a846457 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -152,8 +152,12 @@ jsonrpsee = { version = "0.24.7", features = ["client-core"] } flate2 = "1.0.24" color-eyre = "0.6.2" rand = "0.8.5" -proptest = "1.2.0" -proptest-derive = "0.4.0" +# The `fork` and `timeout` feature doesn't compile with Wasm (wait-timeout doesn't find the `imp` module). +proptest = { version = "1.6.0", default-features = false, features = [ + "std", + "bit-set", +] } +proptest-derive = "0.5.0" rayon = "1.8.0" sha2 = { version = "0.10.6", features = ["compress"] } sha3 = "0.10.6" diff --git a/tooling/fuzzer/src/dictionary/mod.rs b/tooling/fuzzer/src/dictionary/mod.rs index 172edfa54c2..7fe1c4cd602 100644 --- a/tooling/fuzzer/src/dictionary/mod.rs +++ b/tooling/fuzzer/src/dictionary/mod.rs @@ -33,9 +33,11 @@ pub(super) fn build_dictionary_from_program(program: &Program) constants } +/// Collect `Field` values used in the opcodes of an ACIR circuit. fn build_dictionary_from_circuit(circuit: &Circuit) -> HashSet { let mut constants: HashSet = HashSet::new(); + /// Pull out all the fields from an expression. fn insert_expr(dictionary: &mut HashSet, expr: &Expression) { let quad_coefficients = expr.mul_terms.iter().map(|(k, _, _)| *k); let linear_coefficients = expr.linear_combinations.iter().map(|(k, _)| *k); @@ -104,6 +106,7 @@ fn build_dictionary_from_circuit(circuit: &Circuit) -> HashSet< constants } +/// Collect `Field` values used in the opcodes of a Brillig function. fn build_dictionary_from_unconstrained_function( function: &BrilligBytecode, ) -> HashSet { diff --git a/tooling/fuzzer/src/lib.rs b/tooling/fuzzer/src/lib.rs index 28a43279c95..324be323fc2 100644 --- a/tooling/fuzzer/src/lib.rs +++ b/tooling/fuzzer/src/lib.rs @@ -22,7 +22,7 @@ use types::{CaseOutcome, CounterExampleOutcome, FuzzOutcome, FuzzTestResult}; use noirc_artifacts::program::ProgramArtifact; -/// An executor for Noir programs which which provides fuzzing support using [`proptest`]. +/// An executor for Noir programs which provides fuzzing support using [`proptest`]. /// /// After instantiation, calling `fuzz` will proceed to hammer the program with /// inputs, until it finds a counterexample. The provided [`TestRunner`] contains all the @@ -38,14 +38,14 @@ pub struct FuzzedExecutor { runner: TestRunner, } -impl< - E: Fn( - &Program, - WitnessMap, - ) -> Result, String>, - > FuzzedExecutor +impl FuzzedExecutor +where + E: Fn( + &Program, + WitnessMap, + ) -> Result, String>, { - /// Instantiates a fuzzed executor given a testrunner + /// Instantiates a fuzzed executor given a [TestRunner]. pub fn new(program: ProgramArtifact, executor: E, runner: TestRunner) -> Self { Self { program, executor, runner } } @@ -53,7 +53,7 @@ impl< /// Fuzzes the provided program. pub fn fuzz(&self) -> FuzzTestResult { let dictionary = build_dictionary_from_program(&self.program.bytecode); - let strategy = strategies::arb_input_map(&self.program.abi, dictionary); + let strategy = strategies::arb_input_map(&self.program.abi, &dictionary); let run_result: Result<(), TestError> = self.runner.clone().run(&strategy, |input_map| { diff --git a/tooling/fuzzer/src/strategies/int.rs b/tooling/fuzzer/src/strategies/int.rs index d11cafcfae5..22dded7c7b7 100644 --- a/tooling/fuzzer/src/strategies/int.rs +++ b/tooling/fuzzer/src/strategies/int.rs @@ -4,6 +4,8 @@ use proptest::{ }; use rand::Rng; +type BinarySearch = proptest::num::i128::BinarySearch; + /// Strategy for signed ints (up to i128). /// The strategy combines 2 different strategies, each assigned a specific weight: /// 1. Generate purely random value in a range. This will first choose bit size uniformly (up `bits` @@ -27,6 +29,7 @@ impl IntStrategy { Self { bits, edge_weight: 10usize, random_weight: 50usize } } + /// Generate random values near MIN or the MAX value. fn generate_edge_tree(&self, runner: &mut TestRunner) -> NewTree { let rng = runner.rng(); @@ -40,16 +43,16 @@ impl IntStrategy { 3 => self.type_max() - offset, _ => unreachable!(), }; - Ok(proptest::num::i128::BinarySearch::new(start)) + Ok(BinarySearch::new(start)) } fn generate_random_tree(&self, runner: &mut TestRunner) -> NewTree { let rng = runner.rng(); - let start: i128 = rng.gen_range(self.type_min()..=self.type_max()); - Ok(proptest::num::i128::BinarySearch::new(start)) + Ok(BinarySearch::new(start)) } + /// Maximum allowed positive number. fn type_max(&self) -> i128 { if self.bits < 128 { (1i128 << (self.bits - 1)) - 1 @@ -58,6 +61,7 @@ impl IntStrategy { } } + /// Minimum allowed negative number. fn type_min(&self) -> i128 { if self.bits < 128 { -(1i128 << (self.bits - 1)) @@ -68,7 +72,7 @@ impl IntStrategy { } impl Strategy for IntStrategy { - type Tree = proptest::num::i128::BinarySearch; + type Tree = BinarySearch; type Value = i128; fn new_tree(&self, runner: &mut TestRunner) -> NewTree { diff --git a/tooling/fuzzer/src/strategies/mod.rs b/tooling/fuzzer/src/strategies/mod.rs index 46187a28d5b..99c7ca56f2e 100644 --- a/tooling/fuzzer/src/strategies/mod.rs +++ b/tooling/fuzzer/src/strategies/mod.rs @@ -11,22 +11,28 @@ use uint::UintStrategy; mod int; mod uint; +/// Create a strategy for generating random values for an [AbiType]. +/// +/// Uses the `dictionary` for unsigned integer types. pub(super) fn arb_value_from_abi_type( abi_type: &AbiType, - dictionary: HashSet, + dictionary: &HashSet, ) -> SBoxedStrategy { match abi_type { AbiType::Field => vec(any::(), 32) .prop_map(|bytes| InputValue::Field(FieldElement::from_be_bytes_reduce(&bytes))) .sboxed(), AbiType::Integer { width, sign } if sign == &Sign::Unsigned => { - UintStrategy::new(*width as usize, dictionary) + // We've restricted the type system to only allow u64s as the maximum integer type. + let width = (*width).min(64); + UintStrategy::new(width as usize, dictionary) .prop_map(|uint| InputValue::Field(uint.into())) .sboxed() } AbiType::Integer { width, .. } => { - let shift = 2i128.pow(*width); - IntStrategy::new(*width as usize) + let width = (*width).min(64); + let shift = 2i128.pow(width); + IntStrategy::new(width as usize) .prop_map(move |mut int| { if int < 0 { int += shift @@ -38,7 +44,6 @@ pub(super) fn arb_value_from_abi_type( AbiType::Boolean => { any::().prop_map(|val| InputValue::Field(FieldElement::from(val))).sboxed() } - AbiType::String { length } => { // Strings only allow ASCII characters as each character must be able to be represented by a single byte. let string_regex = format!("[[:ascii:]]{{{length}}}"); @@ -53,12 +58,11 @@ pub(super) fn arb_value_from_abi_type( elements.prop_map(InputValue::Vec).sboxed() } - AbiType::Struct { fields, .. } => { let fields: Vec> = fields .iter() .map(|(name, typ)| { - (Just(name.clone()), arb_value_from_abi_type(typ, dictionary.clone())).sboxed() + (Just(name.clone()), arb_value_from_abi_type(typ, dictionary)).sboxed() }) .collect(); @@ -69,25 +73,25 @@ pub(super) fn arb_value_from_abi_type( }) .sboxed() } - AbiType::Tuple { fields } => { let fields: Vec<_> = - fields.iter().map(|typ| arb_value_from_abi_type(typ, dictionary.clone())).collect(); + fields.iter().map(|typ| arb_value_from_abi_type(typ, dictionary)).collect(); fields.prop_map(InputValue::Vec).sboxed() } } } +/// Given the [Abi] description of a [ProgramArtifact], generate random [InputValue]s for each circuit parameter. +/// +/// Use the `dictionary` to draw values from for numeric types. pub(super) fn arb_input_map( abi: &Abi, - dictionary: HashSet, + dictionary: &HashSet, ) -> BoxedStrategy { let values: Vec<_> = abi .parameters .iter() - .map(|param| { - (Just(param.name.clone()), arb_value_from_abi_type(¶m.typ, dictionary.clone())) - }) + .map(|param| (Just(param.name.clone()), arb_value_from_abi_type(¶m.typ, dictionary))) .collect(); values diff --git a/tooling/fuzzer/src/strategies/uint.rs b/tooling/fuzzer/src/strategies/uint.rs index 94610dbc829..402e6559752 100644 --- a/tooling/fuzzer/src/strategies/uint.rs +++ b/tooling/fuzzer/src/strategies/uint.rs @@ -7,6 +7,8 @@ use proptest::{ }; use rand::Rng; +type BinarySearch = proptest::num::u128::BinarySearch; + /// Value tree for unsigned ints (up to u128). /// The strategy combines 2 different strategies, each assigned a specific weight: /// 1. Generate purely random value in a range. This will first choose bit size uniformly (up `bits` @@ -14,7 +16,7 @@ use rand::Rng; /// 2. Generate a random value around the edges (+/- 3 around 0 and max possible value) #[derive(Debug)] pub struct UintStrategy { - /// Bit size of uint (e.g. 128) + /// Bit size of uint (e.g. 64) bits: usize, /// A set of fixtures to be generated fixtures: Vec, @@ -31,25 +33,29 @@ impl UintStrategy { /// # Arguments /// * `bits` - Size of uint in bits /// * `fixtures` - Set of `FieldElements` representing values which the fuzzer weight towards testing. - pub fn new(bits: usize, fixtures: HashSet) -> Self { + pub fn new(bits: usize, fixtures: &HashSet) -> Self { Self { bits, - fixtures: fixtures.into_iter().collect(), + // We can only consider the fixtures which fit into the bit width. + fixtures: fixtures.iter().filter(|f| f.num_bits() <= bits as u32).copied().collect(), edge_weight: 10usize, fixtures_weight: 40usize, random_weight: 50usize, } } + /// Generate random numbers starting from near 0 or the maximum of the range. fn generate_edge_tree(&self, runner: &mut TestRunner) -> NewTree { let rng = runner.rng(); // Choose if we want values around 0 or max let is_min = rng.gen_bool(0.5); let offset = rng.gen_range(0..4); let start = if is_min { offset } else { self.type_max().saturating_sub(offset) }; - Ok(proptest::num::u128::BinarySearch::new(start)) + Ok(BinarySearch::new(start)) } + /// Pick a random `FieldElement` from the `fixtures` as a starting point for + /// generating random numbers. fn generate_fixtures_tree(&self, runner: &mut TestRunner) -> NewTree { // generate random cases if there's no fixtures if self.fixtures.is_empty() { @@ -58,21 +64,19 @@ impl UintStrategy { // Generate value tree from fixture. let fixture = &self.fixtures[runner.rng().gen_range(0..self.fixtures.len())]; - if fixture.num_bits() <= self.bits as u32 { - return Ok(proptest::num::u128::BinarySearch::new(fixture.to_u128())); - } - // If fixture is not a valid type, generate random value. - self.generate_random_tree(runner) + Ok(BinarySearch::new(fixture.to_u128())) } + /// Generate random values between 0 and the MAX with the given bit width. fn generate_random_tree(&self, runner: &mut TestRunner) -> NewTree { let rng = runner.rng(); let start = rng.gen_range(0..=self.type_max()); - Ok(proptest::num::u128::BinarySearch::new(start)) + Ok(BinarySearch::new(start)) } + /// Maximum integer that fits in the given bit width. fn type_max(&self) -> u128 { if self.bits < 128 { (1 << self.bits) - 1 @@ -83,8 +87,10 @@ impl UintStrategy { } impl Strategy for UintStrategy { - type Tree = proptest::num::u128::BinarySearch; + type Tree = BinarySearch; type Value = u128; + + /// Pick randomly from the 3 available strategies for generating unsigned integers. fn new_tree(&self, runner: &mut TestRunner) -> NewTree { let total_weight = self.random_weight + self.fixtures_weight + self.edge_weight; let bias = runner.rng().gen_range(0..total_weight); diff --git a/tooling/nargo/Cargo.toml b/tooling/nargo/Cargo.toml index 9fb46b78bc9..4587638d693 100644 --- a/tooling/nargo/Cargo.toml +++ b/tooling/nargo/Cargo.toml @@ -28,15 +28,13 @@ tracing.workspace = true serde.workspace = true serde_json.workspace = true walkdir = "2.5.0" +noir_fuzzer = { workspace = true } +proptest = { workspace = true } # Some dependencies are optional so we can compile to Wasm. tokio = { workspace = true, optional = true } rand = { workspace = true, optional = true } -[target.'cfg(not(target_arch = "wasm32"))'.dependencies] -noir_fuzzer = { workspace = true } -proptest = { workspace = true } - [dev-dependencies] jsonrpsee = { workspace = true, features = ["server"] } diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index bfd5cd3713f..a2f94cd61eb 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -96,70 +96,58 @@ where status } } else { - #[cfg(target_arch = "wasm32")] - { - // We currently don't support fuzz testing on wasm32 as the u128 strategies do not exist on this platform. - TestStatus::Fail { - message: "Fuzz tests are not supported on wasm32".to_string(), - error_diagnostic: None, - } - } + use acvm::acir::circuit::Program; + use noir_fuzzer::FuzzedExecutor; + use proptest::test_runner::Config; + use proptest::test_runner::TestRunner; - #[cfg(not(target_arch = "wasm32"))] - { - use acvm::acir::circuit::Program; - use noir_fuzzer::FuzzedExecutor; - use proptest::test_runner::Config; - use proptest::test_runner::TestRunner; + let runner = + TestRunner::new(Config { failure_persistence: None, ..Config::default() }); - let runner = - TestRunner::new(Config { failure_persistence: None, ..Config::default() }); + let abi = compiled_program.abi.clone(); + let debug = compiled_program.debug.clone(); - let abi = compiled_program.abi.clone(); - let debug = compiled_program.debug.clone(); + let executor = |program: &Program, + initial_witness: WitnessMap| + -> Result, String> { + // Use a base layer that doesn't handle anything, which we handle in the `execute` below. + let inner_executor = + build_foreign_call_executor(PrintOutput::None, layers::Unhandled); - let executor = - |program: &Program, - initial_witness: WitnessMap| - -> Result, String> { - // Use a base layer that doesn't handle anything, which we handle in the `execute` below. - let inner_executor = - build_foreign_call_executor(PrintOutput::None, layers::Unhandled); - let mut foreign_call_executor = - TestForeignCallExecutor::new(inner_executor); + let mut foreign_call_executor = TestForeignCallExecutor::new(inner_executor); - let circuit_execution = execute_program( - program, - initial_witness, - blackbox_solver, - &mut foreign_call_executor, - ); + let circuit_execution = execute_program( + program, + initial_witness, + blackbox_solver, + &mut foreign_call_executor, + ); - let status = test_status_program_compile_pass( - test_function, - &abi, - &debug, - &circuit_execution, - ); + // Check if a failure was actually expected. + let status = test_status_program_compile_pass( + test_function, + &abi, + &debug, + &circuit_execution, + ); - if let TestStatus::Fail { message, error_diagnostic: _ } = status { - Err(message) - } else { - // The fuzzer doesn't care about the actual result. - Ok(WitnessStack::default()) - } - }; + if let TestStatus::Fail { message, error_diagnostic: _ } = status { + Err(message) + } else { + // The fuzzer doesn't care about the actual result. + Ok(WitnessStack::default()) + } + }; - let fuzzer = FuzzedExecutor::new(compiled_program.into(), executor, runner); + let fuzzer = FuzzedExecutor::new(compiled_program.into(), executor, runner); - let result = fuzzer.fuzz(); - if result.success { - TestStatus::Pass - } else { - TestStatus::Fail { - message: result.reason.unwrap_or_default(), - error_diagnostic: None, - } + let result = fuzzer.fuzz(); + if result.success { + TestStatus::Pass + } else { + TestStatus::Fail { + message: result.reason.unwrap_or_default(), + error_diagnostic: None, } } } From fd29c2f835fd7ce4bbf3face58ba31e783d4a0e3 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 6 Jan 2025 12:59:07 +0000 Subject: [PATCH 02/11] chore: bump `noir-gates-diff` (#6949) --- .github/workflows/reports.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/reports.yml b/.github/workflows/reports.yml index 9ab66c4c16e..6c93156eddd 100644 --- a/.github/workflows/reports.yml +++ b/.github/workflows/reports.yml @@ -76,7 +76,7 @@ jobs: - name: Compare gates reports id: gates_diff - uses: noir-lang/noir-gates-diff@60c57a6e3b67bf1553a5774ded82920150f2210e + uses: noir-lang/noir-gates-diff@7e4ddaa91c69380f15ccba514eac17bc7432a8cc with: report: gates_report.json summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%) @@ -121,7 +121,7 @@ jobs: - name: Compare Brillig bytecode size reports id: brillig_bytecode_diff - uses: noir-lang/noir-gates-diff@60c57a6e3b67bf1553a5774ded82920150f2210e + uses: noir-lang/noir-gates-diff@7e4ddaa91c69380f15ccba514eac17bc7432a8cc with: report: gates_report_brillig.json header: | From bbdf1a2935f3c23b0ecf5e359626975471a31701 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 6 Jan 2025 13:57:35 +0000 Subject: [PATCH 03/11] chore: add reproduction case for bignum test failure (#6464) --- .../regression_bignum/Nargo.toml | 7 +++ .../regression_bignum/src/main.nr | 51 +++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 test_programs/execution_success/regression_bignum/Nargo.toml create mode 100644 test_programs/execution_success/regression_bignum/src/main.nr diff --git a/test_programs/execution_success/regression_bignum/Nargo.toml b/test_programs/execution_success/regression_bignum/Nargo.toml new file mode 100644 index 00000000000..5cadd364e43 --- /dev/null +++ b/test_programs/execution_success/regression_bignum/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_bignum" +version = "0.1.0" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/execution_success/regression_bignum/src/main.nr b/test_programs/execution_success/regression_bignum/src/main.nr new file mode 100644 index 00000000000..013ab6b2023 --- /dev/null +++ b/test_programs/execution_success/regression_bignum/src/main.nr @@ -0,0 +1,51 @@ +fn main() { + let numerator = + [790096867046896348, 1063071665130103641, 602707730209562162, 996751591622961462, 28650, 0]; + unsafe { __udiv_mod(numerator) }; + + let denominator = [12, 0, 0, 0, 0, 0]; + let result = unsafe { __validate_gt_remainder(denominator) }; + assert(result[4] == 0); + assert(result[5] == 0); +} + +unconstrained fn __udiv_mod(remainder_u60: [u64; 6]) { + let bit_difference = get_msb(remainder_u60); + let accumulator_u60: [u64; 6] = shl(bit_difference); +} + +unconstrained fn __validate_gt_remainder(a_u60: [u64; 6]) -> [u64; 6] { + let mut addend_u60: [u64; 6] = [0; 6]; + let mut result_u60: [u64; 6] = [0; 6]; + + for i in 0..6 { + result_u60[i] = a_u60[i] + addend_u60[i]; + } + + result_u60 +} + +unconstrained fn get_msb(val: [u64; 6]) -> u32 { + let mut count = 0; + for i in 0..6 { + let v = val[(6 - 1) - i]; + if (v > 0) { + count = 60 * ((6 - 1) - i); + break; + } + } + count +} + +unconstrained fn shl(shift: u32) -> [u64; 6] { + let num_shifted_limbs = shift / 60; + let limb_shift = (shift % 60) as u8; + + let mut result = [0; 6]; + result[num_shifted_limbs] = (1 << limb_shift); + + for i in 1..(6 - num_shifted_limbs) { + result[i + num_shifted_limbs] = 0; + } + result +} From da94c2b45fd43cbd6a5f2ba15b665dcb3a5e33a5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 6 Jan 2025 11:51:55 -0300 Subject: [PATCH 04/11] feat: don't simplify SSA instructions when creating them from a string (#6948) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/ssa/function_builder/mod.rs | 26 ++++++++++++++----- .../src/ssa/ir/instruction/call.rs | 2 +- .../src/ssa/ir/instruction/call/blackbox.rs | 6 ++--- .../src/ssa/opt/constant_folding.rs | 2 +- .../src/ssa/opt/flatten_cfg.rs | 1 + .../src/ssa/opt/normalize_value_ids.rs | 13 +++++----- .../src/ssa/parser/into_ssa.rs | 11 ++++---- .../noirc_evaluator/src/ssa/parser/mod.rs | 18 +++++++++---- .../noirc_evaluator/src/ssa/parser/tests.rs | 12 +++++++++ 9 files changed, 64 insertions(+), 27 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 28cf48bb171..2be8e571cd3 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -40,6 +40,10 @@ pub(crate) struct FunctionBuilder { finished_functions: Vec, call_stack: CallStackId, error_types: BTreeMap, + + /// Whether instructions are simplified as soon as they are inserted into this builder. + /// This is true by default unless changed to false after constructing a builder. + pub(crate) simplify: bool, } impl FunctionBuilder { @@ -56,6 +60,7 @@ impl FunctionBuilder { finished_functions: Vec::new(), call_stack: CallStackId::root(), error_types: BTreeMap::default(), + simplify: true, } } @@ -172,12 +177,21 @@ impl FunctionBuilder { ctrl_typevars: Option>, ) -> InsertInstructionResult { let block = self.current_block(); - self.current_function.dfg.insert_instruction_and_results( - instruction, - block, - ctrl_typevars, - self.call_stack, - ) + if self.simplify { + self.current_function.dfg.insert_instruction_and_results( + instruction, + block, + ctrl_typevars, + self.call_stack, + ) + } else { + self.current_function.dfg.insert_instruction_and_results_without_simplification( + instruction, + block, + ctrl_typevars, + self.call_stack, + ) + } } /// Switch to inserting instructions in the given block. diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index b6b1ac3f063..b0e5506e8dd 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -730,7 +730,7 @@ mod tests { return v2 } "#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); let expected = r#" brillig(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs index ffacf6fe8b5..fac2e8b4d5a 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs @@ -323,7 +323,7 @@ mod test { v2 = call multi_scalar_mul (v1, v0) -> [Field; 3] return v2 }"#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); let expected_src = r#" acir(inline) fn main f0 { @@ -351,7 +351,7 @@ mod test { return v4 }"#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); //First point is zero, second scalar is zero, so we should be left with the scalar mul of the last point. let expected_src = r#" acir(inline) fn main f0 { @@ -379,7 +379,7 @@ mod test { v4 = call multi_scalar_mul (v3, v2) -> [Field; 3] return v4 }"#; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); //First and last scalar/point are constant, so we should be left with the msm of the middle point and the folded constant point let expected_src = r#" acir(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index c81a557178b..e3b8c6e99aa 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1045,7 +1045,7 @@ mod test { return } "; - let ssa = Ssa::from_str(src).unwrap(); + let ssa = Ssa::from_str_simplifying(src).unwrap(); let expected = " acir(inline) fn main f0 { diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 16abf03eec0..748867c7409 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -1081,6 +1081,7 @@ mod test { v23 = mul v20, Field 6 v24 = mul v21, v16 v25 = add v23, v24 + enable_side_effects v0 enable_side_effects v3 v26 = cast v3 as Field v27 = cast v0 as Field diff --git a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs index 63ca523bd57..7f5352155de 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -96,12 +96,13 @@ impl Context { .requires_ctrl_typevars() .then(|| vecmap(old_results, |result| old_function.dfg.type_of_value(*result))); - let new_results = new_function.dfg.insert_instruction_and_results( - instruction, - new_block_id, - ctrl_typevars, - new_call_stack, - ); + let new_results = + new_function.dfg.insert_instruction_and_results_without_simplification( + instruction, + new_block_id, + ctrl_typevars, + new_call_stack, + ); assert_eq!(old_results.len(), new_results.len()); for (old_result, new_result) in old_results.iter().zip(new_results.results().iter()) diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index 7c7e977c6ce..afada8c0e2c 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -16,8 +16,8 @@ use super::{ }; impl ParsedSsa { - pub(crate) fn into_ssa(self) -> Result { - Translator::translate(self) + pub(crate) fn into_ssa(self, simplify: bool) -> Result { + Translator::translate(self, simplify) } } @@ -41,8 +41,8 @@ struct Translator { } impl Translator { - fn translate(mut parsed_ssa: ParsedSsa) -> Result { - let mut translator = Self::new(&mut parsed_ssa)?; + fn translate(mut parsed_ssa: ParsedSsa, simplify: bool) -> Result { + let mut translator = Self::new(&mut parsed_ssa, simplify)?; // Note that the `new` call above removed the main function, // so all we are left with are non-main functions. @@ -53,12 +53,13 @@ impl Translator { Ok(translator.finish()) } - fn new(parsed_ssa: &mut ParsedSsa) -> Result { + fn new(parsed_ssa: &mut ParsedSsa, simplify: bool) -> Result { // A FunctionBuilder must be created with a main Function, so here wer remove it // from the parsed SSA to avoid adding it twice later on. let main_function = parsed_ssa.functions.remove(0); let main_id = FunctionId::test_new(0); let mut builder = FunctionBuilder::new(main_function.external_name.clone(), main_id); + builder.simplify = simplify; builder.set_runtime(main_function.runtime_type); // Map function names to their IDs so calls can be resolved diff --git a/compiler/noirc_evaluator/src/ssa/parser/mod.rs b/compiler/noirc_evaluator/src/ssa/parser/mod.rs index b400c28875a..96aef1f3c32 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/mod.rs @@ -32,16 +32,24 @@ mod token; impl Ssa { /// Creates an Ssa object from the given string. - /// - /// Note that the resulting Ssa might not be exactly the same as the given string. - /// This is because, internally, the Ssa is built using a `FunctionBuilder`, so - /// some instructions might be simplified while they are inserted. pub(crate) fn from_str(src: &str) -> Result { + Self::from_str_impl(src, false) + } + + /// Creates an Ssa object from the given string but trying to simplify + /// each parsed instruction as it's inserted into the final SSA. + pub(crate) fn from_str_simplifying(src: &str) -> Result { + Self::from_str_impl(src, true) + } + + fn from_str_impl(src: &str, simplify: bool) -> Result { let mut parser = Parser::new(src).map_err(|err| SsaErrorWithSource::parse_error(err, src))?; let parsed_ssa = parser.parse_ssa().map_err(|err| SsaErrorWithSource::parse_error(err, src))?; - parsed_ssa.into_ssa().map_err(|error| SsaErrorWithSource { src: src.to_string(), error }) + parsed_ssa + .into_ssa(simplify) + .map_err(|error| SsaErrorWithSource { src: src.to_string(), error }) } } diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index dab96dfa04f..899a5a571e7 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -502,3 +502,15 @@ fn test_function_type() { "; assert_ssa_roundtrip(src); } + +#[test] +fn test_does_not_simplify() { + let src = " + acir(inline) fn main f0 { + b0(): + v2 = add Field 1, Field 2 + return v2 + } + "; + assert_ssa_roundtrip(src); +} From 54d81ca7236b2d418cd87d8c26e87f4790ab3d78 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 6 Jan 2025 13:08:05 -0300 Subject: [PATCH 05/11] feat: lock on Nargo.toml on several nargo commands (#6941) --- Cargo.lock | 22 +++++++-------- Cargo.toml | 2 -- tooling/nargo_cli/Cargo.toml | 4 +-- tooling/nargo_cli/build.rs | 23 --------------- tooling/nargo_cli/src/cli/mod.rs | 48 ++++++++++++++++++++++++++++++++ 5 files changed, 61 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6235fbf4f8f..050c70a0f0b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1697,16 +1697,6 @@ dependencies = [ "windows-sys 0.52.0", ] -[[package]] -name = "file-lock" -version = "2.1.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "040b48f80a749da50292d0f47a1e2d5bf1d772f52836c07f64bfccc62ba6e664" -dependencies = [ - "cc", - "libc", -] - [[package]] name = "filetime" version = "0.2.25" @@ -1780,6 +1770,16 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fs2" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "fsevent-sys" version = "4.1.0" @@ -2999,8 +2999,8 @@ dependencies = [ "criterion", "dap", "dirs", - "file-lock", "fm", + "fs2", "fxhash", "iai", "iter-extended", diff --git a/Cargo.toml b/Cargo.toml index 8d48a846457..567f1db085b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -83,8 +83,6 @@ noir_lsp = { path = "tooling/lsp" } noir_debugger = { path = "tooling/debugger" } noirc_abi = { path = "tooling/noirc_abi" } noirc_artifacts = { path = "tooling/noirc_artifacts" } -bb_abstraction_leaks = { path = "tooling/bb_abstraction_leaks" } -acvm_cli = { path = "tooling/acvm_cli" } # Arkworks ark-bn254 = { version = "^0.5.0", default-features = false, features = [ diff --git a/tooling/nargo_cli/Cargo.toml b/tooling/nargo_cli/Cargo.toml index 742c397a0d3..001306bb162 100644 --- a/tooling/nargo_cli/Cargo.toml +++ b/tooling/nargo_cli/Cargo.toml @@ -75,6 +75,7 @@ tracing.workspace = true tracing-subscriber.workspace = true tracing-appender = "0.2.3" clap_complete = "4.5.36" +fs2 = "0.4.3" [target.'cfg(not(unix))'.dependencies] tokio-util = { version = "0.7.8", features = ["compat"] } @@ -86,7 +87,6 @@ dirs.workspace = true assert_cmd = "2.0.8" assert_fs = "1.0.10" predicates = "2.1.5" -file-lock = "2.1.11" fm.workspace = true criterion.workspace = true pprof.workspace = true @@ -102,7 +102,7 @@ light-poseidon = "0.2.0" ark-bn254-v04 = { package = "ark-bn254", version = "^0.4.0", default-features = false, features = [ "curve", ] } -ark-ff-v04 = { package = "ark-ff", version = "^0.4.0", default-features = false } +ark-ff-v04 = { package = "ark-ff", version = "^0.4.0", default-features = false } [[bench]] name = "criterion" diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index d041cf3e53f..04cc463f6b3 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -177,33 +177,13 @@ fn generate_test_cases( } let test_cases = test_cases.join("\n"); - // We need to isolate test cases in the same group, otherwise they overwrite each other's artifacts. - // On CI we use `cargo nextest`, which runs tests in different processes; for this we use a file lock. - // Locally we might be using `cargo test`, which run tests in the same process; in this case the file lock - // wouldn't work, becuase the process itself has the lock, and it looks like it can have N instances without - // any problems; for this reason we also use a `Mutex`. - let mutex_name = format! {"TEST_MUTEX_{}", test_name.to_uppercase()}; write!( test_file, r#" -lazy_static::lazy_static! {{ - /// Prevent concurrent tests in the matrix from overwriting the compilation artifacts in {test_dir} - static ref {mutex_name}: std::sync::Mutex<()> = std::sync::Mutex::new(()); -}} - {test_cases} fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner) {{ let test_program_dir = PathBuf::from("{test_dir}"); - // Ignore poisoning errors if some of the matrix cases failed. - let mutex_guard = {mutex_name}.lock().unwrap_or_else(|e| e.into_inner()); - - let file_guard = file_lock::FileLock::lock( - test_program_dir.join("Nargo.toml"), - true, - file_lock::FileOptions::new().read(true).write(true).append(true) - ).expect("failed to lock Nargo.toml"); - let mut nargo = Command::cargo_bin("nargo").unwrap(); nargo.arg("--program-dir").arg(test_program_dir); nargo.arg("{test_command}").arg("--force"); @@ -221,9 +201,6 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner }} {test_content} - - drop(file_guard); - drop(mutex_guard); }} "# ) diff --git a/tooling/nargo_cli/src/cli/mod.rs b/tooling/nargo_cli/src/cli/mod.rs index 43e3de9c6d0..0b725afcf4e 100644 --- a/tooling/nargo_cli/src/cli/mod.rs +++ b/tooling/nargo_cli/src/cli/mod.rs @@ -118,6 +118,11 @@ enum NargoCommand { #[cfg(not(feature = "codegen-docs"))] #[tracing::instrument(level = "trace")] pub(crate) fn start_cli() -> eyre::Result<()> { + use std::fs::File; + + use fs2::FileExt as _; + use nargo_toml::get_package_manifest; + let NargoCli { command, mut config } = NargoCli::parse(); // If the provided `program_dir` is relative, make it absolute by joining it to the current directory. @@ -130,6 +135,28 @@ pub(crate) fn start_cli() -> eyre::Result<()> { config.program_dir = program_dir; } + let lock_file = match needs_lock(&command) { + Some(exclusive) => { + let toml_path = get_package_manifest(&config.program_dir)?; + let file = File::open(toml_path).expect("Expected Nargo.toml to exist"); + if exclusive { + if file.try_lock_exclusive().is_err() { + eprintln!("Waiting for lock on Nargo.toml..."); + } + + file.lock_exclusive().expect("Failed to lock Nargo.toml"); + } else { + if file.try_lock_shared().is_err() { + eprintln!("Waiting for lock on Nargo.toml..."); + } + + file.lock_shared().expect("Failed to lock Nargo.toml"); + } + Some(file) + } + None => None, + }; + match command { NargoCommand::New(args) => new_cmd::run(args, config), NargoCommand::Init(args) => init_cmd::run(args, config), @@ -146,6 +173,10 @@ pub(crate) fn start_cli() -> eyre::Result<()> { NargoCommand::GenerateCompletionScript(args) => generate_completion_script_cmd::run(args), }?; + if let Some(lock_file) = lock_file { + lock_file.unlock().expect("Failed to unlock Nargo.toml"); + } + Ok(()) } @@ -193,6 +224,23 @@ fn command_dir(cmd: &NargoCommand, program_dir: &Path) -> Result Ok(Some(nargo_toml::find_root(program_dir, workspace)?)) } +fn needs_lock(cmd: &NargoCommand) -> Option { + match cmd { + NargoCommand::Check(..) + | NargoCommand::Compile(..) + | NargoCommand::Execute(..) + | NargoCommand::Export(..) + | NargoCommand::Info(..) => Some(true), + NargoCommand::Debug(..) | NargoCommand::Test(..) => Some(false), + NargoCommand::Fmt(..) + | NargoCommand::New(..) + | NargoCommand::Init(..) + | NargoCommand::Lsp(..) + | NargoCommand::Dap(..) + | NargoCommand::GenerateCompletionScript(..) => None, + } +} + #[cfg(test)] mod tests { use clap::Parser; From da18a12e32e60fb2301e747fd24505fb46d679d7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 6 Jan 2025 14:24:32 -0300 Subject: [PATCH 06/11] feat!: turn CannotReexportItemWithLessVisibility into an error (#6952) --- compiler/noirc_frontend/src/hir/def_collector/errors.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index cafbc670e32..1582e297144 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -183,7 +183,7 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { } DefCollectorErrorKind::PathResolutionError(error) => error.into(), DefCollectorErrorKind::CannotReexportItemWithLessVisibility{item_name, desired_visibility} => { - Diagnostic::simple_warning( + Diagnostic::simple_error( format!("cannot re-export {item_name} because it has less visibility than this use statement"), format!("consider marking {item_name} as {desired_visibility}"), item_name.span()) From 2e5d91f3d69ce629d07eb6041736b5a8e8ed2013 Mon Sep 17 00:00:00 2001 From: jfecher Date: Mon, 6 Jan 2025 12:55:26 -0600 Subject: [PATCH 07/11] chore: Separate unconstrained functions during monomorphization (#6894) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- Cargo.lock | 1 + compiler/noirc_driver/src/lib.rs | 12 +- compiler/noirc_evaluator/src/ssa.rs | 29 +- .../src/ssa/function_builder/mod.rs | 44 ++- .../noirc_evaluator/src/ssa/ir/printer.rs | 5 +- .../src/ssa/opt/constant_folding.rs | 4 +- compiler/noirc_evaluator/src/ssa/opt/die.rs | 17 +- compiler/noirc_evaluator/src/ssa/opt/hint.rs | 1 - compiler/noirc_evaluator/src/ssa/opt/mod.rs | 2 +- .../src/ssa/opt/normalize_value_ids.rs | 4 +- .../src/ssa/opt/remove_unreachable.rs | 80 ++++ .../src/ssa/opt/runtime_separation.rs | 351 ------------------ .../noirc_evaluator/src/ssa/parser/tests.rs | 4 +- .../src/ssa/ssa_gen/context.rs | 11 +- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 24 +- .../src/ssa/ssa_gen/program.rs | 31 +- compiler/noirc_frontend/Cargo.toml | 1 + .../src/monomorphization/mod.rs | 110 ++++-- compiler/noirc_frontend/src/tests.rs | 2 +- 19 files changed, 240 insertions(+), 493 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs delete mode 100644 compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs diff --git a/Cargo.lock b/Cargo.lock index 050c70a0f0b..889c5c1ec6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3359,6 +3359,7 @@ dependencies = [ "bn254_blackbox_solver", "cfg-if", "fm", + "fxhash", "im", "iter-extended", "noirc_arena", diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 1b311504b5c..6b852354824 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -590,10 +590,17 @@ pub fn compile_no_check( cached_program: Option, force_compile: bool, ) -> Result { + let force_unconstrained = options.force_brillig; + let program = if options.instrument_debug { - monomorphize_debug(main_function, &mut context.def_interner, &context.debug_instrumenter)? + monomorphize_debug( + main_function, + &mut context.def_interner, + &context.debug_instrumenter, + force_unconstrained, + )? } else { - monomorphize(main_function, &mut context.def_interner)? + monomorphize(main_function, &mut context.def_interner, force_unconstrained)? }; if options.show_monomorphized { @@ -632,7 +639,6 @@ pub fn compile_no_check( } }, enable_brillig_logging: options.show_brillig, - force_brillig_output: options.force_brillig, print_codegen_timings: options.benchmark_codegen, expression_width: if options.bounded_codegen { options.expression_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 8d22e8e628d..2500b8685a1 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -56,9 +56,6 @@ pub struct SsaEvaluatorOptions { pub enable_brillig_logging: bool, - /// Force Brillig output (for step debugging) - pub force_brillig_output: bool, - /// Pretty print benchmark times of each code generation pass pub print_codegen_timings: bool, @@ -99,7 +96,6 @@ pub(crate) fn optimize_into_acir( let builder = SsaBuilder::new( program, options.ssa_logging.clone(), - options.force_brillig_output, options.print_codegen_timings, &options.emit_ssa, )?; @@ -154,15 +150,16 @@ pub(crate) fn optimize_into_acir( /// Run all SSA passes. fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result { Ok(builder + .run_pass(Ssa::remove_unreachable_functions, "Removing Unreachable Functions") .run_pass(Ssa::defunctionalize, "Defunctionalization") .run_pass(Ssa::remove_paired_rc, "Removing Paired rc_inc & rc_decs") - .run_pass(Ssa::separate_runtime, "Runtime Separation") .run_pass(Ssa::resolve_is_unconstrained, "Resolving IsUnconstrained") .run_pass(|ssa| ssa.inline_functions(options.inliner_aggressiveness), "Inlining (1st)") // Run mem2reg with the CFG separated into blocks .run_pass(Ssa::mem2reg, "Mem2Reg (1st)") .run_pass(Ssa::simplify_cfg, "Simplifying (1st)") .run_pass(Ssa::as_slice_optimization, "`as_slice` optimization") + .run_pass(Ssa::remove_unreachable_functions, "Removing Unreachable Functions") .try_run_pass( Ssa::evaluate_static_assert_and_assert_constant, "`static_assert` and `assert_constant`", @@ -275,19 +272,12 @@ pub fn create_program( (generated_acirs, generated_brillig, brillig_function_names, error_types), ssa_level_warnings, ) = optimize_into_acir(program, options)?; - if options.force_brillig_output { - assert_eq!( - generated_acirs.len(), - 1, - "Only the main ACIR is expected when forcing Brillig output" - ); - } else { - assert_eq!( - generated_acirs.len(), - func_sigs.len(), - "The generated ACIRs should match the supplied function signatures" - ); - } + + assert_eq!( + generated_acirs.len(), + func_sigs.len(), + "The generated ACIRs should match the supplied function signatures" + ); let error_types = error_types .into_iter() @@ -450,11 +440,10 @@ impl SsaBuilder { fn new( program: Program, ssa_logging: SsaLogging, - force_brillig_runtime: bool, print_codegen_timings: bool, emit_ssa: &Option, ) -> Result { - let ssa = ssa_gen::generate_ssa(program, force_brillig_runtime)?; + let ssa = ssa_gen::generate_ssa(program)?; if let Some(emit_ssa) = emit_ssa { let mut emit_ssa_dir = emit_ssa.clone(); // We expect the full package artifact path to be passed in here, diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 2be8e571cd3..d08d5339237 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -483,29 +483,33 @@ impl FunctionBuilder { /// /// Returns whether a reference count instruction was issued. fn update_array_reference_count(&mut self, value: ValueId, increment: bool) -> bool { - match self.type_of_value(value) { - Type::Numeric(_) => false, - Type::Function => false, - Type::Reference(element) => { - if element.contains_an_array() { - let reference = value; - let value = self.insert_load(reference, element.as_ref().clone()); - self.update_array_reference_count(value, increment); - true - } else { - false + if self.current_function.runtime().is_brillig() { + match self.type_of_value(value) { + Type::Numeric(_) => false, + Type::Function => false, + Type::Reference(element) => { + if element.contains_an_array() { + let reference = value; + let value = self.insert_load(reference, element.as_ref().clone()); + self.update_array_reference_count(value, increment); + true + } else { + false + } } - } - Type::Array(..) | Type::Slice(..) => { - // If there are nested arrays or slices, we wait until ArrayGet - // is issued to increment the count of that array. - if increment { - self.insert_inc_rc(value); - } else { - self.insert_dec_rc(value); + Type::Array(..) | Type::Slice(..) => { + // If there are nested arrays or slices, we wait until ArrayGet + // is issued to increment the count of that array. + if increment { + self.insert_inc_rc(value); + } else { + self.insert_dec_rc(value); + } + true } - true } + } else { + false } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 322639a03d2..598f7c27eff 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -51,9 +51,8 @@ fn value(function: &Function, id: ValueId) -> String { } Value::Function(id) => id.to_string(), Value::Intrinsic(intrinsic) => intrinsic.to_string(), - Value::Param { .. } | Value::Instruction { .. } | Value::ForeignFunction(_) => { - id.to_string() - } + Value::ForeignFunction(function) => function.clone(), + Value::Param { .. } | Value::Instruction { .. } => id.to_string(), } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index e3b8c6e99aa..3a33accf3da 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1550,7 +1550,7 @@ mod test { fn deduplicates_side_effecting_intrinsics() { let src = " // After EnableSideEffectsIf removal: - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: Field, v1: Field, v2: u1): v4 = call is_unconstrained() -> u1 v7 = call to_be_radix(v0, u32 256) -> [u8; 1] // `a.to_be_radix(256)`; @@ -1567,7 +1567,7 @@ mod test { "; let ssa = Ssa::from_str(src).unwrap(); let expected = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: Field, v1: Field, v2: u1): v4 = call is_unconstrained() -> u1 v7 = call to_be_radix(v0, u32 256) -> [u8; 1] diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 7b38b764eab..7fdcb4c26c2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -637,10 +637,12 @@ mod test { use std::sync::Arc; use im::vector; + use noirc_frontend::monomorphization::ast::InlineType; use crate::ssa::{ function_builder::FunctionBuilder, ir::{ + function::RuntimeType, map::Id, types::{NumericType, Type}, }, @@ -742,7 +744,7 @@ mod test { #[test] fn keep_paired_rcs_with_array_set() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 2]): inc_rc v0 v2 = array_set v0, index u32 0, value u32 0 @@ -759,7 +761,7 @@ mod test { #[test] fn keep_inc_rc_on_borrowed_array_store() { - // acir(inline) fn main f0 { + // brillig(inline) fn main f0 { // b0(): // v1 = make_array [u32 0, u32 0] // v2 = allocate @@ -776,6 +778,7 @@ mod test { // Compiling main let mut builder = FunctionBuilder::new("main".into(), main_id); + builder.set_runtime(RuntimeType::Brillig(InlineType::Inline)); let zero = builder.numeric_constant(0u128, NumericType::unsigned(32)); let array_type = Type::Array(Arc::new(vec![Type::unsigned(32)]), 2); let v1 = builder.insert_make_array(vector![zero, zero], array_type.clone()); @@ -810,7 +813,7 @@ mod test { #[test] fn keep_inc_rc_on_borrowed_array_set() { - // acir(inline) fn main f0 { + // brillig(inline) fn main f0 { // b0(v0: [u32; 2]): // inc_rc v0 // v3 = array_set v0, index u32 0, value u32 1 @@ -821,7 +824,7 @@ mod test { // return v4 // } let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [u32; 2]): inc_rc v0 v3 = array_set v0, index u32 0, value u32 1 @@ -837,7 +840,7 @@ mod test { // We expect the output to be unchanged // Except for the repeated inc_rc instructions let expected = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [u32; 2]): inc_rc v0 v3 = array_set v0, index u32 0, value u32 1 @@ -875,7 +878,7 @@ mod test { #[test] fn remove_inc_rcs_that_are_never_mutably_borrowed() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 2]): inc_rc v0 inc_rc v0 @@ -893,7 +896,7 @@ mod test { assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); let expected = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 2]): v2 = array_get v0, index u32 0 -> Field return v2 diff --git a/compiler/noirc_evaluator/src/ssa/opt/hint.rs b/compiler/noirc_evaluator/src/ssa/opt/hint.rs index 567a0795edc..1326c2cc010 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/hint.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/hint.rs @@ -14,7 +14,6 @@ mod tests { let options = &SsaEvaluatorOptions { ssa_logging: SsaLogging::None, enable_brillig_logging: false, - force_brillig_output: false, print_codegen_timings: false, expression_width: ExpressionWidth::default(), emit_ssa: None, diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index bd0c86570e2..58dbc25e869 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -20,8 +20,8 @@ mod rc; mod remove_bit_shifts; mod remove_enable_side_effects; mod remove_if_else; +mod remove_unreachable; mod resolve_is_unconstrained; -mod runtime_separation; mod simplify_cfg; mod unrolling; diff --git a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs index 7f5352155de..56f69a912d4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -181,7 +181,9 @@ impl IdMaps { } Value::Function(id) => { - let new_id = self.function_ids[id]; + let new_id = *self.function_ids.get(id).unwrap_or_else(|| { + unreachable!("Unmapped function with id {id}") + }); new_function.dfg.import_function(new_id) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs new file mode 100644 index 00000000000..41023b5f376 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs @@ -0,0 +1,80 @@ +use std::collections::BTreeSet; + +use fxhash::FxHashSet as HashSet; + +use crate::ssa::{ + ir::{ + function::{Function, FunctionId}, + instruction::Instruction, + value::Value, + }, + ssa_gen::Ssa, +}; + +impl Ssa { + /// Removes any unreachable functions from the code. These can result from + /// optimizations making existing functions unreachable, e.g. `if false { foo() }`, + /// or even from monomorphizing an unconstrained version of a constrained function + /// where the original constrained version ends up never being used. + pub(crate) fn remove_unreachable_functions(mut self) -> Self { + let mut used_functions = HashSet::default(); + + for function_id in self.functions.keys() { + if self.is_entry_point(*function_id) { + collect_reachable_functions(&self, *function_id, &mut used_functions); + } + } + + self.functions.retain(|id, _| used_functions.contains(id)); + self + } +} + +fn collect_reachable_functions( + ssa: &Ssa, + current_func_id: FunctionId, + reachable_functions: &mut HashSet, +) { + if reachable_functions.contains(¤t_func_id) { + return; + } + reachable_functions.insert(current_func_id); + + // If the debugger is used, its possible for function inlining + // to remove functions that the debugger still references + let Some(func) = ssa.functions.get(¤t_func_id) else { + return; + }; + + let used_functions = used_functions(func); + + for called_func_id in used_functions.iter() { + collect_reachable_functions(ssa, *called_func_id, reachable_functions); + } +} + +fn used_functions(func: &Function) -> BTreeSet { + let mut used_function_ids = BTreeSet::default(); + + let mut find_functions = |value| { + if let Value::Function(function) = func.dfg[func.dfg.resolve(value)] { + used_function_ids.insert(function); + } + }; + + for block_id in func.reachable_blocks() { + let block = &func.dfg[block_id]; + + for instruction_id in block.instructions() { + let instruction = &func.dfg[*instruction_id]; + + if matches!(instruction, Instruction::Store { .. } | Instruction::Call { .. }) { + instruction.for_each_value(&mut find_functions); + } + } + + block.unwrap_terminator().for_each_value(&mut find_functions); + } + + used_function_ids +} diff --git a/compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs b/compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs deleted file mode 100644 index b671d5011a1..00000000000 --- a/compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs +++ /dev/null @@ -1,351 +0,0 @@ -use std::collections::BTreeSet; - -use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; - -use crate::ssa::{ - ir::{ - function::{Function, FunctionId, RuntimeType}, - instruction::Instruction, - value::{Value, ValueId}, - }, - ssa_gen::Ssa, -}; - -impl Ssa { - /// This optimization step separates the runtime of the functions in the SSA. - /// After this step, all functions with runtime `Acir` will be converted to Acir and - /// the functions with runtime `Brillig` will be converted to Brillig. - /// It does so by cloning all ACIR functions called from a Brillig context - /// and changing the runtime of the cloned functions to Brillig. - /// This pass needs to run after functions as values have been resolved (defunctionalization). - #[tracing::instrument(level = "trace", skip(self))] - pub(crate) fn separate_runtime(mut self) -> Self { - RuntimeSeparatorContext::separate_runtime(&mut self); - self - } -} - -#[derive(Debug, Default)] -struct RuntimeSeparatorContext { - // Original functions to clone to brillig - acir_functions_called_from_brillig: BTreeSet, - // Tracks the original => cloned version - mapped_functions: HashMap, -} - -impl RuntimeSeparatorContext { - pub(crate) fn separate_runtime(ssa: &mut Ssa) { - let mut runtime_separator = RuntimeSeparatorContext::default(); - - // We first collect all the acir functions called from a brillig context by exploring the SSA recursively - let mut processed_functions = HashSet::default(); - runtime_separator.collect_acir_functions_called_from_brillig( - ssa, - ssa.main_id, - false, - &mut processed_functions, - ); - - // Now we clone the relevant acir functions and change their runtime to brillig - runtime_separator.convert_acir_functions_called_from_brillig_to_brillig(ssa); - - // Now we update any calls within a brillig context to the mapped functions - runtime_separator.replace_calls_to_mapped_functions(ssa); - - // Some functions might be unreachable now (for example an acir function only called from brillig) - prune_unreachable_functions(ssa); - } - - fn collect_acir_functions_called_from_brillig( - &mut self, - ssa: &Ssa, - current_func_id: FunctionId, - mut within_brillig: bool, - processed_functions: &mut HashSet<(/* within_brillig */ bool, FunctionId)>, - ) { - // Processed functions needs the within brillig flag, since it is possible to call the same function from both brillig and acir - if processed_functions.contains(&(within_brillig, current_func_id)) { - return; - } - processed_functions.insert((within_brillig, current_func_id)); - - let func = &ssa.functions[¤t_func_id]; - if matches!(func.runtime(), RuntimeType::Brillig(_)) { - within_brillig = true; - } - - let called_functions = called_functions(func); - - if within_brillig { - for called_func_id in called_functions.iter() { - let called_func = &ssa.functions[called_func_id]; - if matches!(called_func.runtime(), RuntimeType::Acir(_)) { - self.acir_functions_called_from_brillig.insert(*called_func_id); - } - } - } - - for called_func_id in called_functions.into_iter() { - self.collect_acir_functions_called_from_brillig( - ssa, - called_func_id, - within_brillig, - processed_functions, - ); - } - } - - fn convert_acir_functions_called_from_brillig_to_brillig(&mut self, ssa: &mut Ssa) { - for acir_func_id in self.acir_functions_called_from_brillig.iter() { - let RuntimeType::Acir(inline_type) = ssa.functions[acir_func_id].runtime() else { - unreachable!("Function to transform to brillig should be ACIR") - }; - let cloned_id = ssa.clone_fn(*acir_func_id); - let new_func = - ssa.functions.get_mut(&cloned_id).expect("Cloned function should exist in SSA"); - new_func.set_runtime(RuntimeType::Brillig(inline_type)); - self.mapped_functions.insert(*acir_func_id, cloned_id); - } - } - - fn replace_calls_to_mapped_functions(&self, ssa: &mut Ssa) { - for (_function_id, func) in ssa.functions.iter_mut() { - if matches!(func.runtime(), RuntimeType::Brillig(_)) { - for called_func_value_id in called_functions_values(func).iter() { - let Value::Function(called_func_id) = &func.dfg[*called_func_value_id] else { - unreachable!("Value should be a function") - }; - if let Some(mapped_func_id) = self.mapped_functions.get(called_func_id) { - let mapped_value_id = func.dfg.import_function(*mapped_func_id); - func.dfg.set_value_from_id(*called_func_value_id, mapped_value_id); - } - } - } - } - } -} - -// We only consider direct calls to functions since functions as values should have been resolved -fn called_functions_values(func: &Function) -> BTreeSet { - let mut called_function_ids = BTreeSet::default(); - for block_id in func.reachable_blocks() { - for instruction_id in func.dfg[block_id].instructions() { - let Instruction::Call { func: called_value_id, .. } = &func.dfg[*instruction_id] else { - continue; - }; - - if let Value::Function(_) = func.dfg[*called_value_id] { - called_function_ids.insert(*called_value_id); - } - } - } - - called_function_ids -} - -fn called_functions(func: &Function) -> BTreeSet { - called_functions_values(func) - .into_iter() - .map(|value_id| { - let Value::Function(func_id) = func.dfg[value_id] else { - unreachable!("Value should be a function") - }; - func_id - }) - .collect() -} - -fn collect_reachable_functions( - ssa: &Ssa, - current_func_id: FunctionId, - reachable_functions: &mut HashSet, -) { - if reachable_functions.contains(¤t_func_id) { - return; - } - reachable_functions.insert(current_func_id); - - let func = &ssa.functions[¤t_func_id]; - let called_functions = called_functions(func); - - for called_func_id in called_functions.iter() { - collect_reachable_functions(ssa, *called_func_id, reachable_functions); - } -} - -fn prune_unreachable_functions(ssa: &mut Ssa) { - let mut reachable_functions = HashSet::default(); - collect_reachable_functions(ssa, ssa.main_id, &mut reachable_functions); - - ssa.functions.retain(|id, _value| reachable_functions.contains(id)); -} - -#[cfg(test)] -mod test { - use std::collections::BTreeSet; - - use noirc_frontend::monomorphization::ast::InlineType; - - use crate::ssa::{ - function_builder::FunctionBuilder, - ir::{ - function::{Function, FunctionId, RuntimeType}, - map::Id, - types::Type, - }, - opt::runtime_separation::called_functions, - ssa_gen::Ssa, - }; - - #[test] - fn basic_runtime_separation() { - // brillig fn foo { - // b0(): - // v0 = call bar() - // return v0 - // } - // acir fn bar { - // b0(): - // return 72 - // } - let foo_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("foo".into(), foo_id); - builder.current_function.set_runtime(RuntimeType::Brillig(InlineType::default())); - - let bar_id = Id::test_new(1); - let bar = builder.import_function(bar_id); - let results = builder.insert_call(bar, Vec::new(), vec![Type::field()]).to_vec(); - builder.terminate_with_return(results); - - builder.new_function("bar".into(), bar_id, InlineType::default()); - let expected_return = 72u128; - let seventy_two = builder.field_constant(expected_return); - builder.terminate_with_return(vec![seventy_two]); - - let ssa = builder.finish(); - assert_eq!(ssa.functions.len(), 2); - - // Expected result - // brillig fn foo { - // b0(): - // v0 = call bar() - // return v0 - // } - // brillig fn bar { - // b0(): - // return 72 - // } - let separated = ssa.separate_runtime(); - - // The original bar function must have been pruned - assert_eq!(separated.functions.len(), 2); - - // All functions should be brillig now - for func in separated.functions.values() { - assert_eq!(func.runtime(), RuntimeType::Brillig(InlineType::default())); - } - } - - fn find_func_by_name<'ssa>( - ssa: &'ssa Ssa, - funcs: &BTreeSet, - name: &str, - ) -> &'ssa Function { - funcs - .iter() - .find_map(|id| { - let func = ssa.functions.get(id).unwrap(); - if func.name() == name { - Some(func) - } else { - None - } - }) - .unwrap() - } - - #[test] - fn same_function_shared_acir_brillig() { - // acir fn foo { - // b0(): - // v0 = call bar() - // v1 = call baz() - // return v0, v1 - // } - // brillig fn bar { - // b0(): - // v0 = call baz() - // return v0 - // } - // acir fn baz { - // b0(): - // return 72 - // } - let foo_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("foo".into(), foo_id); - - let bar_id = Id::test_new(1); - let baz_id = Id::test_new(2); - let bar = builder.import_function(bar_id); - let baz = builder.import_function(baz_id); - let v0 = builder.insert_call(bar, Vec::new(), vec![Type::field()]).to_vec(); - let v1 = builder.insert_call(baz, Vec::new(), vec![Type::field()]).to_vec(); - builder.terminate_with_return(vec![v0[0], v1[0]]); - - builder.new_brillig_function("bar".into(), bar_id, InlineType::default()); - let baz = builder.import_function(baz_id); - let v0 = builder.insert_call(baz, Vec::new(), vec![Type::field()]).to_vec(); - builder.terminate_with_return(v0); - - builder.new_function("baz".into(), baz_id, InlineType::default()); - let expected_return = 72u128; - let seventy_two = builder.field_constant(expected_return); - builder.terminate_with_return(vec![seventy_two]); - - let ssa = builder.finish(); - assert_eq!(ssa.functions.len(), 3); - - // Expected result - // acir fn foo { - // b0(): - // v0 = call bar() - // v1 = call baz() <- baz_acir - // return v0, v1 - // } - // brillig fn bar { - // b0(): - // v0 = call baz() <- baz_brillig - // return v0 - // } - // acir fn baz { - // b0(): - // return 72 - // } - // brillig fn baz { - // b0(): - // return 72 - // } - let separated = ssa.separate_runtime(); - - // The original baz function must have been duplicated - assert_eq!(separated.functions.len(), 4); - - let main_function = separated.functions.get(&separated.main_id).unwrap(); - assert_eq!(main_function.runtime(), RuntimeType::Acir(InlineType::Inline)); - - let main_calls = called_functions(main_function); - assert_eq!(main_calls.len(), 2); - - let bar = find_func_by_name(&separated, &main_calls, "bar"); - let baz_acir = find_func_by_name(&separated, &main_calls, "baz"); - - assert_eq!(baz_acir.runtime(), RuntimeType::Acir(InlineType::Inline)); - assert_eq!(bar.runtime(), RuntimeType::Brillig(InlineType::default())); - - let bar_calls = called_functions(bar); - assert_eq!(bar_calls.len(), 1); - - let baz_brillig = find_func_by_name(&separated, &bar_calls, "baz"); - assert_eq!(baz_brillig.runtime(), RuntimeType::Brillig(InlineType::default())); - } -} diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index 899a5a571e7..b5aac13cfd8 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -415,7 +415,7 @@ fn test_store() { #[test] fn test_inc_rc() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 3]): inc_rc v0 return @@ -427,7 +427,7 @@ fn test_inc_rc() { #[test] fn test_dec_rc() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 3]): dec_rc v0 return diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 7807658dabb..e89d1d2a0c3 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -4,7 +4,7 @@ use acvm::{acir::AcirField, FieldElement}; use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::ast::{BinaryOpKind, Signedness}; -use noirc_frontend::monomorphization::ast::{self, InlineType, LocalId, Parameters}; +use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; use crate::errors::RuntimeError; @@ -119,14 +119,9 @@ impl<'a> FunctionContext<'a> { /// /// Note that the previous function cannot be resumed after calling this. Developers should /// avoid calling new_function until the previous function is completely finished with ssa-gen. - pub(super) fn new_function( - &mut self, - id: IrFunctionId, - func: &ast::Function, - force_brillig_runtime: bool, - ) { + pub(super) fn new_function(&mut self, id: IrFunctionId, func: &ast::Function) { self.definitions.clear(); - if func.unconstrained || (force_brillig_runtime && func.inline_type != InlineType::Inline) { + if func.unconstrained { self.builder.new_brillig_function(func.name.clone(), id, func.inline_type); } else { self.builder.new_function(func.name.clone(), id, func.inline_type); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d3821158b80..b3a8ecf1314 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -39,10 +39,7 @@ pub(crate) const SSA_WORD_SIZE: u32 = 32; /// Generates SSA for the given monomorphized program. /// /// This function will generate the SSA but does not perform any optimizations on it. -pub(crate) fn generate_ssa( - program: Program, - force_brillig_runtime: bool, -) -> Result { +pub(crate) fn generate_ssa(program: Program) -> Result { // see which parameter has call_data/return_data attribute let is_databus = DataBusBuilder::is_databus(&program.main_function_signature); @@ -56,16 +53,13 @@ pub(crate) fn generate_ssa( // Queue the main function for compilation context.get_or_queue_function(main_id); - let mut function_context = FunctionContext::new( - main.name.clone(), - &main.parameters, - if force_brillig_runtime || main.unconstrained { - RuntimeType::Brillig(main.inline_type) - } else { - RuntimeType::Acir(main.inline_type) - }, - &context, - ); + let main_runtime = if main.unconstrained { + RuntimeType::Brillig(main.inline_type) + } else { + RuntimeType::Acir(main.inline_type) + }; + let mut function_context = + FunctionContext::new(main.name.clone(), &main.parameters, main_runtime, &context); // Generate the call_data bus from the relevant parameters. We create it *before* processing the function body let call_data = function_context.builder.call_data_bus(is_databus); @@ -122,7 +116,7 @@ pub(crate) fn generate_ssa( // to generate SSA for each function used within the program. while let Some((src_function_id, dest_id)) = context.pop_next_function_in_queue() { let function = &context.program[src_function_id]; - function_context.new_function(dest_id, function, force_brillig_runtime); + function_context.new_function(dest_id, function); function_context.codegen_function_body(&function.body)?; } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 12892efa598..98cdc0adad9 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use crate::ssa::ir::{ - function::{Function, FunctionId, RuntimeType}, + function::{Function, FunctionId}, map::AtomicCounter, }; use noirc_frontend::hir_def::types::Type as HirType; @@ -78,29 +78,10 @@ impl Ssa { new_id } - /// Clones an already existing function with a fresh id - pub(crate) fn clone_fn(&mut self, existing_function_id: FunctionId) -> FunctionId { - let new_id = self.next_id.next(); - let function = Function::clone_with_id(new_id, &self.functions[&existing_function_id]); - self.functions.insert(new_id, function); - new_id - } pub(crate) fn generate_entry_point_index(mut self) -> Self { - self.entry_point_to_generated_index = btree_map( - self.functions - .iter() - .filter(|(_, func)| { - let runtime = func.runtime(); - match func.runtime() { - RuntimeType::Acir(_) => { - runtime.is_entry_point() || func.id() == self.main_id - } - RuntimeType::Brillig(_) => false, - } - }) - .enumerate(), - |(i, (id, _))| (*id, i as u32), - ); + let entry_points = + self.functions.keys().filter(|function| self.is_entry_point(**function)).enumerate(); + self.entry_point_to_generated_index = btree_map(entry_points, |(i, id)| (*id, i as u32)); self } @@ -112,6 +93,10 @@ impl Ssa { ); self.entry_point_to_generated_index.get(func_id).copied() } + + pub(crate) fn is_entry_point(&self, function: FunctionId) -> bool { + function == self.main_id || self.functions[&function].runtime().is_entry_point() + } } impl Display for Ssa { diff --git a/compiler/noirc_frontend/Cargo.toml b/compiler/noirc_frontend/Cargo.toml index 5f8f02689c8..041c1b1e015 100644 --- a/compiler/noirc_frontend/Cargo.toml +++ b/compiler/noirc_frontend/Cargo.toml @@ -31,6 +31,7 @@ petgraph = "0.6" rangemap = "1.4.0" strum.workspace = true strum_macros.workspace = true +fxhash.workspace = true [dev-dependencies] diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 8c07d71de21..fbc80cc0ac9 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -25,11 +25,12 @@ use crate::{ Kind, Type, TypeBinding, TypeBindings, }; use acvm::{acir::AcirField, FieldElement}; +use fxhash::FxHashMap as HashMap; use iter_extended::{btree_map, try_vecmap, vecmap}; use noirc_errors::Location; use noirc_printable_type::PrintableType; use std::{ - collections::{BTreeMap, HashMap, VecDeque}, + collections::{BTreeMap, VecDeque}, unreachable, }; @@ -56,14 +57,12 @@ struct LambdaContext { /// This struct holds the FIFO queue of functions to monomorphize, which is added to /// whenever a new (function, type) combination is encountered. struct Monomorphizer<'interner> { - /// Functions are keyed by their unique ID and expected type so that we can monomorphize - /// a new version of the function for each type. - /// We also key by any turbofish generics that are specified. - /// This is necessary for a case where we may have a trait generic that can be instantiated - /// outside of a function parameter or return value. + /// Functions are keyed by their unique ID, whether they're unconstrained, their expected type, + /// and any generics they have so that we can monomorphize a new version of the function for each type. /// - /// Using nested HashMaps here lets us avoid cloning HirTypes when calling .get() - functions: HashMap), FuncId>>, + /// Keying by any turbofish generics that are specified is necessary for a case where we may have a + /// trait generic that can be instantiated outside of a function parameter or return value. + functions: Functions, /// Unlike functions, locals are only keyed by their unique ID because they are never /// duplicated during monomorphization. Doing so would allow them to be used polymorphically @@ -72,8 +71,15 @@ struct Monomorphizer<'interner> { locals: HashMap, /// Queue of functions to monomorphize next each item in the queue is a tuple of: - /// (old_id, new_monomorphized_id, any type bindings to apply, the trait method if old_id is from a trait impl) - queue: VecDeque<(node_interner::FuncId, FuncId, TypeBindings, Option, Location)>, + /// (old_id, new_monomorphized_id, any type bindings to apply, the trait method if old_id is from a trait impl, is_unconstrained, location) + queue: VecDeque<( + node_interner::FuncId, + FuncId, + TypeBindings, + Option, + bool, + Location, + )>, /// When a function finishes being monomorphized, the monomorphized ast::Function is /// stored here along with its FuncId. @@ -92,8 +98,16 @@ struct Monomorphizer<'interner> { return_location: Option, debug_type_tracker: DebugTypeTracker, + + in_unconstrained_function: bool, } +/// Using nested HashMaps here lets us avoid cloning HirTypes when calling .get() +type Functions = HashMap< + (node_interner::FuncId, /*is_unconstrained:*/ bool), + HashMap, FuncId>>, +>; + type HirType = crate::Type; /// Starting from the given `main` function, monomorphize the entire program, @@ -111,24 +125,29 @@ type HirType = crate::Type; pub fn monomorphize( main: node_interner::FuncId, interner: &mut NodeInterner, + force_unconstrained: bool, ) -> Result { - monomorphize_debug(main, interner, &DebugInstrumenter::default()) + monomorphize_debug(main, interner, &DebugInstrumenter::default(), force_unconstrained) } pub fn monomorphize_debug( main: node_interner::FuncId, interner: &mut NodeInterner, debug_instrumenter: &DebugInstrumenter, + force_unconstrained: bool, ) -> Result { let debug_type_tracker = DebugTypeTracker::build_from_debug_instrumenter(debug_instrumenter); let mut monomorphizer = Monomorphizer::new(interner, debug_type_tracker); + monomorphizer.in_unconstrained_function = force_unconstrained; let function_sig = monomorphizer.compile_main(main)?; while !monomorphizer.queue.is_empty() { - let (next_fn_id, new_id, bindings, trait_method, location) = + let (next_fn_id, new_id, bindings, trait_method, is_unconstrained, location) = monomorphizer.queue.pop_front().unwrap(); monomorphizer.locals.clear(); + monomorphizer.in_unconstrained_function = is_unconstrained; + perform_instantiation_bindings(&bindings); let interner = &monomorphizer.interner; let impl_bindings = perform_impl_bindings(interner, trait_method, next_fn_id, location) @@ -143,7 +162,9 @@ pub fn monomorphize_debug( .finished_functions .iter() .flat_map(|(_, f)| { - if f.inline_type.is_entry_point() || f.id == Program::main_id() { + if (!force_unconstrained && f.inline_type.is_entry_point()) + || f.id == Program::main_id() + { Some(f.func_sig.clone()) } else { None @@ -172,8 +193,8 @@ pub fn monomorphize_debug( impl<'interner> Monomorphizer<'interner> { fn new(interner: &'interner mut NodeInterner, debug_type_tracker: DebugTypeTracker) -> Self { Monomorphizer { - functions: HashMap::new(), - locals: HashMap::new(), + functions: HashMap::default(), + locals: HashMap::default(), queue: VecDeque::new(), finished_functions: BTreeMap::new(), next_local_id: 0, @@ -183,6 +204,7 @@ impl<'interner> Monomorphizer<'interner> { is_range_loop: false, return_location: None, debug_type_tracker, + in_unconstrained_function: false, } } @@ -207,14 +229,18 @@ impl<'interner> Monomorphizer<'interner> { id: node_interner::FuncId, expr_id: node_interner::ExprId, typ: &HirType, - turbofish_generics: Vec, + turbofish_generics: &[HirType], trait_method: Option, ) -> Definition { let typ = typ.follow_bindings(); + let turbofish_generics = vecmap(turbofish_generics, |typ| typ.follow_bindings()); + let is_unconstrained = self.is_unconstrained(id); + match self .functions - .get(&id) - .and_then(|inner_map| inner_map.get(&(typ.clone(), turbofish_generics.clone()))) + .get(&(id, is_unconstrained)) + .and_then(|inner_map| inner_map.get(&typ)) + .and_then(|inner_map| inner_map.get(&turbofish_generics)) { Some(id) => Definition::Function(*id), None => { @@ -257,14 +283,21 @@ impl<'interner> Monomorphizer<'interner> { } /// Prerequisite: typ = typ.follow_bindings() + /// and: turbofish_generics = vecmap(turbofish_generics, Type::follow_bindings) fn define_function( &mut self, id: node_interner::FuncId, typ: HirType, turbofish_generics: Vec, + is_unconstrained: bool, new_id: FuncId, ) { - self.functions.entry(id).or_default().insert((typ, turbofish_generics), new_id); + self.functions + .entry((id, is_unconstrained)) + .or_default() + .entry(typ) + .or_default() + .insert(turbofish_generics, new_id); } fn compile_main( @@ -275,6 +308,7 @@ impl<'interner> Monomorphizer<'interner> { assert_eq!(new_main_id, Program::main_id()); let location = self.interner.function_meta(&main_id).location; + self.in_unconstrained_function = self.is_unconstrained(main_id); self.function(main_id, new_main_id, location)?; self.return_location = @@ -324,7 +358,7 @@ impl<'interner> Monomorphizer<'interner> { }; let return_type = Self::convert_type(return_type, meta.location)?; - let unconstrained = modifiers.is_unconstrained; + let unconstrained = self.in_unconstrained_function; let attributes = self.interner.function_attributes(&f); let inline_type = InlineType::from(attributes); @@ -874,7 +908,7 @@ impl<'interner> Monomorphizer<'interner> { *func_id, expr_id, &typ, - generics.unwrap_or_default(), + &generics.unwrap_or_default(), None, ); let typ = Self::convert_type(&typ, ident.location)?; @@ -1281,7 +1315,7 @@ impl<'interner> Monomorphizer<'interner> { .map_err(MonomorphizationError::InterpreterError)?; let func_id = - match self.lookup_function(func_id, expr_id, &function_type, vec![], Some(method)) { + match self.lookup_function(func_id, expr_id, &function_type, &[], Some(method)) { Definition::Function(func_id) => func_id, _ => unreachable!(), }; @@ -1546,12 +1580,19 @@ impl<'interner> Monomorphizer<'interner> { trait_method: Option, ) -> FuncId { let new_id = self.next_function_id(); - self.define_function(id, function_type.clone(), turbofish_generics, new_id); + let is_unconstrained = self.is_unconstrained(id); + self.define_function( + id, + function_type.clone(), + turbofish_generics, + is_unconstrained, + new_id, + ); let location = self.interner.expr_location(&expr_id); let bindings = self.interner.get_instantiation_bindings(expr_id); let bindings = self.follow_bindings(bindings); - self.queue.push_back((id, new_id, bindings, trait_method, location)); + self.queue.push_back((id, new_id, bindings, trait_method, is_unconstrained, location)); new_id } @@ -1638,19 +1679,15 @@ impl<'interner> Monomorphizer<'interner> { let parameters = self.parameters(¶meters)?; let body = self.expr(lambda.body)?; - let id = self.next_function_id(); - let return_type = ret_type.clone(); - let name = lambda_name.to_owned(); - let unconstrained = false; let function = ast::Function { id, - name, + name: lambda_name.to_owned(), parameters, body, - return_type, - unconstrained, + return_type: ret_type.clone(), + unconstrained: self.in_unconstrained_function, inline_type: InlineType::default(), func_sig: FunctionSignature::default(), }; @@ -1773,18 +1810,16 @@ impl<'interner> Monomorphizer<'interner> { typ: lambda_fn_typ.clone(), }); - let mut parameters = vec![]; - parameters.push((env_local_id, true, env_name.to_string(), env_typ.clone())); + let mut parameters = vec![(env_local_id, true, env_name.to_string(), env_typ.clone())]; parameters.append(&mut converted_parameters); - let unconstrained = false; let function = ast::Function { id, name, parameters, body, return_type, - unconstrained, + unconstrained: self.in_unconstrained_function, inline_type: InlineType::default(), func_sig: FunctionSignature::default(), }; @@ -2007,6 +2042,11 @@ impl<'interner> Monomorphizer<'interner> { Ok(ast::Expression::Call(ast::Call { func, arguments, return_type, location })) } + + fn is_unconstrained(&self, func_id: node_interner::FuncId) -> bool { + self.in_unconstrained_function + || self.interner.function_modifiers(&func_id).is_unconstrained + } } fn unwrap_tuple_type(typ: &HirType) -> Vec { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 3d908d1aa0c..3377c260922 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1244,7 +1244,7 @@ fn resolve_fmt_strings() { fn monomorphize_program(src: &str) -> Result { let (_program, mut context, _errors) = get_program(src); let main_func_id = context.def_interner.find_function("main").unwrap(); - monomorphize(main_func_id, &mut context.def_interner) + monomorphize(main_func_id, &mut context.def_interner, false) } fn get_monomorphization_error(src: &str) -> Option { From 50647a6448f86faec59660f0ea96bc3d10559d8c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 6 Jan 2025 15:11:55 -0500 Subject: [PATCH 08/11] chore: Move comment as part of #6945 (#6959) --- tooling/nargo_cli/build.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 04cc463f6b3..140575e0209 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -188,6 +188,7 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner nargo.arg("--program-dir").arg(test_program_dir); nargo.arg("{test_command}").arg("--force"); nargo.arg("--inliner-aggressiveness").arg(inliner_aggressiveness.0.to_string()); + // Check whether the test case is non-deterministic nargo.arg("--check-non-determinism"); if force_brillig.0 {{ @@ -196,8 +197,6 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner // Set the maximum increase so that part of the optimization is exercised (it might fail). nargo.arg("--max-bytecode-increase-percent"); nargo.arg("50"); - - // Check whether the test case is non-deterministic }} {test_content} From ab8807d27b0442bd7e9ff73050f44bacea3add16 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 6 Jan 2025 20:26:00 +0000 Subject: [PATCH 09/11] feat(ssa): Immediately simplify away RefCount instructions in ACIR functions (#6893) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: Tom French Co-authored-by: Maxim Vezenov --- compiler/noirc_evaluator/src/acir/mod.rs | 3 +- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 34 +++++++++++++++++-- .../noirc_evaluator/src/ssa/ir/function.rs | 18 ++++++---- .../noirc_evaluator/src/ssa/ir/instruction.rs | 6 ++++ compiler/noirc_evaluator/src/ssa/ir/value.rs | 8 ++--- .../src/ssa/opt/constant_folding.rs | 4 +++ compiler/noirc_evaluator/src/ssa/opt/rc.rs | 2 ++ .../src/ssa/parser/into_ssa.rs | 2 +- 8 files changed, 61 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index a82c54d8ce6..e7fa601fbe3 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -769,7 +769,8 @@ impl<'a> Context<'a> { unreachable!("Expected all load instructions to be removed before acir_gen") } Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => { - // Do nothing. Only Brillig needs to worry about reference counted arrays + // Only Brillig needs to worry about reference counted arrays + unreachable!("Expected all Rc instructions to be removed before acir_gen") } Instruction::RangeCheck { value, max_bit_size, assert_message } => { let acir_var = self.convert_numeric_value(*value, dfg)?; diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index f531e8307f1..8f1b360a120 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -5,7 +5,7 @@ use crate::ssa::{function_builder::data_bus::DataBus, ir::instruction::SimplifyR use super::{ basic_block::{BasicBlock, BasicBlockId}, call_stack::{CallStack, CallStackHelper, CallStackId}, - function::FunctionId, + function::{FunctionId, RuntimeType}, instruction::{ Instruction, InstructionId, InstructionResultType, Intrinsic, TerminatorInstruction, }, @@ -26,8 +26,13 @@ use serde_with::DisplayFromStr; /// owning most data in a function and handing out Ids to this data that can be /// shared without worrying about ownership. #[serde_as] -#[derive(Debug, Default, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, Default)] pub(crate) struct DataFlowGraph { + /// Runtime of the [Function] that owns this [DataFlowGraph]. + /// This might change during the `runtime_separation` pass where + /// ACIR functions are cloned as Brillig functions. + runtime: RuntimeType, + /// All of the instructions in a function instructions: DenseMap, @@ -100,6 +105,16 @@ pub(crate) struct DataFlowGraph { } impl DataFlowGraph { + /// Runtime type of the function. + pub(crate) fn runtime(&self) -> RuntimeType { + self.runtime + } + + /// Set runtime type of the function. + pub(crate) fn set_runtime(&mut self, runtime: RuntimeType) { + self.runtime = runtime; + } + /// Creates a new basic block with no parameters. /// After being created, the block is unreachable in the current function /// until another block is made to jump to it. @@ -164,6 +179,11 @@ impl DataFlowGraph { id } + /// Check if the function runtime would simply ignore this instruction. + pub(crate) fn is_handled_by_runtime(&self, instruction: &Instruction) -> bool { + !(self.runtime().is_acir() && instruction.is_brillig_only()) + } + fn insert_instruction_without_simplification( &mut self, instruction_data: Instruction, @@ -184,6 +204,10 @@ impl DataFlowGraph { ctrl_typevars: Option>, call_stack: CallStackId, ) -> InsertInstructionResult { + if !self.is_handled_by_runtime(&instruction_data) { + return InsertInstructionResult::InstructionRemoved; + } + let id = self.insert_instruction_without_simplification( instruction_data, block, @@ -194,7 +218,8 @@ impl DataFlowGraph { InsertInstructionResult::Results(id, self.instruction_results(id)) } - /// Inserts a new instruction at the end of the given block and returns its results + /// Simplifies a new instruction and inserts it at the end of the given block and returns its results. + /// If the instruction is not handled by the current runtime, `InstructionRemoved` is returned. pub(crate) fn insert_instruction_and_results( &mut self, instruction: Instruction, @@ -202,6 +227,9 @@ impl DataFlowGraph { ctrl_typevars: Option>, call_stack: CallStackId, ) -> InsertInstructionResult { + if !self.is_handled_by_runtime(&instruction) { + return InsertInstructionResult::InstructionRemoved; + } match instruction.simplify(self, block, ctrl_typevars.clone(), call_stack) { SimplifyResult::SimplifiedTo(simplification) => { InsertInstructionResult::SimplifiedTo(simplification) diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index 6413107c04a..109c2a59781 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -56,6 +56,12 @@ impl RuntimeType { } } +impl Default for RuntimeType { + fn default() -> Self { + RuntimeType::Acir(InlineType::default()) + } +} + /// A function holds a list of instructions. /// These instructions are further grouped into Basic blocks /// @@ -72,8 +78,6 @@ pub(crate) struct Function { id: FunctionId, - runtime: RuntimeType, - /// The DataFlowGraph holds the majority of data pertaining to the function /// including its blocks, instructions, and values. pub(crate) dfg: DataFlowGraph, @@ -86,20 +90,20 @@ impl Function { pub(crate) fn new(name: String, id: FunctionId) -> Self { let mut dfg = DataFlowGraph::default(); let entry_block = dfg.make_block(); - Self { name, id, entry_block, dfg, runtime: RuntimeType::Acir(InlineType::default()) } + Self { name, id, entry_block, dfg } } /// Creates a new function as a clone of the one passed in with the passed in id. pub(crate) fn clone_with_id(id: FunctionId, another: &Function) -> Self { let dfg = another.dfg.clone(); let entry_block = another.entry_block; - Self { name: another.name.clone(), id, entry_block, dfg, runtime: another.runtime } + Self { name: another.name.clone(), id, entry_block, dfg } } /// Takes the signature (function name & runtime) from a function but does not copy the body. pub(crate) fn clone_signature(id: FunctionId, another: &Function) -> Self { let mut new_function = Function::new(another.name.clone(), id); - new_function.runtime = another.runtime; + new_function.set_runtime(another.runtime()); new_function } @@ -116,12 +120,12 @@ impl Function { /// Runtime type of the function. pub(crate) fn runtime(&self) -> RuntimeType { - self.runtime + self.dfg.runtime() } /// Set runtime type of the function. pub(crate) fn set_runtime(&mut self, runtime: RuntimeType) { - self.runtime = runtime; + self.dfg.set_runtime(runtime); } pub(crate) fn is_no_predicates(&self) -> bool { diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index a66e272fb0c..aadd35040cf 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -1056,6 +1056,12 @@ impl Instruction { Instruction::Noop => Remove, } } + + /// Some instructions are only to be used in Brillig and should be eliminated + /// after runtime separation, never to be be reintroduced in an ACIR runtime. + pub(crate) fn is_brillig_only(&self) -> bool { + matches!(self, Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. }) + } } /// Given a chain of operations like: diff --git a/compiler/noirc_evaluator/src/ssa/ir/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index ec7a8e25246..39c63e3efcd 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -20,10 +20,10 @@ pub(crate) type ValueId = Id; pub(crate) enum Value { /// This value was created due to an instruction /// - /// instruction -- This is the instruction which defined it - /// typ -- This is the `Type` of the instruction - /// position -- Returns the position in the results - /// vector that this `Value` is located. + /// * `instruction`: This is the instruction which defined it + /// * `typ`: This is the `Type` of the instruction + /// * `position`: Returns the position in the results vector that this `Value` is located. + /// /// Example, if you add two numbers together, then the resulting /// value would have position `0`, the typ would be the type /// of the operands, and the instruction would map to an add instruction. diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 3a33accf3da..17aefdf21b8 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -830,9 +830,12 @@ fn simplify(dfg: &DataFlowGraph, lhs: ValueId, rhs: ValueId) -> Option<(ValueId, mod test { use std::sync::Arc; + use noirc_frontend::monomorphization::ast::InlineType; + use crate::ssa::{ function_builder::FunctionBuilder, ir::{ + function::RuntimeType, map::Id, types::{NumericType, Type}, }, @@ -1153,6 +1156,7 @@ mod test { // Compiling main let mut builder = FunctionBuilder::new("main".into(), main_id); + builder.set_runtime(RuntimeType::Brillig(InlineType::default())); let v0 = builder.add_parameter(Type::unsigned(64)); let zero = builder.numeric_constant(0u128, NumericType::unsigned(64)); let typ = Type::Array(Arc::new(vec![Type::unsigned(64)]), 25); diff --git a/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/compiler/noirc_evaluator/src/ssa/opt/rc.rs index 64f6e2ddfea..e25ad350145 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -246,6 +246,7 @@ mod test { // } let main_id = Id::test_new(0); let mut builder = FunctionBuilder::new("mutator".into(), main_id); + builder.set_runtime(RuntimeType::Brillig(InlineType::default())); let array_type = Type::Array(Arc::new(vec![Type::field()]), 2); let v0 = builder.add_parameter(array_type.clone()); @@ -295,6 +296,7 @@ mod test { // } let main_id = Id::test_new(0); let mut builder = FunctionBuilder::new("mutator2".into(), main_id); + builder.set_runtime(RuntimeType::Brillig(InlineType::default())); let array_type = Type::Array(Arc::new(vec![Type::field()]), 2); let reference_type = Type::Reference(Arc::new(array_type.clone())); diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index afada8c0e2c..fcaaf74f533 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -59,8 +59,8 @@ impl Translator { let main_function = parsed_ssa.functions.remove(0); let main_id = FunctionId::test_new(0); let mut builder = FunctionBuilder::new(main_function.external_name.clone(), main_id); - builder.simplify = simplify; builder.set_runtime(main_function.runtime_type); + builder.simplify = simplify; // Map function names to their IDs so calls can be resolved let mut function_id_counter = 1; From 04714a7d4917e08f79b771e0f1176df7387845ac Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Mon, 6 Jan 2025 22:30:32 +0100 Subject: [PATCH 10/11] chore: simplify boolean in a mul of a mul (#6951) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/ssa/ir/instruction/binary.rs | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index ce65343c7ef..ab37080ac1d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -131,11 +131,39 @@ impl Binary { let zero = dfg.make_constant(FieldElement::zero(), operand_type); return SimplifyResult::SimplifiedTo(zero); } - if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) - && dfg.get_value_max_num_bits(self.lhs) == 1 - { + if dfg.get_value_max_num_bits(self.lhs) == 1 { // Squaring a boolean value is a noop. - return SimplifyResult::SimplifiedTo(self.lhs); + if dfg.resolve(self.lhs) == dfg.resolve(self.rhs) { + return SimplifyResult::SimplifiedTo(self.lhs); + } + // b*(b*x) = b*x if b is boolean + if let super::Value::Instruction { instruction, .. } = &dfg[self.rhs] { + if let Instruction::Binary(Binary { lhs, rhs, operator }) = + dfg[*instruction] + { + if operator == BinaryOp::Mul + && (dfg.resolve(self.lhs) == dfg.resolve(lhs) + || dfg.resolve(self.lhs) == dfg.resolve(rhs)) + { + return SimplifyResult::SimplifiedTo(self.rhs); + } + } + } + } + // (b*x)*b = b*x if b is boolean + if dfg.get_value_max_num_bits(self.rhs) == 1 { + if let super::Value::Instruction { instruction, .. } = &dfg[self.lhs] { + if let Instruction::Binary(Binary { lhs, rhs, operator }) = + dfg[*instruction] + { + if operator == BinaryOp::Mul + && (dfg.resolve(self.rhs) == dfg.resolve(lhs) + || dfg.resolve(self.rhs) == dfg.resolve(rhs)) + { + return SimplifyResult::SimplifiedTo(self.lhs); + } + } + } } } BinaryOp::Div => { From 7cc8dbfe670aeaf9ec84f30265417658f5465d11 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 6 Jan 2025 20:33:09 -0300 Subject: [PATCH 11/11] feat: don't report warnings for dependencies (#6926) --- compiler/noirc_driver/src/lib.rs | 18 +++++++++++------- .../src/hir/def_collector/dc_crate.rs | 13 ++----------- compiler/noirc_frontend/src/hir/def_map/mod.rs | 8 +++++--- compiler/noirc_frontend/src/hir/mod.rs | 6 +++++- compiler/noirc_frontend/src/tests.rs | 2 -- 5 files changed, 23 insertions(+), 24 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 6b852354824..87fbd3d1d3d 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -311,13 +311,9 @@ pub fn check_crate( crate_id: CrateId, options: &CompileOptions, ) -> CompilationResult<()> { - let error_on_unused_imports = true; - let diagnostics = CrateDefMap::collect_defs( - crate_id, - context, - options.debug_comptime_in_file.as_deref(), - error_on_unused_imports, - ); + let diagnostics = + CrateDefMap::collect_defs(crate_id, context, options.debug_comptime_in_file.as_deref()); + let crate_files = context.crate_files(&crate_id); let warnings_and_errors: Vec = diagnostics .into_iter() .map(|(error, file_id)| { @@ -328,6 +324,14 @@ pub fn check_crate( // We filter out any warnings if they're going to be ignored later on to free up memory. !options.silence_warnings || diagnostic.diagnostic.kind != DiagnosticKind::Warning }) + .filter(|error| { + // Only keep warnings from the crate we are checking + if error.diagnostic.is_warning() { + crate_files.contains(&error.file_id) + } else { + true + } + }) .collect(); if has_errors(&warnings_and_errors, options.deny_warnings) { diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 33dab802b21..b59c09eccaa 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -275,7 +275,6 @@ impl DefCollector { ast: SortedModule, root_file_id: FileId, debug_comptime_in_file: Option<&str>, - error_on_unused_items: bool, ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; let crate_id = def_map.krate; @@ -288,13 +287,7 @@ impl DefCollector { let crate_graph = &context.crate_graph[crate_id]; for dep in crate_graph.dependencies.clone() { - let error_on_usage_tracker = false; - errors.extend(CrateDefMap::collect_defs( - dep.crate_id, - context, - debug_comptime_in_file, - error_on_usage_tracker, - )); + errors.extend(CrateDefMap::collect_defs(dep.crate_id, context, debug_comptime_in_file)); let dep_def_map = context.def_map(&dep.crate_id).expect("ice: def map was just created"); @@ -464,9 +457,7 @@ impl DefCollector { errors.append(&mut more_errors); - if error_on_unused_items { - Self::check_unused_items(context, crate_id, &mut errors); - } + Self::check_unused_items(context, crate_id, &mut errors); errors } diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index ffb885cc121..123bf3a910c 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -8,7 +8,7 @@ use crate::token::{FunctionAttribute, SecondaryAttribute, TestScope}; use fm::{FileId, FileManager}; use noirc_arena::{Arena, Index}; use noirc_errors::Location; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; mod module_def; pub use module_def::*; mod item_scope; @@ -78,7 +78,6 @@ impl CrateDefMap { crate_id: CrateId, context: &mut Context, debug_comptime_in_file: Option<&str>, - error_on_unused_imports: bool, ) -> Vec<(CompilationError, FileId)> { // Check if this Crate has already been compiled // XXX: There is probably a better alternative for this. @@ -121,7 +120,6 @@ impl CrateDefMap { ast, root_file_id, debug_comptime_in_file, - error_on_unused_imports, )); errors.extend( @@ -159,6 +157,10 @@ impl CrateDefMap { self.modules[module_id.0].location.file } + pub fn file_ids(&self) -> HashSet { + self.modules.iter().map(|(_, module_data)| module_data.location.file).collect() + } + /// Go through all modules in this crate, and find all functions in /// each module with the #[test] attribute pub fn get_all_test_functions<'a>( diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index 3342b3f8b50..b231f8c9698 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -19,7 +19,7 @@ use fm::{FileId, FileManager}; use iter_extended::vecmap; use noirc_errors::Location; use std::borrow::Cow; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::PathBuf; use std::rc::Rc; @@ -252,6 +252,10 @@ impl Context<'_, '_> { }) } + pub fn crate_files(&self, crate_id: &CrateId) -> HashSet { + self.def_maps.get(crate_id).map(|def_map| def_map.file_ids()).unwrap_or_default() + } + /// Activates LSP mode, which will track references for all definitions. pub fn activate_lsp_mode(&mut self) { self.def_interner.lsp_mode = true; diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 3377c260922..f29747431ea 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -114,7 +114,6 @@ pub(crate) fn get_program_with_maybe_parser_errors( }; let debug_comptime_in_file = None; - let error_on_unused_imports = true; // Now we want to populate the CrateDefMap using the DefCollector errors.extend(DefCollector::collect_crate_and_dependencies( @@ -123,7 +122,6 @@ pub(crate) fn get_program_with_maybe_parser_errors( program.clone().into_sorted(), root_file_id, debug_comptime_in_file, - error_on_unused_imports, )); } (program, context, errors)