From 4ff308128755c95b4d461bbcb7e3a49f16145585 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 29 Nov 2024 21:27:10 +0100 Subject: [PATCH 01/13] feat(ssa): Option to set the maximum acceptable Brillig bytecode increase in unrolling (#6641) --- Cargo.lock | 1 + compiler/noirc_driver/src/lib.rs | 11 +- compiler/noirc_evaluator/Cargo.toml | 1 + compiler/noirc_evaluator/src/brillig/mod.rs | 8 +- compiler/noirc_evaluator/src/ssa.rs | 19 ++- .../noirc_evaluator/src/ssa/ir/function.rs | 6 + .../noirc_evaluator/src/ssa/opt/unrolling.rs | 137 +++++++++++++++--- tooling/nargo_cli/build.rs | 5 + 8 files changed, 155 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 94a84b89d05..af91bafef52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3151,6 +3151,7 @@ dependencies = [ "serde_json", "serde_with", "similar-asserts", + "test-case", "thiserror", "tracing", ] diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 72ea464805f..a7cd9ff90ac 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -126,11 +126,19 @@ pub struct CompileOptions { #[arg(long)] pub skip_underconstrained_check: bool, - /// Setting to decide on an inlining strategy for brillig functions. + /// Setting to decide on an inlining strategy for Brillig functions. /// A more aggressive inliner should generate larger programs but more optimized /// A less aggressive inliner should generate smaller programs #[arg(long, hide = true, allow_hyphen_values = true, default_value_t = i64::MAX)] pub inliner_aggressiveness: i64, + + /// Setting the maximum acceptable increase in Brillig bytecode size due to + /// unrolling small loops. When left empty, any change is accepted as long + /// as it required fewer SSA instructions. + /// A higher value results in fewer jumps but a larger program. + /// A lower value keeps the original program if it was smaller, even if it has more jumps. + #[arg(long, hide = true, allow_hyphen_values = true)] + pub max_bytecode_increase_percent: Option, } pub fn parse_expression_width(input: &str) -> Result { @@ -589,6 +597,7 @@ pub fn compile_no_check( emit_ssa: if options.emit_ssa { Some(context.package_build_path.clone()) } else { None }, skip_underconstrained_check: options.skip_underconstrained_check, inliner_aggressiveness: options.inliner_aggressiveness, + max_bytecode_increase_percent: options.max_bytecode_increase_percent, }; let SsaProgramArtifact { program, debug, warnings, names, brillig_names, error_types, .. } = diff --git a/compiler/noirc_evaluator/Cargo.toml b/compiler/noirc_evaluator/Cargo.toml index e25b5bf855a..bb8c62cfd95 100644 --- a/compiler/noirc_evaluator/Cargo.toml +++ b/compiler/noirc_evaluator/Cargo.toml @@ -33,6 +33,7 @@ cfg-if.workspace = true proptest.workspace = true similar-asserts.workspace = true num-traits.workspace = true +test-case.workspace = true [features] bn254 = ["noirc_frontend/bn254"] diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 1b61ae1a864..cb8c35cd8e0 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -12,7 +12,7 @@ use self::{ }, }; use crate::ssa::{ - ir::function::{Function, FunctionId, RuntimeType}, + ir::function::{Function, FunctionId}, ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; @@ -59,7 +59,7 @@ impl std::ops::Index for Brillig { } impl Ssa { - /// Compile to brillig brillig functions and ACIR functions reachable from them + /// Compile Brillig functions and ACIR functions reachable from them #[tracing::instrument(level = "trace", skip_all)] pub(crate) fn to_brillig(&self, enable_debug_trace: bool) -> Brillig { // Collect all the function ids that are reachable from brillig @@ -67,9 +67,7 @@ impl Ssa { let brillig_reachable_function_ids = self .functions .iter() - .filter_map(|(id, func)| { - matches!(func.runtime(), RuntimeType::Brillig(_)).then_some(*id) - }) + .filter_map(|(id, func)| func.runtime().is_brillig().then_some(*id)) .collect::>(); let mut brillig = Brillig::default(); diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 97c1760d87c..80514b2f2cf 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -67,6 +67,11 @@ pub struct SsaEvaluatorOptions { /// The higher the value, the more inlined brillig functions will be. pub inliner_aggressiveness: i64, + + /// Maximum accepted percentage increase in the Brillig bytecode size after unrolling loops. + /// When `None` the size increase check is skipped altogether and any decrease in the SSA + /// instruction count is accepted. + pub max_bytecode_increase_percent: Option, } pub(crate) struct ArtifactsAndWarnings(Artifacts, Vec); @@ -104,7 +109,10 @@ pub(crate) fn optimize_into_acir( "After `static_assert` and `assert_constant`:", )? .run_pass(Ssa::loop_invariant_code_motion, "After Loop Invariant Code Motion:") - .try_run_pass(Ssa::unroll_loops_iteratively, "After Unrolling:")? + .try_run_pass( + |ssa| ssa.unroll_loops_iteratively(options.max_bytecode_increase_percent), + "After Unrolling:", + )? .run_pass(Ssa::simplify_cfg, "After Simplifying (2nd):") .run_pass(Ssa::flatten_cfg, "After Flattening:") .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:") @@ -450,11 +458,10 @@ impl SsaBuilder { } /// The same as `run_pass` but for passes that may fail - fn try_run_pass( - mut self, - pass: fn(Ssa) -> Result, - msg: &str, - ) -> Result { + fn try_run_pass(mut self, pass: F, msg: &str) -> Result + where + F: FnOnce(Ssa) -> Result, + { self.ssa = time(msg, self.print_codegen_timings, || pass(self.ssa))?; Ok(self.print(msg)) } diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index b1233e3063e..6413107c04a 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -197,6 +197,12 @@ impl Function { } } +impl Clone for Function { + fn clone(&self) -> Self { + Function::clone_with_id(self.id(), self) + } +} + impl std::fmt::Display for RuntimeType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 777c16dacd1..5883ce25936 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -19,8 +19,10 @@ //! When unrolling ACIR code, we remove reference count instructions because they are //! only used by Brillig bytecode. use acvm::{acir::AcirField, FieldElement}; +use im::HashSet; use crate::{ + brillig::brillig_gen::convert_ssa_function, errors::RuntimeError, ssa::{ ir::{ @@ -37,38 +39,60 @@ use crate::{ ssa_gen::Ssa, }, }; -use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use fxhash::FxHashMap as HashMap; impl Ssa { /// Loop unrolling can return errors, since ACIR functions need to be fully unrolled. /// This meta-pass will keep trying to unroll loops and simplifying the SSA until no more errors are found. - #[tracing::instrument(level = "trace", skip(ssa))] - pub(crate) fn unroll_loops_iteratively(mut ssa: Ssa) -> Result { - for (_, function) in ssa.functions.iter_mut() { + /// + /// The `max_bytecode_incr_pct`, when given, is used to limit the growth of the Brillig bytecode size + /// after unrolling small loops to some percentage of the original loop. For example a value of 150 would + /// mean the new loop can be 150% (ie. 2.5 times) larger than the original loop. It will still contain + /// fewer SSA instructions, but that can still result in more Brillig opcodes. + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn unroll_loops_iteratively( + mut self: Ssa, + max_bytecode_increase_percent: Option, + ) -> Result { + for (_, function) in self.functions.iter_mut() { + // Take a snapshot of the function to compare byte size increase, + // but only if the setting indicates we have to, otherwise skip it. + let orig_func_and_max_incr_pct = max_bytecode_increase_percent + .filter(|_| function.runtime().is_brillig()) + .map(|max_incr_pct| (function.clone(), max_incr_pct)); + // Try to unroll loops first: - let mut unroll_errors = function.try_unroll_loops(); + let (mut has_unrolled, mut unroll_errors) = function.try_unroll_loops(); // Keep unrolling until no more errors are found while !unroll_errors.is_empty() { let prev_unroll_err_count = unroll_errors.len(); // Simplify the SSA before retrying - - // Do a mem2reg after the last unroll to aid simplify_cfg - function.mem2reg(); - function.simplify_function(); - // Do another mem2reg after simplify_cfg to aid the next unroll - function.mem2reg(); + simplify_between_unrolls(function); // Unroll again - unroll_errors = function.try_unroll_loops(); + let (new_unrolled, new_errors) = function.try_unroll_loops(); + unroll_errors = new_errors; + has_unrolled |= new_unrolled; + // If we didn't manage to unroll any more loops, exit if unroll_errors.len() >= prev_unroll_err_count { return Err(unroll_errors.swap_remove(0)); } } + + if has_unrolled { + if let Some((orig_function, max_incr_pct)) = orig_func_and_max_incr_pct { + let new_size = brillig_bytecode_size(function); + let orig_size = brillig_bytecode_size(&orig_function); + if !is_new_size_ok(orig_size, new_size, max_incr_pct) { + *function = orig_function; + } + } + } } - Ok(ssa) + Ok(self) } } @@ -77,7 +101,7 @@ impl Function { // This can also be true for ACIR, but we have no alternative to unrolling in ACIR. // Brillig also generally prefers smaller code rather than faster code, // so we only attempt to unroll small loops, which we decide on a case-by-case basis. - fn try_unroll_loops(&mut self) -> Vec { + fn try_unroll_loops(&mut self) -> (bool, Vec) { Loops::find_all(self).unroll_each(self) } } @@ -170,8 +194,10 @@ impl Loops { /// Unroll all loops within a given function. /// Any loops which fail to be unrolled (due to using non-constant indices) will be unmodified. - fn unroll_each(mut self, function: &mut Function) -> Vec { + /// Returns whether any blocks have been modified + fn unroll_each(mut self, function: &mut Function) -> (bool, Vec) { let mut unroll_errors = vec![]; + let mut has_unrolled = false; while let Some(next_loop) = self.yet_to_unroll.pop() { if function.runtime().is_brillig() && !next_loop.is_small_loop(function, &self.cfg) { continue; @@ -181,13 +207,17 @@ impl Loops { if next_loop.blocks.iter().any(|block| self.modified_blocks.contains(block)) { let mut new_loops = Self::find_all(function); new_loops.failed_to_unroll = self.failed_to_unroll; - return unroll_errors.into_iter().chain(new_loops.unroll_each(function)).collect(); + let (new_unrolled, new_errors) = new_loops.unroll_each(function); + return (has_unrolled || new_unrolled, [unroll_errors, new_errors].concat()); } // Don't try to unroll the loop again if it is known to fail if !self.failed_to_unroll.contains(&next_loop.header) { match next_loop.unroll(function, &self.cfg) { - Ok(_) => self.modified_blocks.extend(next_loop.blocks), + Ok(_) => { + has_unrolled = true; + self.modified_blocks.extend(next_loop.blocks); + } Err(call_stack) => { self.failed_to_unroll.insert(next_loop.header); unroll_errors.push(RuntimeError::UnknownLoopBound { call_stack }); @@ -195,7 +225,7 @@ impl Loops { } } } - unroll_errors + (has_unrolled, unroll_errors) } } @@ -947,21 +977,59 @@ impl<'f> LoopIteration<'f> { } } +/// Unrolling leaves some duplicate instructions which can potentially be removed. +fn simplify_between_unrolls(function: &mut Function) { + // Do a mem2reg after the last unroll to aid simplify_cfg + function.mem2reg(); + function.simplify_function(); + // Do another mem2reg after simplify_cfg to aid the next unroll + function.mem2reg(); +} + +/// Convert the function to Brillig bytecode and return the resulting size. +fn brillig_bytecode_size(function: &Function) -> usize { + // We need to do some SSA passes in order for the conversion to be able to go ahead, + // otherwise we can hit `unreachable!()` instructions in `convert_ssa_instruction`. + // Creating a clone so as not to modify the originals. + let mut temp = function.clone(); + + // Might as well give it the best chance. + simplify_between_unrolls(&mut temp); + + // This is to try to prevent hitting ICE. + temp.dead_instruction_elimination(false); + + convert_ssa_function(&temp, false).byte_code.len() +} + +/// Decide if the new bytecode size is acceptable, compared to the original. +/// +/// The maximum increase can be expressed as a negative value if we demand a decrease. +/// (Values -100 and under mean the new size should be 0). +fn is_new_size_ok(orig_size: usize, new_size: usize, max_incr_pct: i32) -> bool { + let max_size_pct = 100i32.saturating_add(max_incr_pct).max(0) as usize; + let max_size = orig_size.saturating_mul(max_size_pct); + new_size.saturating_mul(100) <= max_size +} + #[cfg(test)] mod tests { use acvm::FieldElement; + use test_case::test_case; use crate::errors::RuntimeError; use crate::ssa::{ir::value::ValueId, opt::assert_normalized_ssa_equals, Ssa}; - use super::{BoilerplateStats, Loops}; + use super::{is_new_size_ok, BoilerplateStats, Loops}; - /// Tries to unroll all loops in each SSA function. + /// Tries to unroll all loops in each SSA function once, calling the `Function` directly, + /// bypassing the iterative loop done by the SSA which does further optimisations. + /// /// If any loop cannot be unrolled, it is left as-is or in a partially unrolled state. fn try_unroll_loops(mut ssa: Ssa) -> (Ssa, Vec) { let mut errors = vec![]; for function in ssa.functions.values_mut() { - errors.extend(function.try_unroll_loops()); + errors.extend(function.try_unroll_loops().1); } (ssa, errors) } @@ -1221,9 +1289,26 @@ mod tests { let (ssa, errors) = try_unroll_loops(ssa); assert_eq!(errors.len(), 0, "Unroll should have no errors"); + // Check that it's still the original assert_normalized_ssa_equals(ssa, parse_ssa().to_string().as_str()); } + #[test] + fn test_brillig_unroll_iteratively_respects_max_increase() { + let ssa = brillig_unroll_test_case(); + let ssa = ssa.unroll_loops_iteratively(Some(-90)).unwrap(); + // Check that it's still the original + assert_normalized_ssa_equals(ssa, brillig_unroll_test_case().to_string().as_str()); + } + + #[test] + fn test_brillig_unroll_iteratively_with_large_max_increase() { + let ssa = brillig_unroll_test_case(); + let ssa = ssa.unroll_loops_iteratively(Some(50)).unwrap(); + // Check that it did the unroll + assert_eq!(ssa.main().reachable_blocks().len(), 2, "The loop should be unrolled"); + } + /// Test that `break` and `continue` stop unrolling without any panic. #[test] fn test_brillig_unroll_break_and_continue() { @@ -1377,4 +1462,14 @@ mod tests { let loop0 = loops.yet_to_unroll.pop().expect("there should be a loop"); loop0.boilerplate_stats(function, &loops.cfg).expect("there should be stats") } + + #[test_case(1000, 700, 50, true; "size decreased")] + #[test_case(1000, 1500, 50, true; "size increased just by the max")] + #[test_case(1000, 1501, 50, false; "size increased over the max")] + #[test_case(1000, 700, -50, false; "size decreased but not enough")] + #[test_case(1000, 250, -50, true; "size decreased over expectations")] + #[test_case(1000, 250, -1250, false; "demanding more than minus 100 is handled")] + fn test_is_new_size_ok(old: usize, new: usize, max: i32, ok: bool) { + assert_eq!(is_new_size_ok(old, new, max), ok); + } } diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 740e5ed2052..f0334eaf713 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -213,8 +213,13 @@ 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()); + if force_brillig.0 {{ nargo.arg("--force-brillig"); + + // Set the maximum increase so that part of the optimization is exercised (it might fail). + nargo.arg("--max-bytecode-increase-percent"); + nargo.arg("50"); }} {test_content} From 59c0c3562ad75d6c10fa7c6a2f74e3fbf68ec3e6 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 2 Dec 2024 11:45:27 +0000 Subject: [PATCH 02/13] fix: always return an array of `u8`s when simplifying `Intrinsic::ToRadix` calls (#6663) --- .../src/ssa/ir/instruction/call.rs | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 67222d06ea8..874e72fc46b 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -60,7 +60,9 @@ pub(super) fn simplify_call( } else { unreachable!("ICE: Intrinsic::ToRadix return type must be array") }; - constant_to_radix(endian, field, 2, limb_count, dfg, block, call_stack) + constant_to_radix(endian, field, 2, limb_count, |values| { + make_constant_array(dfg, values.into_iter(), Type::bool(), block, call_stack) + }) } else { SimplifyResult::None } @@ -75,7 +77,15 @@ pub(super) fn simplify_call( } else { unreachable!("ICE: Intrinsic::ToRadix return type must be array") }; - constant_to_radix(endian, field, radix, limb_count, dfg, block, call_stack) + constant_to_radix(endian, field, radix, limb_count, |values| { + make_constant_array( + dfg, + values.into_iter(), + Type::unsigned(8), + block, + call_stack, + ) + }) } else { SimplifyResult::None } @@ -661,9 +671,7 @@ fn constant_to_radix( field: FieldElement, radix: u32, limb_count: u32, - dfg: &mut DataFlowGraph, - block: BasicBlockId, - call_stack: &CallStack, + mut make_array: impl FnMut(Vec) -> ValueId, ) -> SimplifyResult { let bit_size = u32::BITS - (radix - 1).leading_zeros(); let radix_big = BigUint::from(radix); @@ -684,13 +692,7 @@ fn constant_to_radix( if endian == Endian::Big { limbs.reverse(); } - let result_array = make_constant_array( - dfg, - limbs.into_iter(), - Type::unsigned(bit_size), - block, - call_stack, - ); + let result_array = make_array(limbs); SimplifyResult::SimplifiedTo(result_array) } } From de3e77a4acaab4bd2edc9a9e5226e54f468fd620 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 10:30:40 -0300 Subject: [PATCH 03/13] fix: use correct type for attribute arguments (#6640) --- .../noirc_frontend/src/elaborator/comptime.rs | 3 +-- .../src/tests/metaprogramming.rs | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index a27e2bf0163..962356d6dd9 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -329,8 +329,6 @@ impl<'context> Elaborator<'context> { push_arg(Value::TraitDefinition(trait_id)); } else { let (expr_id, expr_type) = interpreter.elaborator.elaborate_expression(arg); - push_arg(interpreter.evaluate(expr_id)?); - if let Err(UnificationError) = expr_type.unify(param_type) { return Err(InterpreterError::TypeMismatch { expected: param_type.clone(), @@ -338,6 +336,7 @@ impl<'context> Elaborator<'context> { location: arg_location, }); } + push_arg(interpreter.evaluate(expr_id)?); }; } diff --git a/compiler/noirc_frontend/src/tests/metaprogramming.rs b/compiler/noirc_frontend/src/tests/metaprogramming.rs index 82c40203244..89a049ebc9d 100644 --- a/compiler/noirc_frontend/src/tests/metaprogramming.rs +++ b/compiler/noirc_frontend/src/tests/metaprogramming.rs @@ -141,3 +141,23 @@ fn errors_if_macros_inject_functions_with_name_collisions() { ) if contents == "foo" )); } + +#[test] +fn uses_correct_type_for_attribute_arguments() { + let src = r#" + #[foo(32)] + comptime fn foo(_f: FunctionDefinition, i: u32) { + let y: u32 = 1; + let _ = y == i; + } + + #[bar([0; 2])] + comptime fn bar(_f: FunctionDefinition, i: [u32; 2]) { + let y: u32 = 1; + let _ = y == i[0]; + } + + fn main() {} + "#; + assert_no_errors(src); +} From 50f4aa72e409e7205724f90046d394bb83584e9c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 10:39:13 -0300 Subject: [PATCH 04/13] feat: allow filtering which SSA passes are printed (#6636) Co-authored-by: Akosh Farkash --- compiler/noirc_driver/src/lib.rs | 18 +++++- compiler/noirc_evaluator/src/ssa.rs | 86 +++++++++++++++++------------ 2 files changed, 66 insertions(+), 38 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index a7cd9ff90ac..e2237c5c0d9 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -13,7 +13,7 @@ use noirc_abi::{AbiParameter, AbiType, AbiValue}; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; use noirc_evaluator::create_program; use noirc_evaluator::errors::RuntimeError; -use noirc_evaluator::ssa::SsaProgramArtifact; +use noirc_evaluator::ssa::{SsaLogging, SsaProgramArtifact}; use noirc_frontend::debug::build_debug_crate_file; use noirc_frontend::hir::def_map::{Contract, CrateDefMap}; use noirc_frontend::hir::Context; @@ -70,6 +70,11 @@ pub struct CompileOptions { #[arg(long, hide = true)] pub show_ssa: bool, + /// Only show SSA passes whose name contains the provided string. + /// This setting takes precedence over `show_ssa` if it's not empty. + #[arg(long, hide = true)] + pub show_ssa_pass_name: Option, + /// Emit the unoptimized SSA IR to file. /// The IR will be dumped into the workspace target directory, /// under `[compiled-package].ssa.json`. @@ -585,7 +590,16 @@ pub fn compile_no_check( } let return_visibility = program.return_visibility; let ssa_evaluator_options = noirc_evaluator::ssa::SsaEvaluatorOptions { - enable_ssa_logging: options.show_ssa, + ssa_logging: match &options.show_ssa_pass_name { + Some(string) => SsaLogging::Contains(string.clone()), + None => { + if options.show_ssa { + SsaLogging::All + } else { + SsaLogging::None + } + } + }, enable_brillig_logging: options.show_brillig, force_brillig_output: options.force_brillig, print_codegen_timings: options.benchmark_codegen, diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 80514b2f2cf..a04791d764c 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -44,9 +44,16 @@ mod opt; pub(crate) mod parser; pub mod ssa_gen; +#[derive(Debug, Clone)] +pub enum SsaLogging { + None, + All, + Contains(String), +} + pub struct SsaEvaluatorOptions { /// Emit debug information for the intermediate SSA IR - pub enable_ssa_logging: bool, + pub ssa_logging: SsaLogging, pub enable_brillig_logging: bool, @@ -90,49 +97,49 @@ pub(crate) fn optimize_into_acir( let mut ssa = SsaBuilder::new( program, - options.enable_ssa_logging, + options.ssa_logging.clone(), options.force_brillig_output, options.print_codegen_timings, &options.emit_ssa, )? - .run_pass(Ssa::defunctionalize, "After Defunctionalization:") - .run_pass(Ssa::remove_paired_rc, "After Removing Paired rc_inc & rc_decs:") - .run_pass(Ssa::separate_runtime, "After Runtime Separation:") - .run_pass(Ssa::resolve_is_unconstrained, "After Resolving IsUnconstrained:") - .run_pass(|ssa| ssa.inline_functions(options.inliner_aggressiveness), "After Inlining (1st):") + .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, "After Mem2Reg (1st):") - .run_pass(Ssa::simplify_cfg, "After Simplifying (1st):") - .run_pass(Ssa::as_slice_optimization, "After `as_slice` optimization") + .run_pass(Ssa::mem2reg, "Mem2Reg (1st)") + .run_pass(Ssa::simplify_cfg, "Simplifying (1st)") + .run_pass(Ssa::as_slice_optimization, "`as_slice` optimization") .try_run_pass( Ssa::evaluate_static_assert_and_assert_constant, - "After `static_assert` and `assert_constant`:", + "`static_assert` and `assert_constant`", )? - .run_pass(Ssa::loop_invariant_code_motion, "After Loop Invariant Code Motion:") + .run_pass(Ssa::loop_invariant_code_motion, "Loop Invariant Code Motion") .try_run_pass( |ssa| ssa.unroll_loops_iteratively(options.max_bytecode_increase_percent), - "After Unrolling:", + "Unrolling", )? - .run_pass(Ssa::simplify_cfg, "After Simplifying (2nd):") - .run_pass(Ssa::flatten_cfg, "After Flattening:") - .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:") + .run_pass(Ssa::simplify_cfg, "Simplifying (2nd)") + .run_pass(Ssa::flatten_cfg, "Flattening") + .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores - .run_pass(Ssa::mem2reg, "After Mem2Reg (2nd):") + .run_pass(Ssa::mem2reg, "Mem2Reg (2nd)") // Run the inlining pass again to handle functions with `InlineType::NoPredicates`. // Before flattening is run, we treat functions marked with the `InlineType::NoPredicates` as an entry point. // This pass must come immediately following `mem2reg` as the succeeding passes // may create an SSA which inlining fails to handle. .run_pass( |ssa| ssa.inline_functions_with_no_predicates(options.inliner_aggressiveness), - "After Inlining (2nd):", + "Inlining (2nd)", ) - .run_pass(Ssa::remove_if_else, "After Remove IfElse:") - .run_pass(Ssa::fold_constants, "After Constant Folding:") - .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffectsIf removal:") - .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:") - .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") - .run_pass(Ssa::simplify_cfg, "After Simplifying:") - .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:") + .run_pass(Ssa::remove_if_else, "Remove IfElse") + .run_pass(Ssa::fold_constants, "Constant Folding") + .run_pass(Ssa::remove_enable_side_effects, "EnableSideEffectsIf removal") + .run_pass(Ssa::fold_constants_using_constraints, "Constraint Folding") + .run_pass(Ssa::dead_instruction_elimination, "Dead Instruction Elimination (1st)") + .run_pass(Ssa::simplify_cfg, "Simplifying:") + .run_pass(Ssa::array_set_optimization, "Array Set Optimizations") .finish(); let ssa_level_warnings = if options.skip_underconstrained_check { @@ -154,14 +161,11 @@ pub(crate) fn optimize_into_acir( let ssa = SsaBuilder { ssa, - print_ssa_passes: options.enable_ssa_logging, + ssa_logging: options.ssa_logging.clone(), print_codegen_timings: options.print_codegen_timings, } - .run_pass( - |ssa| ssa.fold_constants_with_brillig(&brillig), - "After Constant Folding with Brillig:", - ) - .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:") + .run_pass(|ssa| ssa.fold_constants_with_brillig(&brillig), "Inlining Brillig Calls Inlining") + .run_pass(Ssa::dead_instruction_elimination, "Dead Instruction Elimination (2nd)") .finish(); drop(ssa_gen_span_guard); @@ -419,14 +423,14 @@ fn split_public_and_private_inputs( // This is just a convenience object to bundle the ssa with `print_ssa_passes` for debug printing. struct SsaBuilder { ssa: Ssa, - print_ssa_passes: bool, + ssa_logging: SsaLogging, print_codegen_timings: bool, } impl SsaBuilder { fn new( program: Program, - print_ssa_passes: bool, + ssa_logging: SsaLogging, force_brillig_runtime: bool, print_codegen_timings: bool, emit_ssa: &Option, @@ -441,7 +445,7 @@ impl SsaBuilder { let ssa_path = emit_ssa.with_extension("ssa.json"); write_to_file(&serde_json::to_vec(&ssa).unwrap(), &ssa_path); } - Ok(SsaBuilder { print_ssa_passes, print_codegen_timings, ssa }.print("Initial SSA:")) + Ok(SsaBuilder { ssa_logging, print_codegen_timings, ssa }.print("Initial SSA:")) } fn finish(self) -> Ssa { @@ -467,9 +471,19 @@ impl SsaBuilder { } fn print(mut self, msg: &str) -> Self { - if self.print_ssa_passes { + let print_ssa_pass = match &self.ssa_logging { + SsaLogging::None => false, + SsaLogging::All => true, + SsaLogging::Contains(string) => { + let string = string.to_lowercase(); + let string = string.strip_prefix("after ").unwrap_or(&string); + let string = string.strip_suffix(':').unwrap_or(string); + msg.to_lowercase().contains(string) + } + }; + if print_ssa_pass { self.ssa.normalize_ids(); - println!("{msg}\n{}", self.ssa); + println!("After {msg}:\n{}", self.ssa); } self } From d80a9d71a8e4081a3aeacf34b2d5c2b0faf87484 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 11:04:54 -0300 Subject: [PATCH 05/13] fix: allow multiple `_` parameters, and disallow `_` as an expression you can read from (#6657) --- compiler/noirc_frontend/src/debug/mod.rs | 71 +++++++++++++------ compiler/noirc_frontend/src/elaborator/mod.rs | 3 + .../noirc_frontend/src/elaborator/patterns.rs | 22 +++--- .../src/hir/resolution/errors.rs | 20 ++++-- compiler/noirc_frontend/src/tests.rs | 30 ++++++++ 5 files changed, 110 insertions(+), 36 deletions(-) diff --git a/compiler/noirc_frontend/src/debug/mod.rs b/compiler/noirc_frontend/src/debug/mod.rs index fed3149118b..f05fc721581 100644 --- a/compiler/noirc_frontend/src/debug/mod.rs +++ b/compiler/noirc_frontend/src/debug/mod.rs @@ -67,12 +67,16 @@ impl DebugInstrumenter { self.insert_state_set_oracle(module, 8); } - fn insert_var(&mut self, var_name: &str) -> SourceVarId { + fn insert_var(&mut self, var_name: &str) -> Option { + if var_name == "_" { + return None; + } + let var_id = SourceVarId(self.next_var_id); self.next_var_id += 1; self.variables.insert(var_id, var_name.to_string()); self.scope.last_mut().unwrap().insert(var_name.to_string(), var_id); - var_id + Some(var_id) } fn lookup_var(&self, var_name: &str) -> Option { @@ -107,9 +111,9 @@ impl DebugInstrumenter { .flat_map(|param| { pattern_vars(¶m.pattern) .iter() - .map(|(id, _is_mut)| { - let var_id = self.insert_var(&id.0.contents); - build_assign_var_stmt(var_id, id_expr(id)) + .filter_map(|(id, _is_mut)| { + let var_id = self.insert_var(&id.0.contents)?; + Some(build_assign_var_stmt(var_id, id_expr(id))) }) .collect::>() }) @@ -225,13 +229,28 @@ impl DebugInstrumenter { } }) .collect(); - let vars_exprs: Vec = vars.iter().map(|(id, _)| id_expr(id)).collect(); + let vars_exprs: Vec = vars + .iter() + .map(|(id, _)| { + // We don't want to generate an expression to read from "_". + // And since this expression is going to be assigned to "_" so it doesn't matter + // what it is, we can use `()` for it. + if id.0.contents == "_" { + ast::Expression { + kind: ast::ExpressionKind::Literal(ast::Literal::Unit), + span: id.span(), + } + } else { + id_expr(id) + } + }) + .collect(); let mut block_stmts = vec![ast::Statement { kind: ast::StatementKind::Let(let_stmt.clone()), span: *span }]; - block_stmts.extend(vars.iter().map(|(id, _)| { - let var_id = self.insert_var(&id.0.contents); - build_assign_var_stmt(var_id, id_expr(id)) + block_stmts.extend(vars.iter().filter_map(|(id, _)| { + let var_id = self.insert_var(&id.0.contents)?; + Some(build_assign_var_stmt(var_id, id_expr(id))) })); block_stmts.push(ast::Statement { kind: ast::StatementKind::Expression(ast::Expression { @@ -422,21 +441,31 @@ impl DebugInstrumenter { let var_name = &for_stmt.identifier.0.contents; let var_id = self.insert_var(var_name); - let set_stmt = build_assign_var_stmt(var_id, id_expr(&for_stmt.identifier)); - let drop_stmt = build_drop_var_stmt(var_id, Span::empty(for_stmt.span.end())); + let set_and_drop_stmt = var_id.map(|var_id| { + ( + build_assign_var_stmt(var_id, id_expr(&for_stmt.identifier)), + build_drop_var_stmt(var_id, Span::empty(for_stmt.span.end())), + ) + }); self.walk_expr(&mut for_stmt.block); + + let mut statements = Vec::new(); + let block_statement = ast::Statement { + kind: ast::StatementKind::Semi(for_stmt.block.clone()), + span: for_stmt.block.span, + }; + + if let Some((set_stmt, drop_stmt)) = set_and_drop_stmt { + statements.push(set_stmt); + statements.push(block_statement); + statements.push(drop_stmt); + } else { + statements.push(block_statement); + } + for_stmt.block = ast::Expression { - kind: ast::ExpressionKind::Block(ast::BlockExpression { - statements: vec![ - set_stmt, - ast::Statement { - kind: ast::StatementKind::Semi(for_stmt.block.clone()), - span: for_stmt.block.span, - }, - drop_stmt, - ], - }), + kind: ast::ExpressionKind::Block(ast::BlockExpression { statements }), span: for_stmt.span, }; } diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 20d27fbc9ac..478504a79be 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -440,6 +440,9 @@ impl<'context> Elaborator<'context> { // so we need to reintroduce the same IDs into scope here. for parameter in &func_meta.parameter_idents { let name = self.interner.definition_name(parameter.id).to_owned(); + if name == "_" { + continue; + } let warn_if_unused = !(func_meta.trait_impl.is_some() && name == "self"); self.add_existing_variable_to_scope(name, parameter.clone(), warn_if_unused); } diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 3928362db11..3fbdadbbee8 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -331,16 +331,18 @@ impl<'context> Elaborator<'context> { let resolver_meta = ResolverMeta { num_times_used: 0, ident: ident.clone(), warn_if_unused }; - let scope = self.scopes.get_mut_scope(); - let old_value = scope.add_key_value(name.clone(), resolver_meta); - - if !allow_shadowing { - if let Some(old_value) = old_value { - self.push_err(ResolverError::DuplicateDefinition { - name, - first_span: old_value.ident.location.span, - second_span: location.span, - }); + if name != "_" { + let scope = self.scopes.get_mut_scope(); + let old_value = scope.add_key_value(name.clone(), resolver_meta); + + if !allow_shadowing { + if let Some(old_value) = old_value { + self.push_err(ResolverError::DuplicateDefinition { + name, + first_span: old_value.ident.location.span, + second_span: location.span, + }); + } } } diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index 80bd5247ee6..5c8e0a1b53e 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -223,11 +223,21 @@ impl<'a> From<&'a ResolverError> for Diagnostic { *span, ) } - ResolverError::VariableNotDeclared { name, span } => Diagnostic::simple_error( - format!("cannot find `{name}` in this scope "), - "not found in this scope".to_string(), - *span, - ), + ResolverError::VariableNotDeclared { name, span } => { + if name == "_" { + Diagnostic::simple_error( + "in expressions, `_` can only be used on the left-hand side of an assignment".to_string(), + "`_` not allowed here".to_string(), + *span, + ) + } else { + Diagnostic::simple_error( + format!("cannot find `{name}` in this scope"), + "not found in this scope".to_string(), + *span, + ) + } + }, ResolverError::PathIsNotIdent { span } => Diagnostic::simple_error( "cannot use path as an identifier".to_string(), String::new(), diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 605236c8dda..bfa8785e6b4 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3845,3 +3845,33 @@ fn disallows_export_attribute_on_trait_impl_method() { )); }); } + +#[test] +fn allows_multiple_underscore_parameters() { + let src = r#" + pub fn foo(_: i32, _: i64) {} + + fn main() {} + "#; + assert_no_errors(src); +} + +#[test] +fn disallows_underscore_on_right_hand_side() { + let src = r#" + fn main() { + let _ = 1; + let _x = _; + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::VariableNotDeclared { name, .. }) = + &errors[0].0 + else { + panic!("Expected a VariableNotDeclared error, got {:?}", errors[0].0); + }; + + assert_eq!(name, "_"); +} From ecaf63da19a3a76e7c2940721d9044b1980a588f Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:01:10 +0000 Subject: [PATCH 06/13] fix: correct signed integer handling in `noirc_abi` (#6638) Co-authored-by: Ary Borenszweig Co-authored-by: guipublic Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com> --- .../input_parser/json.txt | 7 +++ .../input_parser/toml.txt | 9 +++ tooling/noirc_abi/src/input_parser/json.rs | 47 ++++++++++++++- tooling/noirc_abi/src/input_parser/mod.rs | 57 ++++++++++++++++++- tooling/noirc_abi/src/input_parser/toml.rs | 47 ++++++++++++++- .../test/browser/abi_encode.test.ts | 5 +- .../test/node/abi_encode.test.ts | 5 +- .../noirc_abi_wasm/test/shared/abi_encode.ts | 4 +- 8 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 tooling/noirc_abi/proptest-regressions/input_parser/json.txt create mode 100644 tooling/noirc_abi/proptest-regressions/input_parser/toml.txt diff --git a/tooling/noirc_abi/proptest-regressions/input_parser/json.txt b/tooling/noirc_abi/proptest-regressions/input_parser/json.txt new file mode 100644 index 00000000000..19de8eeaf48 --- /dev/null +++ b/tooling/noirc_abi/proptest-regressions/input_parser/json.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc b3f9ae88d54944ca274764f4d99a2023d4b0ac09beb89bc599cbba1e45dd3620 # shrinks to (typ, value) = (Integer { sign: Signed, width: 1 }, -1) diff --git a/tooling/noirc_abi/proptest-regressions/input_parser/toml.txt b/tooling/noirc_abi/proptest-regressions/input_parser/toml.txt new file mode 100644 index 00000000000..1448cb67ef1 --- /dev/null +++ b/tooling/noirc_abi/proptest-regressions/input_parser/toml.txt @@ -0,0 +1,9 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 9d200afb8f5c01e3414d24eebe1436a7eef5377a46a9a9235aaa7f81e0b33656 # shrinks to (typ, value) = (Integer { sign: Signed, width: 8 }, -1) +cc 7fd29637e5566d819992185c1a95438e9949a555928a911b3918eed2e3f7a1fd # shrinks to (typ, value) = (Integer { sign: Signed, width: 64 }, -1) +cc 8ecbda39d887674b53ca23a861ac30fbb10c123bb70c57e69b336c86a3d9dea8 # shrinks to (abi, input_map) = (Abi { parameters: [AbiParameter { name: "¡", typ: Struct { path: "�)\u{1b}=�?Ⱥ\u{59424}?{\u{e4d5e}%Ѩ/Q\u{36a17}/*\";\u{b}&iC_\u{d313f}S\u{1b}\u{9dfec}\r/\u{10530d}", fields: [("?p*\"/\u{202e}\u{6f038}\u{537ca}.y@~𘛶?4\u{1b}*", Field), (".Ⱥ/$\u{7f}\u{103c06}%\\\u{202e}][0\u{88479}]\"*~\u{36fd5}\u{5}\u{feff}]{/", Tuple { fields: [String { length: 937 }] }), ("r\u{ac3a5}&:", Boolean), ("$d6🕴/:|�\u{37f8b}\r\u{a13b7}C$𲁹\\&\u{f8712}?\u{db61c}t%\u{57be1}\0", Field), ("/\u{6378b}\u{a426c}¥\u{7}/\u{fcb29}$\u{53c6b}\u{12d6f}\u{12bd3}.\u{f2f82}\u{8613e}*$\u{fd32f}\u{e29f7}\0𨺉'¬\"1", Struct { path: "\\\u{4a5ac}<\u{9e505}\u{4f3af}🕴&?<:^\u{7}\u{88}\u{3e1ff}(¥\u{531f3}K{:¥𦺀", fields: [("n\0Ѩ/\u{1b}𥐰\u{a4906}�¥`{\u{389d4}`1\u{7708a})\u{3dac4}8\u{93e5f}㒭\\\"\u{e6824}\u{b}Ѩ\u{88946}Ⱥ{", Integer { sign: Signed, width: 127 })] }), ("¥🕴\u{1b}¥🕴=sR\0\u{35f36}\u{867dc}>ä\u{202e}f:BȺ?:``*·¥\u{74ca5}\"", Tuple { fields: [Boolean, Field, String { length: 205 }, String { length: 575 }, Integer { sign: Signed, width: 124 }, String { length: 923 }, String { length: 294 }] })] }, visibility: Public }], return_type: None, error_types: {} }, {"¡": Struct({"$d6🕴/:|�\u{37f8b}\r\u{a13b7}C$𲁹\\&\u{f8712}?\u{db61c}t%\u{57be1}\0": Field(-8275115097504119425402713293372777967031130481426075481525511323101167533940), ".Ⱥ/$\u{7f}\u{103c06}%\\\u{202e}][0\u{88479}]\"*~\u{36fd5}\u{5}\u{feff}]{/": Vec([String("A \0A 0 aA0 a0aa00 A\000 0 \0\0aA\0\0a \0 \0a 0A\0A\0 Aa0aAA0A\0aa\00 0\0\0\0\0\00a Aa0 \0 a A0 \0AA0A Aa Aa\00aAaAaaA0A0 aA0 \0 Aa\00 \0000AAA a \0AAaaA\0\0a A0a0AA\0aA00 aA a0A\0AAa0a\0A0a\0\0A0A \00Aaaaa a A AO.*D\r.`bD4a\n*\u{15}\\B\"ace.8&A\t[AV8w<\u{18}\"\u{f}4`^Q\u{1b}U*$Z/\0\u{b}]qw${`\"=X&A\\\u{e}%`\\:\"$\u{1}.(6_C:\u{7}a`V=N**\u{1b})#Y\u{7f}#\u{b}$l\t}.Mns5!\t*$g\u{18}\rC\u{11}\"$=\u{7}.?&\u{1}yW\t.Y|<6\u{12}\u{e}/4JJ*&/V$`\"&`x#R\np\\%'*\n:P\0K\u{b}*`\r7Ym\t_\u{b}=$\u{16}`0v\u{7f}'NV^N4J<9=G*A:!b\u{1c}:'c{ST&z![\u{7f}/.={E*pmaWC\u{7f}7p{<\"']\u{8}?`\u{1b}\"\\\u{1}$\u{18}/!\u{16}-\t:E7CUs%_qw*xf.S\t\u{4}'=\"&%t'\u{1f}\u{7f}\u{b}$.=f\u{6}\"$A}xV_$\u{1a}nH\n\u{1b}?<&\n\u{15}U\\-b\u{1d}|\u{b}\u{2}t \rwA{L\u{11}\u{6}\u{10}\0\u{1b}G[x?&Yi?&7\u{b}?\r\u{1f}b\\$=\u{b}x& Q/\t\u{4}|X\"7\"{\0\0j'.\0\\e1zR.\u{c}\n<\u{b}Q*R+y8\u{19}(o\u{1f}@m\nt+\u{7f}Q\\+.Rn?\u{17}UZ\"$\u{b}/\0B=9=\t{\u{8}qZ&`!:D{\u{6}IO.H\u{7f}:?/3@\r\u{1b}oä\u{202e}f:BȺ?:``*·¥\u{74ca5}\"": Vec([Field(1), Field(8822392870083219098626030699076694602179106416928939583840848325203494062169), String("*TXn;{}\"_)_9\nk\\#ts\u{10}%\\c\n/2._::Oj*\u{7f}\0\r&PUMl\u{10}$/u?L}\u{7f}*P&<%=\u{7}S#%A\n \u{e}\\#v!\"\nepRp.{vH{&@\t\u{1f}\u{b}?=T\u{f}\"B\u{11}\n/{HY.\u{16}\n\nj<&\u{3}{f\n/9J*&x.$/,\r\0\u{1c}'\u{5}\u{13}\u{1b}`T\0`\n&/&\u{15}\u{b}w:{SK\u{7f}\\apR%/'0`0\n'd$$\u{7f}Vs\t<{\nDTT\\F\n\u{15}y.\\\t*-)&D$*u\u{b}\u{1b}?{\u{b}/\n\u{7f}0*.7\0\n:\u{b}.rSk<6~>{#"), String(".\"JA%q6i\ra/:F\u{16}?q<\t\rN\\13?H<;?{`\u{1d}p{.\"5?*@'N\"\u{1a}P,\u{1b}\u{7f}c+dt5':Y\u{1b}k/G>k/eM$XIX')\u{1b}'&\u{7f}\\\r\u{1b}`'P_.\n.?\0p`Y\u{c}`._\u{b}B\0\ng/*v$jfJ:\u{c}\u{1b}Pv}xn7ph@#{_<{.JD?r%'E\n7s9n/],u![;%*\u{2}{y`MgRdok8\"%<*>*{GyFJ}?\0W%#\0\u{1b}\u{7f}\u{16}G:\t=w\u{7f}:q\u{7f}:{k?\u{b}(:ca{$*1X/cw\u{1b}Z6I\rX\0\u{1b}(.^14\r\\=s\u{1b}w\u{3}F~\n\u{1e})/$0:=[\u{1},\\\\\tg\u{16}:],J`\0N\n\u{1b}\u{1b}\u{1b}{.xb\u{1a}\r'12#?e\\#/\tA\u{7f}\".\\Ke=\\?!v+P\u{17}\r\u{12}x.=A.`0<&?\niR/*WW\rnV)5vY.~\n _h\0&5f#\r\u{2}-S%\t s..\u{7f}!X}\"=\"?\u{5}y\u{4}`fr&R&d: 1Ht\"4`y_/S.71#{|%$%&ehy\u{16}J_\u{e}=:.%'\"N=J:\r:{&.\u{12}\u{b})&N\u{10}R_3;11\u{b}Qd<`<{?xF:~\"%<=<<\03:t??&\r;{\u{13}?__Y\u{6})\\k,vs?\n`G(*\n!\u{1b}[@z\0$?*yKLJh_\u{13}FkY'\\?T^\u{1f}$1n`'[\n\u{7f}\0+l\u{b}\u{1a}E\u{b}&(/\u{b}\rr\t:&\0+N'N:oC:*``IN\u{b}*.:\t$7+'*U:\t Result { let json_value = match (value, abi_type) { + (InputValue::Field(f), AbiType::Integer { sign: crate::Sign::Signed, width }) => { + JsonTypes::String(field_to_signed_hex(*f, *width)) + } (InputValue::Field(f), AbiType::Field | AbiType::Integer { .. }) => { JsonTypes::String(Self::format_field_string(*f)) } @@ -143,6 +146,9 @@ impl InputValue { ) -> Result { let input_value = match (value, param_type) { (JsonTypes::String(string), AbiType::String { .. }) => InputValue::String(string), + (JsonTypes::String(string), AbiType::Integer { sign: crate::Sign::Signed, width }) => { + InputValue::Field(parse_str_to_signed(&string, *width)?) + } ( JsonTypes::String(string), AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean, @@ -192,3 +198,40 @@ impl InputValue { Ok(input_value) } } + +#[cfg(test)] +mod test { + use proptest::prelude::*; + + use crate::{ + arbitrary::arb_abi_and_input_map, + input_parser::{arbitrary::arb_signed_integer_type_and_value, json::JsonTypes, InputValue}, + }; + + use super::{parse_json, serialize_to_json}; + + proptest! { + #[test] + fn serializing_and_parsing_returns_original_input((abi, input_map) in arb_abi_and_input_map()) { + let json = serialize_to_json(&input_map, &abi).expect("should be serializable"); + let parsed_input_map = parse_json(&json, &abi).expect("should be parsable"); + + prop_assert_eq!(parsed_input_map, input_map); + } + + #[test] + fn signed_integer_serialization_roundtrip((typ, value) in arb_signed_integer_type_and_value()) { + let string_input = JsonTypes::String(value.to_string()); + let input_value = InputValue::try_from_json(string_input, &typ, "foo").expect("should be parsable"); + let JsonTypes::String(output_string) = JsonTypes::try_from_input_value(&input_value, &typ).expect("should be serializable") else { + panic!("wrong type output"); + }; + let output_number = if let Some(output_string) = output_string.strip_prefix("-0x") { + -i64::from_str_radix(output_string, 16).unwrap() + } else { + i64::from_str_radix(output_string.strip_prefix("0x").unwrap(), 16).unwrap() + }; + prop_assert_eq!(output_number, value); + } + } +} diff --git a/tooling/noirc_abi/src/input_parser/mod.rs b/tooling/noirc_abi/src/input_parser/mod.rs index d7bbb0adfe3..b7732235eb2 100644 --- a/tooling/noirc_abi/src/input_parser/mod.rs +++ b/tooling/noirc_abi/src/input_parser/mod.rs @@ -248,6 +248,11 @@ mod serialization_tests { typ: AbiType::Field, visibility: AbiVisibility::Private, }, + AbiParameter { + name: "signed_example".into(), + typ: AbiType::Integer { sign: Sign::Signed, width: 8 }, + visibility: AbiVisibility::Private, + }, AbiParameter { name: "bar".into(), typ: AbiType::Struct { @@ -272,6 +277,7 @@ mod serialization_tests { let input_map: BTreeMap = BTreeMap::from([ ("foo".into(), InputValue::Field(FieldElement::one())), + ("signed_example".into(), InputValue::Field(FieldElement::from(240u128))), ( "bar".into(), InputValue::Struct(BTreeMap::from([ @@ -317,7 +323,9 @@ fn parse_str_to_field(value: &str) -> Result { } fn parse_str_to_signed(value: &str, width: u32) -> Result { - let big_num = if let Some(hex) = value.strip_prefix("0x") { + let big_num = if let Some(hex) = value.strip_prefix("-0x") { + BigInt::from_str_radix(hex, 16).map(|value| -value) + } else if let Some(hex) = value.strip_prefix("0x") { BigInt::from_str_radix(hex, 16) } else { BigInt::from_str_radix(value, 10) @@ -357,12 +365,23 @@ fn field_from_big_int(bigint: BigInt) -> FieldElement { } } +fn field_to_signed_hex(f: FieldElement, bit_size: u32) -> String { + let f_u128 = f.to_u128(); + let max = 2_u128.pow(bit_size - 1) - 1; + if f_u128 > max { + let f = FieldElement::from(2_u128.pow(bit_size) - f_u128); + format!("-0x{}", f.to_hex()) + } else { + format!("0x{}", f.to_hex()) + } +} + #[cfg(test)] mod test { use acvm::{AcirField, FieldElement}; use num_bigint::BigUint; - use super::parse_str_to_field; + use super::{parse_str_to_field, parse_str_to_signed}; fn big_uint_from_field(field: FieldElement) -> BigUint { BigUint::from_bytes_be(&field.to_be_bytes()) @@ -400,4 +419,38 @@ mod test { let noncanonical_field = FieldElement::modulus().to_string(); assert!(parse_str_to_field(&noncanonical_field).is_err()); } + + #[test] + fn test_parse_str_to_signed() { + let value = parse_str_to_signed("1", 8).unwrap(); + assert_eq!(value, FieldElement::from(1_u128)); + + let value = parse_str_to_signed("-1", 8).unwrap(); + assert_eq!(value, FieldElement::from(255_u128)); + + let value = parse_str_to_signed("-1", 16).unwrap(); + assert_eq!(value, FieldElement::from(65535_u128)); + } +} + +#[cfg(test)] +mod arbitrary { + use proptest::prelude::*; + + use crate::{AbiType, Sign}; + + pub(super) fn arb_signed_integer_type_and_value() -> BoxedStrategy<(AbiType, i64)> { + (2u32..=64) + .prop_flat_map(|width| { + let typ = Just(AbiType::Integer { width, sign: Sign::Signed }); + let value = if width == 64 { + // Avoid overflow + i64::MIN..i64::MAX + } else { + -(2i64.pow(width - 1))..(2i64.pow(width - 1) - 1) + }; + (typ, value) + }) + .boxed() + } } diff --git a/tooling/noirc_abi/src/input_parser/toml.rs b/tooling/noirc_abi/src/input_parser/toml.rs index 321d3511b5d..6f2be68a0c4 100644 --- a/tooling/noirc_abi/src/input_parser/toml.rs +++ b/tooling/noirc_abi/src/input_parser/toml.rs @@ -1,4 +1,4 @@ -use super::{parse_str_to_field, parse_str_to_signed, InputValue}; +use super::{field_to_signed_hex, parse_str_to_field, parse_str_to_signed, InputValue}; use crate::{errors::InputParserError, Abi, AbiType, MAIN_RETURN_NAME}; use acvm::{AcirField, FieldElement}; use iter_extended::{try_btree_map, try_vecmap}; @@ -60,7 +60,7 @@ pub(crate) fn serialize_to_toml( Ok(toml_string) } -#[derive(Debug, Deserialize, Serialize, Clone)] +#[derive(Debug, Deserialize, Serialize, Clone, PartialEq)] #[serde(untagged)] enum TomlTypes { // This is most likely going to be a hex string @@ -83,6 +83,9 @@ impl TomlTypes { abi_type: &AbiType, ) -> Result { let toml_value = match (value, abi_type) { + (InputValue::Field(f), AbiType::Integer { sign: crate::Sign::Signed, width }) => { + TomlTypes::String(field_to_signed_hex(*f, *width)) + } (InputValue::Field(f), AbiType::Field | AbiType::Integer { .. }) => { let f_str = format!("0x{}", f.to_hex()); TomlTypes::String(f_str) @@ -126,6 +129,7 @@ impl InputValue { ) -> Result { let input_value = match (value, param_type) { (TomlTypes::String(string), AbiType::String { .. }) => InputValue::String(string), + ( TomlTypes::String(string), AbiType::Field @@ -139,7 +143,7 @@ impl InputValue { TomlTypes::Integer(integer), AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean, ) => { - let new_value = FieldElement::from(i128::from(integer)); + let new_value = FieldElement::from(u128::from(integer)); InputValue::Field(new_value) } @@ -179,3 +183,40 @@ impl InputValue { Ok(input_value) } } + +#[cfg(test)] +mod test { + use proptest::prelude::*; + + use crate::{ + arbitrary::arb_abi_and_input_map, + input_parser::{arbitrary::arb_signed_integer_type_and_value, toml::TomlTypes, InputValue}, + }; + + use super::{parse_toml, serialize_to_toml}; + + proptest! { + #[test] + fn serializing_and_parsing_returns_original_input((abi, input_map) in arb_abi_and_input_map()) { + let toml = serialize_to_toml(&input_map, &abi).expect("should be serializable"); + let parsed_input_map = parse_toml(&toml, &abi).expect("should be parsable"); + + prop_assert_eq!(parsed_input_map, input_map); + } + + #[test] + fn signed_integer_serialization_roundtrip((typ, value) in arb_signed_integer_type_and_value()) { + let string_input = TomlTypes::String(value.to_string()); + let input_value = InputValue::try_from_toml(string_input.clone(), &typ, "foo").expect("should be parsable"); + let TomlTypes::String(output_string) = TomlTypes::try_from_input_value(&input_value, &typ).expect("should be serializable") else { + panic!("wrong type output"); + }; + let output_number = if let Some(output_string) = output_string.strip_prefix("-0x") { + -i64::from_str_radix(output_string, 16).unwrap() + } else { + i64::from_str_radix(output_string.strip_prefix("0x").unwrap(), 16).unwrap() + }; + prop_assert_eq!(output_number, value); + } + } +} diff --git a/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts b/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts index e1aaf0dc2c0..77903ea8ddd 100644 --- a/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts +++ b/tooling/noirc_abi_wasm/test/browser/abi_encode.test.ts @@ -15,7 +15,8 @@ it('recovers original inputs when abi encoding and decoding', async () => { const foo: Field = inputs.foo as Field; const bar: Field[] = inputs.bar as Field[]; expect(BigInt(decoded_inputs.inputs.foo)).to.be.equal(BigInt(foo)); - expect(BigInt(decoded_inputs.inputs.bar[0])).to.be.equal(BigInt(bar[0])); - expect(BigInt(decoded_inputs.inputs.bar[1])).to.be.equal(BigInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0])); + expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2])); expect(decoded_inputs.return_value).to.be.null; }); diff --git a/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts b/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts index a49c10b6ea6..4bb46f1ddd1 100644 --- a/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts +++ b/tooling/noirc_abi_wasm/test/node/abi_encode.test.ts @@ -11,7 +11,8 @@ it('recovers original inputs when abi encoding and decoding', async () => { const foo: Field = inputs.foo as Field; const bar: Field[] = inputs.bar as Field[]; expect(BigInt(decoded_inputs.inputs.foo)).to.be.equal(BigInt(foo)); - expect(BigInt(decoded_inputs.inputs.bar[0])).to.be.equal(BigInt(bar[0])); - expect(BigInt(decoded_inputs.inputs.bar[1])).to.be.equal(BigInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[0])).to.be.equal(parseInt(bar[0])); + expect(parseInt(decoded_inputs.inputs.bar[1])).to.be.equal(parseInt(bar[1])); + expect(parseInt(decoded_inputs.inputs.bar[2])).to.be.equal(parseInt(bar[2])); expect(decoded_inputs.return_value).to.be.null; }); diff --git a/tooling/noirc_abi_wasm/test/shared/abi_encode.ts b/tooling/noirc_abi_wasm/test/shared/abi_encode.ts index 62eb7658f43..b789bb05371 100644 --- a/tooling/noirc_abi_wasm/test/shared/abi_encode.ts +++ b/tooling/noirc_abi_wasm/test/shared/abi_encode.ts @@ -5,7 +5,7 @@ export const abi: Abi = { { name: 'foo', type: { kind: 'field' }, visibility: 'private' }, { name: 'bar', - type: { kind: 'array', length: 2, type: { kind: 'field' } }, + type: { kind: 'array', length: 3, type: { kind: 'integer', sign: 'signed', width: 32 } }, visibility: 'private', }, ], @@ -15,5 +15,5 @@ export const abi: Abi = { export const inputs: InputMap = { foo: '1', - bar: ['1', '2'], + bar: ['1', '2', '-1'], }; From f0c5fe95072d592934193c9a138ab9db34ca52be Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:21:20 +0000 Subject: [PATCH 07/13] chore: refactor foreign call executors (#6659) --- compiler/noirc_printable_type/src/lib.rs | 3 + tooling/acvm_cli/src/cli/execute_cmd.rs | 2 +- tooling/debugger/src/foreign_calls.rs | 2 +- tooling/nargo/src/foreign_calls/mocker.rs | 176 +++++++ tooling/nargo/src/foreign_calls/mod.rs | 146 ++++++ tooling/nargo/src/foreign_calls/print.rs | 36 ++ tooling/nargo/src/foreign_calls/rpc.rs | 227 ++++++++ tooling/nargo/src/lib.rs | 1 + tooling/nargo/src/ops/execute.rs | 3 +- tooling/nargo/src/ops/foreign_calls.rs | 494 ------------------ tooling/nargo/src/ops/mod.rs | 2 - tooling/nargo/src/ops/test.rs | 6 +- tooling/nargo_cli/benches/criterion.rs | 2 +- tooling/nargo_cli/src/cli/execute_cmd.rs | 2 +- tooling/nargo_cli/src/cli/info_cmd.rs | 2 +- tooling/nargo_cli/tests/stdlib-props.rs | 5 +- .../src/cli/execution_flamegraph_cmd.rs | 2 +- 17 files changed, 601 insertions(+), 510 deletions(-) create mode 100644 tooling/nargo/src/foreign_calls/mocker.rs create mode 100644 tooling/nargo/src/foreign_calls/mod.rs create mode 100644 tooling/nargo/src/foreign_calls/print.rs create mode 100644 tooling/nargo/src/foreign_calls/rpc.rs delete mode 100644 tooling/nargo/src/ops/foreign_calls.rs diff --git a/compiler/noirc_printable_type/src/lib.rs b/compiler/noirc_printable_type/src/lib.rs index 5ab04c6f576..838a2472125 100644 --- a/compiler/noirc_printable_type/src/lib.rs +++ b/compiler/noirc_printable_type/src/lib.rs @@ -69,6 +69,9 @@ pub enum PrintableValueDisplay { #[derive(Debug, Error)] pub enum ForeignCallError { + #[error("No handler could be found for foreign call `{0}`")] + NoHandler(String), + #[error("Foreign call inputs needed for execution are missing")] MissingForeignCallInputs, diff --git a/tooling/acvm_cli/src/cli/execute_cmd.rs b/tooling/acvm_cli/src/cli/execute_cmd.rs index c453936568c..bf5969718e5 100644 --- a/tooling/acvm_cli/src/cli/execute_cmd.rs +++ b/tooling/acvm_cli/src/cli/execute_cmd.rs @@ -8,7 +8,7 @@ use clap::Args; use crate::cli::fs::inputs::{read_bytecode_from_file, read_inputs_from_file}; use crate::errors::CliError; -use nargo::ops::{execute_program, DefaultForeignCallExecutor}; +use nargo::{foreign_calls::DefaultForeignCallExecutor, ops::execute_program}; use super::fs::witness::{create_output_witness_string, save_witness_to_dir}; diff --git a/tooling/debugger/src/foreign_calls.rs b/tooling/debugger/src/foreign_calls.rs index 6a773a4b348..ecf27a22f29 100644 --- a/tooling/debugger/src/foreign_calls.rs +++ b/tooling/debugger/src/foreign_calls.rs @@ -3,7 +3,7 @@ use acvm::{ pwg::ForeignCallWaitInfo, AcirField, FieldElement, }; -use nargo::ops::{DefaultForeignCallExecutor, ForeignCallExecutor}; +use nargo::foreign_calls::{DefaultForeignCallExecutor, ForeignCallExecutor}; use noirc_artifacts::debug::{DebugArtifact, DebugVars, StackFrame}; use noirc_errors::debug_info::{DebugFnId, DebugVarId}; use noirc_printable_type::ForeignCallError; diff --git a/tooling/nargo/src/foreign_calls/mocker.rs b/tooling/nargo/src/foreign_calls/mocker.rs new file mode 100644 index 00000000000..c93d16bbaf6 --- /dev/null +++ b/tooling/nargo/src/foreign_calls/mocker.rs @@ -0,0 +1,176 @@ +use acvm::{ + acir::brillig::{ForeignCallParam, ForeignCallResult}, + pwg::ForeignCallWaitInfo, + AcirField, +}; +use noirc_printable_type::{decode_string_value, ForeignCallError}; +use serde::{Deserialize, Serialize}; + +use super::{ForeignCall, ForeignCallExecutor}; + +/// This struct represents an oracle mock. It can be used for testing programs that use oracles. +#[derive(Debug, PartialEq, Eq, Clone)] +struct MockedCall { + /// The id of the mock, used to update or remove it + id: usize, + /// The oracle it's mocking + name: String, + /// Optionally match the parameters + params: Option>>, + /// The parameters with which the mock was last called + last_called_params: Option>>, + /// The result to return when this mock is called + result: ForeignCallResult, + /// How many times should this mock be called before it is removed + times_left: Option, +} + +impl MockedCall { + fn new(id: usize, name: String) -> Self { + Self { + id, + name, + params: None, + last_called_params: None, + result: ForeignCallResult { values: vec![] }, + times_left: None, + } + } +} + +impl MockedCall { + fn matches(&self, name: &str, params: &[ForeignCallParam]) -> bool { + self.name == name && (self.params.is_none() || self.params.as_deref() == Some(params)) + } +} + +#[derive(Debug, Default)] +pub(crate) struct MockForeignCallExecutor { + /// Mocks have unique ids used to identify them in Noir, allowing to update or remove them. + last_mock_id: usize, + /// The registered mocks + mocked_responses: Vec>, +} + +impl MockForeignCallExecutor { + fn extract_mock_id( + foreign_call_inputs: &[ForeignCallParam], + ) -> Result<(usize, &[ForeignCallParam]), ForeignCallError> { + let (id, params) = + foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; + let id = + usize::try_from(id.unwrap_field().try_to_u64().expect("value does not fit into u64")) + .expect("value does not fit into usize"); + Ok((id, params)) + } + + fn find_mock_by_id(&self, id: usize) -> Option<&MockedCall> { + self.mocked_responses.iter().find(|response| response.id == id) + } + + fn find_mock_by_id_mut(&mut self, id: usize) -> Option<&mut MockedCall> { + self.mocked_responses.iter_mut().find(|response| response.id == id) + } + + fn parse_string(param: &ForeignCallParam) -> String { + let fields: Vec<_> = param.fields().to_vec(); + decode_string_value(&fields) + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for MockForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::CreateMock) => { + let mock_oracle_name = Self::parse_string(&foreign_call.inputs[0]); + assert!(ForeignCall::lookup(&mock_oracle_name).is_none()); + let id = self.last_mock_id; + self.mocked_responses.push(MockedCall::new(id, mock_oracle_name)); + self.last_mock_id += 1; + + Ok(F::from(id).into()) + } + Some(ForeignCall::SetMockParams) => { + let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; + self.find_mock_by_id_mut(id) + .unwrap_or_else(|| panic!("Unknown mock id {}", id)) + .params = Some(params.to_vec()); + + Ok(ForeignCallResult::default()) + } + Some(ForeignCall::GetMockLastParams) => { + let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; + let mock = + self.find_mock_by_id(id).unwrap_or_else(|| panic!("Unknown mock id {}", id)); + + let last_called_params = mock + .last_called_params + .clone() + .unwrap_or_else(|| panic!("Mock {} was never called", mock.name)); + + Ok(last_called_params.into()) + } + Some(ForeignCall::SetMockReturns) => { + let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; + self.find_mock_by_id_mut(id) + .unwrap_or_else(|| panic!("Unknown mock id {}", id)) + .result = ForeignCallResult { values: params.to_vec() }; + + Ok(ForeignCallResult::default()) + } + Some(ForeignCall::SetMockTimes) => { + let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; + let times = + params[0].unwrap_field().try_to_u64().expect("Invalid bit size of times"); + + self.find_mock_by_id_mut(id) + .unwrap_or_else(|| panic!("Unknown mock id {}", id)) + .times_left = Some(times); + + Ok(ForeignCallResult::default()) + } + Some(ForeignCall::ClearMock) => { + let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; + self.mocked_responses.retain(|response| response.id != id); + Ok(ForeignCallResult::default()) + } + _ => { + let mock_response_position = self + .mocked_responses + .iter() + .position(|response| response.matches(foreign_call_name, &foreign_call.inputs)); + + if let Some(response_position) = mock_response_position { + // If the program has registered a mocked response to this oracle call then we prefer responding + // with that. + + let mock = self + .mocked_responses + .get_mut(response_position) + .expect("Invalid position of mocked response"); + + mock.last_called_params = Some(foreign_call.inputs.clone()); + + let result = mock.result.values.clone(); + + if let Some(times_left) = &mut mock.times_left { + *times_left -= 1; + if *times_left == 0 { + self.mocked_responses.remove(response_position); + } + } + + Ok(result.into()) + } else { + Err(ForeignCallError::NoHandler(foreign_call_name.to_string())) + } + } + } + } +} diff --git a/tooling/nargo/src/foreign_calls/mod.rs b/tooling/nargo/src/foreign_calls/mod.rs new file mode 100644 index 00000000000..0ef3247ee59 --- /dev/null +++ b/tooling/nargo/src/foreign_calls/mod.rs @@ -0,0 +1,146 @@ +use std::path::PathBuf; + +use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField}; +use mocker::MockForeignCallExecutor; +use noirc_printable_type::ForeignCallError; +use print::PrintForeignCallExecutor; +use rand::Rng; +use rpc::RPCForeignCallExecutor; +use serde::{Deserialize, Serialize}; + +mod mocker; +mod print; +mod rpc; + +pub trait ForeignCallExecutor { + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError>; +} + +/// This enumeration represents the Brillig foreign calls that are natively supported by nargo. +/// After resolution of a foreign call, nargo will restart execution of the ACVM +pub enum ForeignCall { + Print, + CreateMock, + SetMockParams, + GetMockLastParams, + SetMockReturns, + SetMockTimes, + ClearMock, +} + +impl std::fmt::Display for ForeignCall { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.name()) + } +} + +impl ForeignCall { + pub(crate) fn name(&self) -> &'static str { + match self { + ForeignCall::Print => "print", + ForeignCall::CreateMock => "create_mock", + ForeignCall::SetMockParams => "set_mock_params", + ForeignCall::GetMockLastParams => "get_mock_last_params", + ForeignCall::SetMockReturns => "set_mock_returns", + ForeignCall::SetMockTimes => "set_mock_times", + ForeignCall::ClearMock => "clear_mock", + } + } + + pub(crate) fn lookup(op_name: &str) -> Option { + match op_name { + "print" => Some(ForeignCall::Print), + "create_mock" => Some(ForeignCall::CreateMock), + "set_mock_params" => Some(ForeignCall::SetMockParams), + "get_mock_last_params" => Some(ForeignCall::GetMockLastParams), + "set_mock_returns" => Some(ForeignCall::SetMockReturns), + "set_mock_times" => Some(ForeignCall::SetMockTimes), + "clear_mock" => Some(ForeignCall::ClearMock), + _ => None, + } + } +} + +#[derive(Debug, Default)] +pub struct DefaultForeignCallExecutor { + /// The executor for any [`ForeignCall::Print`] calls. + printer: Option, + mocker: MockForeignCallExecutor, + external: Option, +} + +impl DefaultForeignCallExecutor { + pub fn new( + show_output: bool, + resolver_url: Option<&str>, + root_path: Option, + package_name: Option, + ) -> Self { + let id = rand::thread_rng().gen(); + let printer = if show_output { Some(PrintForeignCallExecutor) } else { None }; + let external_resolver = resolver_url.map(|resolver_url| { + RPCForeignCallExecutor::new(resolver_url, id, root_path, package_name) + }); + DefaultForeignCallExecutor { + printer, + mocker: MockForeignCallExecutor::default(), + external: external_resolver, + } + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for DefaultForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::Print) => { + if let Some(printer) = &mut self.printer { + printer.execute(foreign_call) + } else { + Ok(ForeignCallResult::default()) + } + } + Some( + ForeignCall::CreateMock + | ForeignCall::SetMockParams + | ForeignCall::GetMockLastParams + | ForeignCall::SetMockReturns + | ForeignCall::SetMockTimes + | ForeignCall::ClearMock, + ) => self.mocker.execute(foreign_call), + + None => { + // First check if there's any defined mock responses for this foreign call. + match self.mocker.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + + if let Some(external_resolver) = &mut self.external { + // If the user has registered an external resolver then we forward any remaining oracle calls there. + match external_resolver.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + } + + // If all executors have no handler for the given foreign call then we cannot + // return a correct response to the ACVM. The best we can do is to return an empty response, + // this allows us to ignore any foreign calls which exist solely to pass information from inside + // the circuit to the environment (e.g. custom logging) as the execution will still be able to progress. + // + // We optimistically return an empty response for all oracle calls as the ACVM will error + // should a response have been required. + Ok(ForeignCallResult::default()) + } + } + } +} diff --git a/tooling/nargo/src/foreign_calls/print.rs b/tooling/nargo/src/foreign_calls/print.rs new file mode 100644 index 00000000000..d0befa0bd0b --- /dev/null +++ b/tooling/nargo/src/foreign_calls/print.rs @@ -0,0 +1,36 @@ +use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField}; +use noirc_printable_type::{ForeignCallError, PrintableValueDisplay}; + +use super::{ForeignCall, ForeignCallExecutor}; + +#[derive(Debug, Default)] +pub(super) struct PrintForeignCallExecutor; + +impl ForeignCallExecutor for PrintForeignCallExecutor { + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::Print) => { + let skip_newline = foreign_call.inputs[0].unwrap_field().is_zero(); + + let foreign_call_inputs = foreign_call + .inputs + .split_first() + .ok_or(ForeignCallError::MissingForeignCallInputs)? + .1; + + let display_values: PrintableValueDisplay = foreign_call_inputs.try_into()?; + let display_string = + format!("{display_values}{}", if skip_newline { "" } else { "\n" }); + + print!("{display_string}"); + + Ok(ForeignCallResult::default()) + } + _ => Err(ForeignCallError::NoHandler(foreign_call_name.to_string())), + } + } +} diff --git a/tooling/nargo/src/foreign_calls/rpc.rs b/tooling/nargo/src/foreign_calls/rpc.rs new file mode 100644 index 00000000000..dab64819b56 --- /dev/null +++ b/tooling/nargo/src/foreign_calls/rpc.rs @@ -0,0 +1,227 @@ +use std::path::PathBuf; + +use acvm::{acir::brillig::ForeignCallResult, pwg::ForeignCallWaitInfo, AcirField}; +use jsonrpc::{arg as build_json_rpc_arg, minreq_http::Builder, Client}; +use noirc_printable_type::ForeignCallError; +use serde::{Deserialize, Serialize}; + +use super::ForeignCallExecutor; + +#[derive(Debug)] +pub(super) struct RPCForeignCallExecutor { + /// A randomly generated id for this `DefaultForeignCallExecutor`. + /// + /// This is used so that a single `external_resolver` can distinguish between requests from multiple + /// instantiations of `DefaultForeignCallExecutor`. + id: u64, + /// JSON RPC client to resolve foreign calls + external_resolver: Client, + /// Root path to the program or workspace in execution. + root_path: Option, + /// Name of the package in execution + package_name: Option, +} + +#[derive(Debug, Serialize, Deserialize)] +struct ResolveForeignCallRequest { + /// A session ID which allows the external RPC server to link this foreign call request to other foreign calls + /// for the same program execution. + /// + /// This is intended to allow a single RPC server to maintain state related to multiple program executions being + /// performed in parallel. + session_id: u64, + + #[serde(flatten)] + /// The foreign call which the external RPC server is to provide a response for. + function_call: ForeignCallWaitInfo, + + #[serde(skip_serializing_if = "Option::is_none")] + /// Root path to the program or workspace in execution. + root_path: Option, + #[serde(skip_serializing_if = "Option::is_none")] + /// Name of the package in execution + package_name: Option, +} + +impl RPCForeignCallExecutor { + pub(super) fn new( + resolver_url: &str, + id: u64, + root_path: Option, + package_name: Option, + ) -> Self { + let mut transport_builder = + Builder::new().url(resolver_url).expect("Invalid oracle resolver URL"); + + if let Some(Ok(timeout)) = + std::env::var("NARGO_FOREIGN_CALL_TIMEOUT").ok().map(|timeout| timeout.parse()) + { + let timeout_duration = std::time::Duration::from_millis(timeout); + transport_builder = transport_builder.timeout(timeout_duration); + }; + let oracle_resolver = Client::with_transport(transport_builder.build()); + + RPCForeignCallExecutor { external_resolver: oracle_resolver, id, root_path, package_name } + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for RPCForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + let encoded_params = vec![build_json_rpc_arg(ResolveForeignCallRequest { + session_id: self.id, + function_call: foreign_call.clone(), + root_path: self.root_path.clone().map(|path| path.to_str().unwrap().to_string()), + package_name: self.package_name.clone(), + })]; + + let req = self.external_resolver.build_request("resolve_foreign_call", &encoded_params); + + let response = self.external_resolver.send_request(req)?; + + let parsed_response: ForeignCallResult = response.result()?; + + Ok(parsed_response) + } +} + +#[cfg(test)] +mod tests { + use acvm::{ + acir::brillig::ForeignCallParam, brillig_vm::brillig::ForeignCallResult, + pwg::ForeignCallWaitInfo, FieldElement, + }; + use jsonrpc_core::Result as RpcResult; + use jsonrpc_derive::rpc; + use jsonrpc_http_server::{Server, ServerBuilder}; + + use super::{ForeignCallExecutor, RPCForeignCallExecutor, ResolveForeignCallRequest}; + + #[allow(unreachable_pub)] + #[rpc] + pub trait OracleResolver { + #[rpc(name = "resolve_foreign_call")] + fn resolve_foreign_call( + &self, + req: ResolveForeignCallRequest, + ) -> RpcResult>; + } + + struct OracleResolverImpl; + + impl OracleResolverImpl { + fn echo(&self, param: ForeignCallParam) -> ForeignCallResult { + vec![param].into() + } + + fn sum(&self, array: ForeignCallParam) -> ForeignCallResult { + let mut res: FieldElement = 0_usize.into(); + + for value in array.fields() { + res += value; + } + + res.into() + } + } + + impl OracleResolver for OracleResolverImpl { + fn resolve_foreign_call( + &self, + req: ResolveForeignCallRequest, + ) -> RpcResult> { + let response = match req.function_call.function.as_str() { + "sum" => self.sum(req.function_call.inputs[0].clone()), + "echo" => self.echo(req.function_call.inputs[0].clone()), + "id" => FieldElement::from(req.session_id as u128).into(), + + _ => panic!("unexpected foreign call"), + }; + Ok(response) + } + } + + fn build_oracle_server() -> (Server, String) { + let mut io = jsonrpc_core::IoHandler::new(); + io.extend_with(OracleResolverImpl.to_delegate()); + + // Choosing port 0 results in a random port being assigned. + let server = ServerBuilder::new(io) + .start_http(&"127.0.0.1:0".parse().expect("Invalid address")) + .expect("Could not start server"); + + let url = format!("http://{}", server.address()); + (server, url) + } + + #[test] + fn test_oracle_resolver_echo() { + let (server, url) = build_oracle_server(); + + let mut executor = RPCForeignCallExecutor::new(&url, 1, None, None); + + let foreign_call: ForeignCallWaitInfo = ForeignCallWaitInfo { + function: "echo".to_string(), + inputs: vec![ForeignCallParam::Single(1_u128.into())], + }; + + let result = executor.execute(&foreign_call); + assert_eq!(result.unwrap(), ForeignCallResult { values: foreign_call.inputs }); + + server.close(); + } + + #[test] + fn test_oracle_resolver_sum() { + let (server, url) = build_oracle_server(); + + let mut executor = RPCForeignCallExecutor::new(&url, 2, None, None); + + let foreign_call: ForeignCallWaitInfo = ForeignCallWaitInfo { + function: "sum".to_string(), + inputs: vec![ForeignCallParam::Array(vec![1_usize.into(), 2_usize.into()])], + }; + + let result = executor.execute(&foreign_call); + assert_eq!(result.unwrap(), FieldElement::from(3_usize).into()); + + server.close(); + } + + #[test] + fn foreign_call_executor_id_is_persistent() { + let (server, url) = build_oracle_server(); + + let mut executor = RPCForeignCallExecutor::new(&url, 3, None, None); + + let foreign_call: ForeignCallWaitInfo = + ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; + + let result_1 = executor.execute(&foreign_call).unwrap(); + let result_2 = executor.execute(&foreign_call).unwrap(); + assert_eq!(result_1, result_2); + + server.close(); + } + + #[test] + fn oracle_resolver_rpc_can_distinguish_executors() { + let (server, url) = build_oracle_server(); + + let mut executor_1 = RPCForeignCallExecutor::new(&url, 4, None, None); + let mut executor_2 = RPCForeignCallExecutor::new(&url, 5, None, None); + + let foreign_call: ForeignCallWaitInfo = + ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; + + let result_1 = executor_1.execute(&foreign_call).unwrap(); + let result_2 = executor_2.execute(&foreign_call).unwrap(); + assert_ne!(result_1, result_2); + + server.close(); + } +} diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index 88f07e0c292..74b7f54d860 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -9,6 +9,7 @@ pub mod constants; pub mod errors; +pub mod foreign_calls; pub mod ops; pub mod package; pub mod workspace; diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index 09ef554d2aa..57116ec2efd 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -10,10 +10,9 @@ use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; use acvm::{AcirField, BlackBoxFunctionSolver}; use crate::errors::ExecutionError; +use crate::foreign_calls::ForeignCallExecutor; use crate::NargoError; -use super::foreign_calls::ForeignCallExecutor; - struct ProgramExecutor<'a, F, B: BlackBoxFunctionSolver, E: ForeignCallExecutor> { functions: &'a [Circuit], diff --git a/tooling/nargo/src/ops/foreign_calls.rs b/tooling/nargo/src/ops/foreign_calls.rs deleted file mode 100644 index 30785949a46..00000000000 --- a/tooling/nargo/src/ops/foreign_calls.rs +++ /dev/null @@ -1,494 +0,0 @@ -use std::path::PathBuf; - -use acvm::{ - acir::brillig::{ForeignCallParam, ForeignCallResult}, - pwg::ForeignCallWaitInfo, - AcirField, -}; -use jsonrpc::{arg as build_json_rpc_arg, minreq_http::Builder, Client}; -use noirc_printable_type::{decode_string_value, ForeignCallError, PrintableValueDisplay}; -use rand::Rng; -use serde::{Deserialize, Serialize}; - -pub trait ForeignCallExecutor { - fn execute( - &mut self, - foreign_call: &ForeignCallWaitInfo, - ) -> Result, ForeignCallError>; -} - -/// This enumeration represents the Brillig foreign calls that are natively supported by nargo. -/// After resolution of a foreign call, nargo will restart execution of the ACVM -pub enum ForeignCall { - Print, - CreateMock, - SetMockParams, - GetMockLastParams, - SetMockReturns, - SetMockTimes, - ClearMock, -} - -impl std::fmt::Display for ForeignCall { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.name()) - } -} - -impl ForeignCall { - pub(crate) fn name(&self) -> &'static str { - match self { - ForeignCall::Print => "print", - ForeignCall::CreateMock => "create_mock", - ForeignCall::SetMockParams => "set_mock_params", - ForeignCall::GetMockLastParams => "get_mock_last_params", - ForeignCall::SetMockReturns => "set_mock_returns", - ForeignCall::SetMockTimes => "set_mock_times", - ForeignCall::ClearMock => "clear_mock", - } - } - - pub(crate) fn lookup(op_name: &str) -> Option { - match op_name { - "print" => Some(ForeignCall::Print), - "create_mock" => Some(ForeignCall::CreateMock), - "set_mock_params" => Some(ForeignCall::SetMockParams), - "get_mock_last_params" => Some(ForeignCall::GetMockLastParams), - "set_mock_returns" => Some(ForeignCall::SetMockReturns), - "set_mock_times" => Some(ForeignCall::SetMockTimes), - "clear_mock" => Some(ForeignCall::ClearMock), - _ => None, - } - } -} - -/// This struct represents an oracle mock. It can be used for testing programs that use oracles. -#[derive(Debug, PartialEq, Eq, Clone)] -struct MockedCall { - /// The id of the mock, used to update or remove it - id: usize, - /// The oracle it's mocking - name: String, - /// Optionally match the parameters - params: Option>>, - /// The parameters with which the mock was last called - last_called_params: Option>>, - /// The result to return when this mock is called - result: ForeignCallResult, - /// How many times should this mock be called before it is removed - times_left: Option, -} - -impl MockedCall { - fn new(id: usize, name: String) -> Self { - Self { - id, - name, - params: None, - last_called_params: None, - result: ForeignCallResult { values: vec![] }, - times_left: None, - } - } -} - -impl MockedCall { - fn matches(&self, name: &str, params: &[ForeignCallParam]) -> bool { - self.name == name && (self.params.is_none() || self.params.as_deref() == Some(params)) - } -} - -#[derive(Debug, Default)] -pub struct DefaultForeignCallExecutor { - /// A randomly generated id for this `DefaultForeignCallExecutor`. - /// - /// This is used so that a single `external_resolver` can distinguish between requests from multiple - /// instantiations of `DefaultForeignCallExecutor`. - id: u64, - - /// Mocks have unique ids used to identify them in Noir, allowing to update or remove them. - last_mock_id: usize, - /// The registered mocks - mocked_responses: Vec>, - /// Whether to print [`ForeignCall::Print`] output. - show_output: bool, - /// JSON RPC client to resolve foreign calls - external_resolver: Option, - /// Root path to the program or workspace in execution. - root_path: Option, - /// Name of the package in execution - package_name: Option, -} - -#[derive(Debug, Serialize, Deserialize)] -struct ResolveForeignCallRequest { - /// A session ID which allows the external RPC server to link this foreign call request to other foreign calls - /// for the same program execution. - /// - /// This is intended to allow a single RPC server to maintain state related to multiple program executions being - /// performed in parallel. - session_id: u64, - - #[serde(flatten)] - /// The foreign call which the external RPC server is to provide a response for. - function_call: ForeignCallWaitInfo, - - #[serde(skip_serializing_if = "Option::is_none")] - /// Root path to the program or workspace in execution. - root_path: Option, - #[serde(skip_serializing_if = "Option::is_none")] - /// Name of the package in execution - package_name: Option, -} - -impl DefaultForeignCallExecutor { - pub fn new( - show_output: bool, - resolver_url: Option<&str>, - root_path: Option, - package_name: Option, - ) -> Self { - let oracle_resolver = resolver_url.map(|resolver_url| { - let mut transport_builder = - Builder::new().url(resolver_url).expect("Invalid oracle resolver URL"); - - if let Some(Ok(timeout)) = - std::env::var("NARGO_FOREIGN_CALL_TIMEOUT").ok().map(|timeout| timeout.parse()) - { - let timeout_duration = std::time::Duration::from_millis(timeout); - transport_builder = transport_builder.timeout(timeout_duration); - }; - Client::with_transport(transport_builder.build()) - }); - DefaultForeignCallExecutor { - show_output, - external_resolver: oracle_resolver, - id: rand::thread_rng().gen(), - mocked_responses: Vec::new(), - last_mock_id: 0, - root_path, - package_name, - } - } -} - -impl DefaultForeignCallExecutor { - fn extract_mock_id( - foreign_call_inputs: &[ForeignCallParam], - ) -> Result<(usize, &[ForeignCallParam]), ForeignCallError> { - let (id, params) = - foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?; - let id = - usize::try_from(id.unwrap_field().try_to_u64().expect("value does not fit into u64")) - .expect("value does not fit into usize"); - Ok((id, params)) - } - - fn find_mock_by_id(&self, id: usize) -> Option<&MockedCall> { - self.mocked_responses.iter().find(|response| response.id == id) - } - - fn find_mock_by_id_mut(&mut self, id: usize) -> Option<&mut MockedCall> { - self.mocked_responses.iter_mut().find(|response| response.id == id) - } - - fn parse_string(param: &ForeignCallParam) -> String { - let fields: Vec<_> = param.fields().to_vec(); - decode_string_value(&fields) - } - - fn execute_print(foreign_call_inputs: &[ForeignCallParam]) -> Result<(), ForeignCallError> { - let skip_newline = foreign_call_inputs[0].unwrap_field().is_zero(); - - let foreign_call_inputs = - foreign_call_inputs.split_first().ok_or(ForeignCallError::MissingForeignCallInputs)?.1; - let display_string = Self::format_printable_value(foreign_call_inputs, skip_newline)?; - - print!("{display_string}"); - - Ok(()) - } - - fn format_printable_value( - foreign_call_inputs: &[ForeignCallParam], - skip_newline: bool, - ) -> Result { - let display_values: PrintableValueDisplay = foreign_call_inputs.try_into()?; - - let result = format!("{display_values}{}", if skip_newline { "" } else { "\n" }); - - Ok(result) - } -} - -impl Deserialize<'a>> ForeignCallExecutor - for DefaultForeignCallExecutor -{ - fn execute( - &mut self, - foreign_call: &ForeignCallWaitInfo, - ) -> Result, ForeignCallError> { - let foreign_call_name = foreign_call.function.as_str(); - match ForeignCall::lookup(foreign_call_name) { - Some(ForeignCall::Print) => { - if self.show_output { - Self::execute_print(&foreign_call.inputs)?; - } - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::CreateMock) => { - let mock_oracle_name = Self::parse_string(&foreign_call.inputs[0]); - assert!(ForeignCall::lookup(&mock_oracle_name).is_none()); - let id = self.last_mock_id; - self.mocked_responses.push(MockedCall::new(id, mock_oracle_name)); - self.last_mock_id += 1; - - Ok(F::from(id).into()) - } - Some(ForeignCall::SetMockParams) => { - let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; - self.find_mock_by_id_mut(id) - .unwrap_or_else(|| panic!("Unknown mock id {}", id)) - .params = Some(params.to_vec()); - - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::GetMockLastParams) => { - let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; - let mock = - self.find_mock_by_id(id).unwrap_or_else(|| panic!("Unknown mock id {}", id)); - - let last_called_params = mock - .last_called_params - .clone() - .unwrap_or_else(|| panic!("Mock {} was never called", mock.name)); - - Ok(last_called_params.into()) - } - Some(ForeignCall::SetMockReturns) => { - let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; - self.find_mock_by_id_mut(id) - .unwrap_or_else(|| panic!("Unknown mock id {}", id)) - .result = ForeignCallResult { values: params.to_vec() }; - - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::SetMockTimes) => { - let (id, params) = Self::extract_mock_id(&foreign_call.inputs)?; - let times = - params[0].unwrap_field().try_to_u64().expect("Invalid bit size of times"); - - self.find_mock_by_id_mut(id) - .unwrap_or_else(|| panic!("Unknown mock id {}", id)) - .times_left = Some(times); - - Ok(ForeignCallResult::default()) - } - Some(ForeignCall::ClearMock) => { - let (id, _) = Self::extract_mock_id(&foreign_call.inputs)?; - self.mocked_responses.retain(|response| response.id != id); - Ok(ForeignCallResult::default()) - } - None => { - let mock_response_position = self - .mocked_responses - .iter() - .position(|response| response.matches(foreign_call_name, &foreign_call.inputs)); - - if let Some(response_position) = mock_response_position { - // If the program has registered a mocked response to this oracle call then we prefer responding - // with that. - - let mock = self - .mocked_responses - .get_mut(response_position) - .expect("Invalid position of mocked response"); - - mock.last_called_params = Some(foreign_call.inputs.clone()); - - let result = mock.result.values.clone(); - - if let Some(times_left) = &mut mock.times_left { - *times_left -= 1; - if *times_left == 0 { - self.mocked_responses.remove(response_position); - } - } - - Ok(result.into()) - } else if let Some(external_resolver) = &self.external_resolver { - // If the user has registered an external resolver then we forward any remaining oracle calls there. - - let encoded_params = vec![build_json_rpc_arg(ResolveForeignCallRequest { - session_id: self.id, - function_call: foreign_call.clone(), - root_path: self - .root_path - .clone() - .map(|path| path.to_str().unwrap().to_string()), - package_name: self.package_name.clone(), - })]; - - let req = - external_resolver.build_request("resolve_foreign_call", &encoded_params); - - let response = external_resolver.send_request(req)?; - - let parsed_response: ForeignCallResult = response.result()?; - - Ok(parsed_response) - } else { - // If there's no registered mock oracle response and no registered resolver then we cannot - // return a correct response to the ACVM. The best we can do is to return an empty response, - // this allows us to ignore any foreign calls which exist solely to pass information from inside - // the circuit to the environment (e.g. custom logging) as the execution will still be able to progress. - // - // We optimistically return an empty response for all oracle calls as the ACVM will error - // should a response have been required. - Ok(ForeignCallResult::default()) - } - } - } - } -} - -#[cfg(test)] -mod tests { - use acvm::{ - acir::brillig::ForeignCallParam, brillig_vm::brillig::ForeignCallResult, - pwg::ForeignCallWaitInfo, FieldElement, - }; - use jsonrpc_core::Result as RpcResult; - use jsonrpc_derive::rpc; - use jsonrpc_http_server::{Server, ServerBuilder}; - - use crate::ops::{DefaultForeignCallExecutor, ForeignCallExecutor}; - - use super::ResolveForeignCallRequest; - - #[allow(unreachable_pub)] - #[rpc] - pub trait OracleResolver { - #[rpc(name = "resolve_foreign_call")] - fn resolve_foreign_call( - &self, - req: ResolveForeignCallRequest, - ) -> RpcResult>; - } - - struct OracleResolverImpl; - - impl OracleResolverImpl { - fn echo(&self, param: ForeignCallParam) -> ForeignCallResult { - vec![param].into() - } - - fn sum(&self, array: ForeignCallParam) -> ForeignCallResult { - let mut res: FieldElement = 0_usize.into(); - - for value in array.fields() { - res += value; - } - - res.into() - } - } - - impl OracleResolver for OracleResolverImpl { - fn resolve_foreign_call( - &self, - req: ResolveForeignCallRequest, - ) -> RpcResult> { - let response = match req.function_call.function.as_str() { - "sum" => self.sum(req.function_call.inputs[0].clone()), - "echo" => self.echo(req.function_call.inputs[0].clone()), - "id" => FieldElement::from(req.session_id as u128).into(), - - _ => panic!("unexpected foreign call"), - }; - Ok(response) - } - } - - fn build_oracle_server() -> (Server, String) { - let mut io = jsonrpc_core::IoHandler::new(); - io.extend_with(OracleResolverImpl.to_delegate()); - - // Choosing port 0 results in a random port being assigned. - let server = ServerBuilder::new(io) - .start_http(&"127.0.0.1:0".parse().expect("Invalid address")) - .expect("Could not start server"); - - let url = format!("http://{}", server.address()); - (server, url) - } - - #[test] - fn test_oracle_resolver_echo() { - let (server, url) = build_oracle_server(); - - let mut executor = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { - function: "echo".to_string(), - inputs: vec![ForeignCallParam::Single(1_u128.into())], - }; - - let result = executor.execute(&foreign_call); - assert_eq!(result.unwrap(), ForeignCallResult { values: foreign_call.inputs }); - - server.close(); - } - - #[test] - fn test_oracle_resolver_sum() { - let (server, url) = build_oracle_server(); - - let mut executor = DefaultForeignCallExecutor::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { - function: "sum".to_string(), - inputs: vec![ForeignCallParam::Array(vec![1_usize.into(), 2_usize.into()])], - }; - - let result = executor.execute(&foreign_call); - assert_eq!(result.unwrap(), FieldElement::from(3_usize).into()); - - server.close(); - } - - #[test] - fn foreign_call_executor_id_is_persistent() { - let (server, url) = build_oracle_server(); - - let mut executor = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; - - let result_1 = executor.execute(&foreign_call).unwrap(); - let result_2 = executor.execute(&foreign_call).unwrap(); - assert_eq!(result_1, result_2); - - server.close(); - } - - #[test] - fn oracle_resolver_rpc_can_distinguish_executors() { - let (server, url) = build_oracle_server(); - - let mut executor_1 = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - let mut executor_2 = - DefaultForeignCallExecutor::::new(false, Some(&url), None, None); - - let foreign_call = ForeignCallWaitInfo { function: "id".to_string(), inputs: Vec::new() }; - - let result_1 = executor_1.execute(&foreign_call).unwrap(); - let result_2 = executor_2.execute(&foreign_call).unwrap(); - assert_ne!(result_1, result_2); - - server.close(); - } -} diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index f70577a14f1..04efeb5a9ec 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -4,7 +4,6 @@ pub use self::compile::{ compile_workspace, report_errors, }; pub use self::execute::{execute_program, execute_program_with_profiling}; -pub use self::foreign_calls::{DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor}; pub use self::optimize::{optimize_contract, optimize_program}; pub use self::transform::{transform_contract, transform_program}; @@ -13,7 +12,6 @@ pub use self::test::{run_test, TestStatus}; mod check; mod compile; mod execute; -mod foreign_calls; mod optimize; mod test; mod transform; diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 370a4235f61..51d7b8c55aa 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -9,9 +9,11 @@ use noirc_driver::{compile_no_check, CompileError, CompileOptions}; use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; use noirc_frontend::hir::{def_map::TestFunction, Context}; -use crate::{errors::try_to_diagnose_runtime_error, NargoError}; +use crate::{ + errors::try_to_diagnose_runtime_error, foreign_calls::DefaultForeignCallExecutor, NargoError, +}; -use super::{execute_program, DefaultForeignCallExecutor}; +use super::execute_program; pub enum TestStatus { Pass, diff --git a/tooling/nargo_cli/benches/criterion.rs b/tooling/nargo_cli/benches/criterion.rs index 488cbfcd243..51de97df139 100644 --- a/tooling/nargo_cli/benches/criterion.rs +++ b/tooling/nargo_cli/benches/criterion.rs @@ -115,7 +115,7 @@ fn criterion_test_execution(c: &mut Criterion, test_program_dir: &Path, force_br let artifacts = RefCell::new(None); let mut foreign_call_executor = - nargo::ops::DefaultForeignCallExecutor::new(false, None, None, None); + nargo::foreign_calls::DefaultForeignCallExecutor::new(false, None, None, None); c.bench_function(&benchmark_name, |b| { b.iter_batched( diff --git a/tooling/nargo_cli/src/cli/execute_cmd.rs b/tooling/nargo_cli/src/cli/execute_cmd.rs index 8dc71b1c7e5..fa95d3123c6 100644 --- a/tooling/nargo_cli/src/cli/execute_cmd.rs +++ b/tooling/nargo_cli/src/cli/execute_cmd.rs @@ -7,7 +7,7 @@ use clap::Args; use nargo::constants::PROVER_INPUT_FILE; use nargo::errors::try_to_diagnose_runtime_error; -use nargo::ops::DefaultForeignCallExecutor; +use nargo::foreign_calls::DefaultForeignCallExecutor; use nargo::package::{CrateName, Package}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::input_parser::{Format, InputValue}; diff --git a/tooling/nargo_cli/src/cli/info_cmd.rs b/tooling/nargo_cli/src/cli/info_cmd.rs index cf416b1fa5f..769a1f79d81 100644 --- a/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/tooling/nargo_cli/src/cli/info_cmd.rs @@ -4,7 +4,7 @@ use clap::Args; use iter_extended::vecmap; use nargo::{ constants::PROVER_INPUT_FILE, - ops::DefaultForeignCallExecutor, + foreign_calls::DefaultForeignCallExecutor, package::{CrateName, Package}, }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; diff --git a/tooling/nargo_cli/tests/stdlib-props.rs b/tooling/nargo_cli/tests/stdlib-props.rs index 0013a90b4ff..370d2c04052 100644 --- a/tooling/nargo_cli/tests/stdlib-props.rs +++ b/tooling/nargo_cli/tests/stdlib-props.rs @@ -2,10 +2,7 @@ use std::{cell::RefCell, collections::BTreeMap, path::Path}; use acvm::{acir::native_types::WitnessStack, AcirField, FieldElement}; use iter_extended::vecmap; -use nargo::{ - ops::{execute_program, DefaultForeignCallExecutor}, - parse_all, -}; +use nargo::{foreign_calls::DefaultForeignCallExecutor, ops::execute_program, parse_all}; use noirc_abi::input_parser::InputValue; use noirc_driver::{ compile_main, file_manager_with_stdlib, prepare_crate, CompilationResult, CompileOptions, diff --git a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs index 981d08a3eb1..6d6da89f660 100644 --- a/tooling/profiler/src/cli/execution_flamegraph_cmd.rs +++ b/tooling/profiler/src/cli/execution_flamegraph_cmd.rs @@ -8,7 +8,7 @@ use crate::flamegraph::{BrilligExecutionSample, FlamegraphGenerator, InfernoFlam use crate::fs::{read_inputs_from_file, read_program_from_file}; use crate::opcode_formatter::format_brillig_opcode; use bn254_blackbox_solver::Bn254BlackBoxSolver; -use nargo::ops::DefaultForeignCallExecutor; +use nargo::foreign_calls::DefaultForeignCallExecutor; use noirc_abi::input_parser::Format; use noirc_artifacts::debug::DebugArtifact; From e3a0914c6dede6f54f426ed7d790a0c98a7e0908 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:50:49 +0000 Subject: [PATCH 08/13] feat: allow ignoring test failures from foreign calls (#6660) --- .github/workflows/test-js-packages.yml | 30 +++-- tooling/lsp/src/requests/test_run.rs | 5 + tooling/nargo/src/foreign_calls/mod.rs | 6 +- tooling/nargo/src/foreign_calls/print.rs | 2 +- tooling/nargo/src/foreign_calls/rpc.rs | 4 +- tooling/nargo/src/ops/test.rs | 146 +++++++++++++++++++++-- tooling/nargo_cli/src/cli/test_cmd.rs | 6 + tooling/nargo_cli/tests/stdlib-tests.rs | 6 + 8 files changed, 180 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test-js-packages.yml b/.github/workflows/test-js-packages.yml index 4a5d0b8179b..422a30ed08f 100644 --- a/.github/workflows/test-js-packages.yml +++ b/.github/workflows/test-js-packages.yml @@ -519,12 +519,25 @@ jobs: fail-fast: false matrix: project: - # Disabled as these are currently failing with many visibility errors - - { repo: AztecProtocol/aztec-nr, path: ./ } + - { repo: noir-lang/ec, path: ./ } + - { repo: noir-lang/eddsa, path: ./ } + - { repo: noir-lang/mimc, path: ./ } + - { repo: noir-lang/noir_sort, path: ./ } + - { repo: noir-lang/noir-edwards, path: ./ } + - { repo: noir-lang/noir-bignum, path: ./ } + - { repo: noir-lang/noir_bigcurve, path: ./ } + - { repo: noir-lang/noir_base64, path: ./ } + - { repo: noir-lang/noir_string_search, path: ./ } + - { repo: noir-lang/sparse_array, path: ./ } + - { repo: noir-lang/noir_rsa, path: ./lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/aztec-nr } - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts } - # Disabled as aztec-packages requires a setup-step in order to generate a `Nargo.toml` - #- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits } - - { repo: noir-lang/noir-edwards, path: ./, ref: 3188ea74fe3b059219a2ea87899589c266256d74 } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/parity-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/private-kernel-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/reset-kernel-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/rollup-lib } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits/crates/types } + name: Check external repo - ${{ matrix.project.repo }} steps: - name: Checkout @@ -554,9 +567,12 @@ jobs: # Github actions seems to not expand "**" in globs by default. shopt -s globstar sed -i '/^compiler_version/d' ./**/Nargo.toml - - name: Run nargo check + + - name: Run nargo test working-directory: ./test-repo/${{ matrix.project.path }} - run: nargo check + run: nargo test --silence-warnings + env: + NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true # This is a job which depends on all test jobs and reports the overall status. # This allows us to add/remove test jobs without having to update the required workflows. diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 50c699bb6a6..937fdcc0a5e 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -101,6 +101,11 @@ fn on_test_run_request_inner( result: "fail".to_string(), message: Some(message), }, + TestStatus::Skipped => NargoTestRunResult { + id: params.id.clone(), + result: "skipped".to_string(), + message: None, + }, TestStatus::CompileError(diag) => NargoTestRunResult { id: params.id.clone(), result: "error".to_string(), diff --git a/tooling/nargo/src/foreign_calls/mod.rs b/tooling/nargo/src/foreign_calls/mod.rs index 0ef3247ee59..16ed71e11e3 100644 --- a/tooling/nargo/src/foreign_calls/mod.rs +++ b/tooling/nargo/src/foreign_calls/mod.rs @@ -8,9 +8,9 @@ use rand::Rng; use rpc::RPCForeignCallExecutor; use serde::{Deserialize, Serialize}; -mod mocker; -mod print; -mod rpc; +pub(crate) mod mocker; +pub(crate) mod print; +pub(crate) mod rpc; pub trait ForeignCallExecutor { fn execute( diff --git a/tooling/nargo/src/foreign_calls/print.rs b/tooling/nargo/src/foreign_calls/print.rs index d0befa0bd0b..92fcd65ae28 100644 --- a/tooling/nargo/src/foreign_calls/print.rs +++ b/tooling/nargo/src/foreign_calls/print.rs @@ -4,7 +4,7 @@ use noirc_printable_type::{ForeignCallError, PrintableValueDisplay}; use super::{ForeignCall, ForeignCallExecutor}; #[derive(Debug, Default)] -pub(super) struct PrintForeignCallExecutor; +pub(crate) struct PrintForeignCallExecutor; impl ForeignCallExecutor for PrintForeignCallExecutor { fn execute( diff --git a/tooling/nargo/src/foreign_calls/rpc.rs b/tooling/nargo/src/foreign_calls/rpc.rs index dab64819b56..0653eb1c7e3 100644 --- a/tooling/nargo/src/foreign_calls/rpc.rs +++ b/tooling/nargo/src/foreign_calls/rpc.rs @@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize}; use super::ForeignCallExecutor; #[derive(Debug)] -pub(super) struct RPCForeignCallExecutor { +pub(crate) struct RPCForeignCallExecutor { /// A randomly generated id for this `DefaultForeignCallExecutor`. /// /// This is used so that a single `external_resolver` can distinguish between requests from multiple @@ -44,7 +44,7 @@ struct ResolveForeignCallRequest { } impl RPCForeignCallExecutor { - pub(super) fn new( + pub(crate) fn new( resolver_url: &str, id: u64, root_path: Option, diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 51d7b8c55aa..6e7a7d1b7db 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -1,16 +1,28 @@ use std::path::PathBuf; use acvm::{ - acir::native_types::{WitnessMap, WitnessStack}, - BlackBoxFunctionSolver, FieldElement, + acir::{ + brillig::ForeignCallResult, + native_types::{WitnessMap, WitnessStack}, + }, + pwg::ForeignCallWaitInfo, + AcirField, BlackBoxFunctionSolver, FieldElement, }; use noirc_abi::Abi; use noirc_driver::{compile_no_check, CompileError, CompileOptions}; use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; use noirc_frontend::hir::{def_map::TestFunction, Context}; +use noirc_printable_type::ForeignCallError; +use rand::Rng; +use serde::{Deserialize, Serialize}; use crate::{ - errors::try_to_diagnose_runtime_error, foreign_calls::DefaultForeignCallExecutor, NargoError, + errors::try_to_diagnose_runtime_error, + foreign_calls::{ + mocker::MockForeignCallExecutor, print::PrintForeignCallExecutor, + rpc::RPCForeignCallExecutor, DefaultForeignCallExecutor, ForeignCall, ForeignCallExecutor, + }, + NargoError, }; use super::execute_program; @@ -18,12 +30,13 @@ use super::execute_program; pub enum TestStatus { Pass, Fail { message: String, error_diagnostic: Option }, + Skipped, CompileError(FileDiagnostic), } impl TestStatus { pub fn failed(&self) -> bool { - !matches!(self, TestStatus::Pass) + !matches!(self, TestStatus::Pass | TestStatus::Skipped) } } @@ -50,23 +63,42 @@ pub fn run_test>( if test_function_has_no_arguments { // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen, // otherwise constraints involving these expressions will not error. + let mut foreign_call_executor = TestForeignCallExecutor::new( + show_output, + foreign_call_resolver_url, + root_path, + package_name, + ); + let circuit_execution = execute_program( &compiled_program.program, WitnessMap::new(), blackbox_solver, - &mut DefaultForeignCallExecutor::new( - show_output, - foreign_call_resolver_url, - root_path, - package_name, - ), + &mut foreign_call_executor, ); - test_status_program_compile_pass( + + let status = test_status_program_compile_pass( test_function, compiled_program.abi, compiled_program.debug, circuit_execution, - ) + ); + + let ignore_foreign_call_failures = + std::env::var("NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS") + .is_ok_and(|var| &var == "true"); + + if let TestStatus::Fail { .. } = status { + if ignore_foreign_call_failures + && foreign_call_executor.encountered_unknown_foreign_call + { + TestStatus::Skipped + } else { + status + } + } else { + status + } } else { #[cfg(target_arch = "wasm32")] { @@ -217,3 +249,93 @@ fn check_expected_failure_message( error_diagnostic, } } + +/// A specialized foreign call executor which tracks whether it has encountered any unknown foreign calls +struct TestForeignCallExecutor { + /// The executor for any [`ForeignCall::Print`] calls. + printer: Option, + mocker: MockForeignCallExecutor, + external: Option, + + encountered_unknown_foreign_call: bool, +} + +impl TestForeignCallExecutor { + fn new( + show_output: bool, + resolver_url: Option<&str>, + root_path: Option, + package_name: Option, + ) -> Self { + let id = rand::thread_rng().gen(); + let printer = if show_output { Some(PrintForeignCallExecutor) } else { None }; + let external_resolver = resolver_url.map(|resolver_url| { + RPCForeignCallExecutor::new(resolver_url, id, root_path, package_name) + }); + TestForeignCallExecutor { + printer, + mocker: MockForeignCallExecutor::default(), + external: external_resolver, + encountered_unknown_foreign_call: false, + } + } +} + +impl Deserialize<'a>> ForeignCallExecutor + for TestForeignCallExecutor +{ + fn execute( + &mut self, + foreign_call: &ForeignCallWaitInfo, + ) -> Result, ForeignCallError> { + // If the circuit has reached a new foreign call opcode then it can't have failed from any previous unknown foreign calls. + self.encountered_unknown_foreign_call = false; + + let foreign_call_name = foreign_call.function.as_str(); + match ForeignCall::lookup(foreign_call_name) { + Some(ForeignCall::Print) => { + if let Some(printer) = &mut self.printer { + printer.execute(foreign_call) + } else { + Ok(ForeignCallResult::default()) + } + } + + Some( + ForeignCall::CreateMock + | ForeignCall::SetMockParams + | ForeignCall::GetMockLastParams + | ForeignCall::SetMockReturns + | ForeignCall::SetMockTimes + | ForeignCall::ClearMock, + ) => self.mocker.execute(foreign_call), + + None => { + // First check if there's any defined mock responses for this foreign call. + match self.mocker.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + + if let Some(external_resolver) = &mut self.external { + // If the user has registered an external resolver then we forward any remaining oracle calls there. + match external_resolver.execute(foreign_call) { + Err(ForeignCallError::NoHandler(_)) => (), + response_or_error => return response_or_error, + }; + } + + self.encountered_unknown_foreign_call = true; + + // If all executors have no handler for the given foreign call then we cannot + // return a correct response to the ACVM. The best we can do is to return an empty response, + // this allows us to ignore any foreign calls which exist solely to pass information from inside + // the circuit to the environment (e.g. custom logging) as the execution will still be able to progress. + // + // We optimistically return an empty response for all oracle calls as the ACVM will error + // should a response have been required. + Ok(ForeignCallResult::default()) + } + } + } +} diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 7b0201226ef..aa0ee1bb94b 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -255,6 +255,12 @@ fn display_test_report( ); } } + TestStatus::Skipped { .. } => { + writer + .set_color(ColorSpec::new().set_fg(Some(Color::Yellow))) + .expect("Failed to set color"); + writeln!(writer, "skipped").expect("Failed to write to stderr"); + } TestStatus::CompileError(err) => { noirc_errors::reporter::report_all( file_manager.as_file_map(), diff --git a/tooling/nargo_cli/tests/stdlib-tests.rs b/tooling/nargo_cli/tests/stdlib-tests.rs index bdc92e625ab..99f0c9a2e7f 100644 --- a/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/tooling/nargo_cli/tests/stdlib-tests.rs @@ -138,6 +138,12 @@ fn display_test_report( ); } } + TestStatus::Skipped { .. } => { + writer + .set_color(ColorSpec::new().set_fg(Some(Color::Yellow))) + .expect("Failed to set color"); + writeln!(writer, "skipped").expect("Failed to write to stderr"); + } TestStatus::CompileError(err) => { noirc_errors::reporter::report_all( file_manager.as_file_map(), From 6acef6d795cd74dee4a21e82c5a912d58b40b06c Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Mon, 2 Dec 2024 17:11:52 +0100 Subject: [PATCH 09/13] feat: add memory report into the CI (#6630) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .github/workflows/memory_report.yml | 88 +++++++++++++++++++++++++++++ cspell.json | 1 + test_programs/memory_report.sh | 48 ++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 .github/workflows/memory_report.yml create mode 100755 test_programs/memory_report.sh diff --git a/.github/workflows/memory_report.yml b/.github/workflows/memory_report.yml new file mode 100644 index 00000000000..a1b28aee31d --- /dev/null +++ b/.github/workflows/memory_report.yml @@ -0,0 +1,88 @@ +name: Report Peak Memory + +on: + push: + branches: + - master + pull_request: + +jobs: + build-nargo: + runs-on: ubuntu-latest + strategy: + matrix: + target: [x86_64-unknown-linux-gnu] + + steps: + - name: Checkout Noir repo + uses: actions/checkout@v4 + + - name: Setup toolchain + uses: dtolnay/rust-toolchain@1.74.1 + + - uses: Swatinem/rust-cache@v2 + with: + key: ${{ matrix.target }} + cache-on-failure: true + save-if: ${{ github.event_name != 'merge_group' }} + + - name: Build Nargo + run: cargo build --package nargo_cli --release + + - name: Package artifacts + run: | + mkdir dist + cp ./target/release/nargo ./dist/nargo + + - name: Upload artifact + uses: actions/upload-artifact@v4 + with: + name: nargo + path: ./dist/* + retention-days: 3 + + generate_memory_report: + needs: [build-nargo] + runs-on: ubuntu-latest + permissions: + pull-requests: write + + steps: + - uses: actions/checkout@v4 + + - name: Download nargo binary + uses: actions/download-artifact@v4 + with: + name: nargo + path: ./nargo + + - name: Set nargo on PATH + run: | + nargo_binary="${{ github.workspace }}/nargo/nargo" + chmod +x $nargo_binary + echo "$(dirname $nargo_binary)" >> $GITHUB_PATH + export PATH="$PATH:$(dirname $nargo_binary)" + nargo -V + + - name: Generate Memory report + working-directory: ./test_programs + run: | + chmod +x memory_report.sh + ./memory_report.sh + mv memory_report.json ../memory_report.json + + - name: Parse memory report + id: memory_report + uses: noir-lang/noir-bench-report@d61bc78ece3c8df1e72914b3d5136bad403d5bdf + with: + report: memory_report.json + header: | + # Memory Report + memory_report: true + + - name: Add memory report to sticky comment + if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target' + uses: marocchino/sticky-pull-request-comment@v2 + with: + header: memory + message: ${{ steps.memory_report.outputs.markdown }} \ No newline at end of file diff --git a/cspell.json b/cspell.json index 36bba737cd7..15bba2cb5f8 100644 --- a/cspell.json +++ b/cspell.json @@ -106,6 +106,7 @@ "Guillaume", "gzipped", "hasher", + "heaptrack", "hexdigit", "higher-kinded", "Hindley-Milner", diff --git a/test_programs/memory_report.sh b/test_programs/memory_report.sh new file mode 100755 index 00000000000..edb254c4ae4 --- /dev/null +++ b/test_programs/memory_report.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +set -e + +sudo apt-get install heaptrack + +NARGO="nargo" + + +# Tests to be profiled for memory report +tests_to_profile=("keccak256" "workspace" "regression_4709") + +current_dir=$(pwd) +execution_success_path="$current_dir/execution_success" +test_dirs=$(ls $execution_success_path) + +FIRST="1" + +echo "{\"memory_reports\": [ " > memory_report.json + + +for test_name in ${tests_to_profile[@]}; do + full_path=$execution_success_path"/"$test_name + cd $full_path + + if [ $FIRST = "1" ] + then + FIRST="0" + else + echo " ," >> $current_dir"/memory_report.json" + fi + heaptrack --output $current_dir/$test_name"_heap" $NARGO compile --force + if test -f $current_dir/$test_name"_heap.gz"; + then + heaptrack --analyze $current_dir/$test_name"_heap.gz" > $current_dir/$test_name"_heap_analysis.txt" + rm $current_dir/$test_name"_heap.gz" + else + heaptrack --analyze $current_dir/$test_name"_heap.zst" > $current_dir/$test_name"_heap_analysis.txt" + rm $current_dir/$test_name"_heap.zst" + fi + consumption="$(grep 'peak heap memory consumption' $current_dir/$test_name'_heap_analysis.txt')" + len=${#consumption}-30 + peak=${consumption:30:len} + rm $current_dir/$test_name"_heap_analysis.txt" + echo -e " {\n \"artifact_name\":\"$test_name\",\n \"peak_memory\":\"$peak\"\n }" >> $current_dir"/memory_report.json" +done + +echo "]}" >> $current_dir"/memory_report.json" + From ea7c04a8410ed8d2ce8e5a27e3c0784ba3195638 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 13:44:03 -0300 Subject: [PATCH 10/13] feat: better error message when trying to invoke struct function field (#6661) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: Akosh Farkash Co-authored-by: jfecher --- .../noirc_frontend/src/elaborator/types.rs | 22 ++++++++++---- .../src/hir/type_check/errors.rs | 9 ++++++ compiler/noirc_frontend/src/hir_def/types.rs | 8 +++++ compiler/noirc_frontend/src/tests.rs | 29 +++++++++++++++++++ 4 files changed, 63 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 7e06964b563..0404ae3c2c0 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1321,11 +1321,23 @@ impl<'context> Elaborator<'context> { { Some(method_id) => Some(HirMethodReference::FuncId(method_id)), None => { - self.push_err(TypeCheckError::UnresolvedMethodCall { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); + let has_field_with_function_type = + typ.borrow().get_fields_as_written().into_iter().any(|field| { + field.name.0.contents == method_name && field.typ.is_function() + }); + if has_field_with_function_type { + self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } else { + self.push_err(TypeCheckError::UnresolvedMethodCall { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } None } } diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index a6b6120986e..dfa431157e3 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -99,6 +99,8 @@ pub enum TypeCheckError { CannotMutateImmutableVariable { name: String, span: Span }, #[error("No method named '{method_name}' found for type '{object_type}'")] UnresolvedMethodCall { method_name: String, object_type: Type, span: Span }, + #[error("Cannot invoke function field '{method_name}' on type '{object_type}' as a method")] + CannotInvokeStructFieldFunctionType { method_name: String, object_type: Type, span: Span }, #[error("Integers must have the same signedness LHS is {sign_x:?}, RHS is {sign_y:?}")] IntegerSignedness { sign_x: Signedness, sign_y: Signedness, span: Span }, #[error("Integers must have the same bit width LHS is {bit_width_x}, RHS is {bit_width_y}")] @@ -511,6 +513,13 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic { TypeCheckError::CyclicType { typ: _, span } => { Diagnostic::simple_error(error.to_string(), "Cyclic types have unlimited size and are prohibited in Noir".into(), *span) } + TypeCheckError::CannotInvokeStructFieldFunctionType { method_name, object_type, span } => { + Diagnostic::simple_error( + format!("Cannot invoke function field '{method_name}' on type '{object_type}' as a method"), + format!("to call the function stored in '{method_name}', surround the field access with parentheses: '(', ')'"), + *span, + ) + }, } } } diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 659fafbbcbb..2c9a44c079d 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1087,6 +1087,14 @@ impl Type { } } + pub fn is_function(&self) -> bool { + match self.follow_bindings_shallow().as_ref() { + Type::Function(..) => true, + Type::Alias(alias_type, _) => alias_type.borrow().typ.is_function(), + _ => false, + } + } + /// True if this type can be used as a parameter to `main` or a contract function. /// This is only false for unsized types like slices or slices that do not make sense /// as a program input such as named generics or mutable references. diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index bfa8785e6b4..cba29d58ea3 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3752,6 +3752,35 @@ fn allows_struct_with_generic_infix_type_as_main_input_3() { assert_no_errors(src); } +#[test] +fn errors_with_better_message_when_trying_to_invoke_struct_field_that_is_a_function() { + let src = r#" + pub struct Foo { + wrapped: fn(Field) -> bool, + } + + impl Foo { + fn call(self) -> bool { + self.wrapped(1) + } + } + + fn main() {} + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::TypeError(TypeCheckError::CannotInvokeStructFieldFunctionType { + method_name, + .. + }) = &errors[0].0 + else { + panic!("Expected a 'CannotInvokeStructFieldFunctionType' error, got {:?}", errors[0].0); + }; + + assert_eq!(method_name, "wrapped"); +} + fn test_disallows_attribute_on_impl_method( attr: &str, check_error: impl FnOnce(&CompilationError), From 26d235198f9a2fedbe438b3f7b39184554c5e1c1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Mon, 2 Dec 2024 11:53:34 -0500 Subject: [PATCH 11/13] feat(ssa): Hoisting of array get using known induction variable maximum (#6639) --- .../src/ssa/opt/loop_invariant.rs | 201 +++++++++++++++++- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 4 +- .../loop_invariant_regression/src/main.nr | 13 ++ 3 files changed, 208 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index 14233ca73e5..f1dd511402e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -7,14 +7,16 @@ //! - Already marked as loop invariants //! //! We also check that we are not hoisting instructions with side effects. -use fxhash::FxHashSet as HashSet; +use acvm::{acir::AcirField, FieldElement}; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::ssa::{ ir::{ basic_block::BasicBlockId, function::{Function, RuntimeType}, function_inserter::FunctionInserter, - instruction::InstructionId, + instruction::{Instruction, InstructionId}, + types::Type, value::ValueId, }, Ssa, @@ -45,25 +47,51 @@ impl Function { } impl Loops { - fn hoist_loop_invariants(self, function: &mut Function) { + fn hoist_loop_invariants(mut self, function: &mut Function) { let mut context = LoopInvariantContext::new(function); - for loop_ in self.yet_to_unroll.iter() { + // The loops should be sorted by the number of blocks. + // We want to access outer nested loops first, which we do by popping + // from the top of the list. + while let Some(loop_) = self.yet_to_unroll.pop() { let Ok(pre_header) = loop_.get_pre_header(context.inserter.function, &self.cfg) else { // If the loop does not have a preheader we skip hoisting loop invariants for this loop continue; }; - context.hoist_loop_invariants(loop_, pre_header); + + context.hoist_loop_invariants(&loop_, pre_header); } context.map_dependent_instructions(); } } +impl Loop { + /// Find the value that controls whether to perform a loop iteration. + /// This is going to be the block parameter of the loop header. + /// + /// Consider the following example of a `for i in 0..4` loop: + /// ```text + /// brillig(inline) fn main f0 { + /// b0(v0: u32): + /// ... + /// jmp b1(u32 0) + /// b1(v1: u32): // Loop header + /// v5 = lt v1, u32 4 // Upper bound + /// jmpif v5 then: b3, else: b2 + /// ``` + /// In the example above, `v1` is the induction variable + fn get_induction_variable(&self, function: &Function) -> ValueId { + function.dfg.block_parameters(self.header)[0] + } +} + struct LoopInvariantContext<'f> { inserter: FunctionInserter<'f>, defined_in_loop: HashSet, loop_invariants: HashSet, + // Maps induction variable -> fixed upper loop bound + outer_induction_variables: HashMap, } impl<'f> LoopInvariantContext<'f> { @@ -72,6 +100,7 @@ impl<'f> LoopInvariantContext<'f> { inserter: FunctionInserter::new(function), defined_in_loop: HashSet::default(), loop_invariants: HashSet::default(), + outer_induction_variables: HashMap::default(), } } @@ -88,13 +117,29 @@ impl<'f> LoopInvariantContext<'f> { self.inserter.push_instruction(instruction_id, *block); } - self.update_values_defined_in_loop_and_invariants(instruction_id, hoist_invariant); + self.extend_values_defined_in_loop_and_invariants(instruction_id, hoist_invariant); } } + + // Keep track of a loop induction variable and respective upper bound. + // This will be used by later loops to determine whether they have operations + // reliant upon the maximum induction variable. + let upper_bound = loop_.get_const_upper_bound(self.inserter.function); + if let Some(upper_bound) = upper_bound { + let induction_variable = loop_.get_induction_variable(self.inserter.function); + let induction_variable = self.inserter.resolve(induction_variable); + self.outer_induction_variables.insert(induction_variable, upper_bound); + } } /// Gather the variables declared within the loop fn set_values_defined_in_loop(&mut self, loop_: &Loop) { + // Clear any values that may be defined in previous loops, as the context is per function. + self.defined_in_loop.clear(); + // These are safe to keep per function, but we want to be clear that these values + // are used per loop. + self.loop_invariants.clear(); + for block in loop_.blocks.iter() { let params = self.inserter.function.dfg.block_parameters(*block); self.defined_in_loop.extend(params); @@ -107,7 +152,7 @@ impl<'f> LoopInvariantContext<'f> { /// Update any values defined in the loop and loop invariants after a /// analyzing and re-inserting a loop's instruction. - fn update_values_defined_in_loop_and_invariants( + fn extend_values_defined_in_loop_and_invariants( &mut self, instruction_id: InstructionId, hoist_invariant: bool, @@ -143,9 +188,45 @@ impl<'f> LoopInvariantContext<'f> { is_loop_invariant &= !self.defined_in_loop.contains(&value) || self.loop_invariants.contains(&value); }); - is_loop_invariant && instruction.can_be_deduplicated(&self.inserter.function.dfg, false) + + let can_be_deduplicated = instruction + .can_be_deduplicated(&self.inserter.function.dfg, false) + || self.can_be_deduplicated_from_upper_bound(&instruction); + + is_loop_invariant && can_be_deduplicated + } + + /// Certain instructions can take advantage of that our induction variable has a fixed maximum. + /// + /// For example, an array access can usually only be safely deduplicated when we have a constant + /// index that is below the length of the array. + /// Checking an array get where the index is the loop's induction variable on its own + /// would determine that the instruction is not safe for hoisting. + /// However, if we know that the induction variable's upper bound will always be in bounds of the array + /// we can safely hoist the array access. + fn can_be_deduplicated_from_upper_bound(&self, instruction: &Instruction) -> bool { + match instruction { + Instruction::ArrayGet { array, index } => { + let array_typ = self.inserter.function.dfg.type_of_value(*array); + let upper_bound = self.outer_induction_variables.get(index); + if let (Type::Array(_, len), Some(upper_bound)) = (array_typ, upper_bound) { + upper_bound.to_u128() as usize <= len + } else { + false + } + } + _ => false, + } } + /// Loop invariant hoisting only operates over loop instructions. + /// The `FunctionInserter` is used for mapping old values to new values after + /// re-inserting loop invariant instructions. + /// However, there may be instructions which are not within loops that are + /// still reliant upon the instruction results altered during the pass. + /// This method re-inserts all instructions so that all instructions have + /// correct new value IDs based upon the `FunctionInserter` internal map. + /// Leaving out this mapping could lead to instructions with values that do not exist. fn map_dependent_instructions(&mut self) { let blocks = self.inserter.function.reachable_blocks(); for block in blocks { @@ -375,4 +456,108 @@ mod test { // The code should be unchanged assert_normalized_ssa_equals(ssa, src); } + + #[test] + fn hoist_array_gets_using_induction_variable_with_const_bound() { + // SSA for the following program: + // + // fn triple_loop(x: u32) { + // let arr = [2; 5]; + // for i in 0..4 { + // for j in 0..4 { + // for _ in 0..4 { + // assert_eq(arr[i], x); + // assert_eq(arr[j], x); + // } + // } + // } + // } + // + // `arr[i]` and `arr[j]` are safe to hoist as we know the maximum possible index + // to be used for both array accesses. + // We want to make sure `arr[i]` is hoisted to the outermost loop body and that + // `arr[j]` is hoisted to the second outermost loop body. + let src = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v6 = make_array [u32 2, u32 2, u32 2, u32 2, u32 2] : [u32; 5] + inc_rc v6 + jmp b1(u32 0) + b1(v2: u32): + v9 = lt v2, u32 4 + jmpif v9 then: b3, else: b2 + b3(): + jmp b4(u32 0) + b4(v3: u32): + v10 = lt v3, u32 4 + jmpif v10 then: b6, else: b5 + b6(): + jmp b7(u32 0) + b7(v4: u32): + v13 = lt v4, u32 4 + jmpif v13 then: b9, else: b8 + b9(): + v15 = array_get v6, index v2 -> u32 + v16 = eq v15, v0 + constrain v15 == v0 + v17 = array_get v6, index v3 -> u32 + v18 = eq v17, v0 + constrain v17 == v0 + v19 = add v4, u32 1 + jmp b7(v19) + b8(): + v14 = add v3, u32 1 + jmp b4(v14) + b5(): + v12 = add v2, u32 1 + jmp b1(v12) + b2(): + return + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + + let expected = " + brillig(inline) fn main f0 { + b0(v0: u32, v1: u32): + v6 = make_array [u32 2, u32 2, u32 2, u32 2, u32 2] : [u32; 5] + inc_rc v6 + jmp b1(u32 0) + b1(v2: u32): + v9 = lt v2, u32 4 + jmpif v9 then: b3, else: b2 + b3(): + v10 = array_get v6, index v2 -> u32 + v11 = eq v10, v0 + jmp b4(u32 0) + b4(v3: u32): + v12 = lt v3, u32 4 + jmpif v12 then: b6, else: b5 + b6(): + v15 = array_get v6, index v3 -> u32 + v16 = eq v15, v0 + jmp b7(u32 0) + b7(v4: u32): + v17 = lt v4, u32 4 + jmpif v17 then: b9, else: b8 + b9(): + constrain v10 == v0 + constrain v15 == v0 + v19 = add v4, u32 1 + jmp b7(v19) + b8(): + v18 = add v3, u32 1 + jmp b4(v18) + b5(): + v14 = add v2, u32 1 + jmp b1(v14) + b2(): + return + } + "; + + let ssa = ssa.loop_invariant_code_motion(); + assert_normalized_ssa_equals(ssa, expected); + } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 5883ce25936..1a13acc5435 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -109,7 +109,7 @@ impl Function { pub(super) struct Loop { /// The header block of a loop is the block which dominates all the /// other blocks in the loop. - header: BasicBlockId, + pub(super) header: BasicBlockId, /// The start of the back_edge n -> d is the block n at the end of /// the loop that jumps back to the header block d which restarts the loop. @@ -299,7 +299,7 @@ impl Loop { /// v5 = lt v1, u32 4 // Upper bound /// jmpif v5 then: b3, else: b2 /// ``` - fn get_const_upper_bound(&self, function: &Function) -> Option { + pub(super) fn get_const_upper_bound(&self, function: &Function) -> Option { let block = &function.dfg[self.header]; let instructions = block.instructions(); assert_eq!( diff --git a/test_programs/execution_success/loop_invariant_regression/src/main.nr b/test_programs/execution_success/loop_invariant_regression/src/main.nr index 25f6e92f868..c28ce063116 100644 --- a/test_programs/execution_success/loop_invariant_regression/src/main.nr +++ b/test_programs/execution_success/loop_invariant_regression/src/main.nr @@ -2,6 +2,7 @@ // to be hoisted to the loop's pre-header block. fn main(x: u32, y: u32) { loop(4, x, y); + array_read_loop(4, x); } fn loop(upper_bound: u32, x: u32, y: u32) { @@ -11,3 +12,15 @@ fn loop(upper_bound: u32, x: u32, y: u32) { assert_eq(z, 12); } } + +fn array_read_loop(upper_bound: u32, x: u32) { + let arr = [2; 5]; + for i in 0..upper_bound { + for j in 0..upper_bound { + for _ in 0..upper_bound { + assert_eq(arr[i], x); + assert_eq(arr[j], x); + } + } + } +} From b4750d8dc13245bad81cbf7bef7010e3794e040a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 2 Dec 2024 14:01:06 -0300 Subject: [PATCH 12/13] fix: Prevent hoisting binary instructions which can overflow (#6672) Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 17 ++++++++-- .../src/ssa/opt/constant_folding.rs | 34 ++++++++++++++++--- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 6737b335b7d..0d7b9d10f4c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -389,9 +389,22 @@ impl Instruction { // This should never be side-effectful MakeArray { .. } => false, + // Some binary math can overflow or underflow + Binary(binary) => match binary.operator { + BinaryOp::Add | BinaryOp::Sub | BinaryOp::Mul | BinaryOp::Div | BinaryOp::Mod => { + true + } + BinaryOp::Eq + | BinaryOp::Lt + | BinaryOp::And + | BinaryOp::Or + | BinaryOp::Xor + | BinaryOp::Shl + | BinaryOp::Shr => false, + }, + // These can have different behavior depending on the EnableSideEffectsIf context. - Binary(_) - | Cast(_, _) + Cast(_, _) | Not(_) | Truncate { .. } | IfElse { .. } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 41c84c935b1..39bda435842 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1187,7 +1187,7 @@ mod test { v2 = lt u32 1000, v0 jmpif v2 then: b1, else: b2 b1(): - v4 = add v0, u32 1 + v4 = shl v0, u32 1 v5 = lt v0, v4 constrain v5 == u1 1 jmp b2() @@ -1195,7 +1195,7 @@ mod test { v7 = lt u32 1000, v0 jmpif v7 then: b3, else: b4 b3(): - v8 = add v0, u32 1 + v8 = shl v0, u32 1 v9 = lt v0, v8 constrain v9 == u1 1 jmp b4() @@ -1213,10 +1213,10 @@ mod test { brillig(inline) fn main f0 { b0(v0: u32): v2 = lt u32 1000, v0 - v4 = add v0, u32 1 + v4 = shl v0, u32 1 jmpif v2 then: b1, else: b2 b1(): - v5 = add v0, u32 1 + v5 = shl v0, u32 1 v6 = lt v0, v5 constrain v6 == u1 1 jmp b2() @@ -1457,6 +1457,32 @@ mod test { assert_normalized_ssa_equals(ssa, src); } + #[test] + fn does_not_hoist_sub_to_common_ancestor() { + let src = " + acir(inline) fn main f0 { + b0(v0: u32): + v2 = eq v0, u32 0 + jmpif v2 then: b4, else: b1 + b4(): + v5 = sub v0, u32 1 + jmp b5() + b5(): + return + b1(): + jmpif v0 then: b3, else: b2 + b3(): + v4 = sub v0, u32 1 // We can't hoist this because v0 is zero here and it will lead to an underflow + jmp b5() + b2(): + jmp b5() + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let ssa = ssa.fold_constants_using_constraints(); + assert_normalized_ssa_equals(ssa, src); + } + #[test] fn deduplicates_side_effecting_intrinsics() { let src = " From 920077da2d75493328c12d99b5684356c1ea1c4c Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Mon, 2 Dec 2024 18:06:00 +0100 Subject: [PATCH 13/13] chore: update noir-bench-report version (#6675) --- .github/workflows/memory_report.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/memory_report.yml b/.github/workflows/memory_report.yml index a1b28aee31d..c31c750e6fc 100644 --- a/.github/workflows/memory_report.yml +++ b/.github/workflows/memory_report.yml @@ -73,7 +73,7 @@ jobs: - name: Parse memory report id: memory_report - uses: noir-lang/noir-bench-report@d61bc78ece3c8df1e72914b3d5136bad403d5bdf + uses: noir-lang/noir-bench-report@ccb0d806a91d3bd86dba0ba3d580a814eed5673c with: report: memory_report.json header: |