From 13c41d21f81fb40cdf2a970be119a83d11da9e03 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 12 Dec 2024 15:12:03 +0000 Subject: [PATCH] fix: optimizer to keep track of changing opcode locations (#6781) --- acvm-repo/acvm/src/compiler/mod.rs | 12 +++-- acvm-repo/acvm/src/compiler/optimizers/mod.rs | 17 ++++--- .../assert_message/Nargo.toml | 7 +++ .../assert_message/src/main.nr | 15 +++++++ .../fuzzer_checks/src/main.nr | 7 ++- tooling/nargo/src/ops/test.rs | 44 ++++++++++++++----- 6 files changed, 79 insertions(+), 23 deletions(-) create mode 100644 test_programs/noir_test_success/assert_message/Nargo.toml create mode 100644 test_programs/noir_test_success/assert_message/src/main.nr diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 4ad4952c5cc..daedd77c4a0 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -82,19 +82,22 @@ pub fn compile( acir: Circuit, expression_width: ExpressionWidth, ) -> (Circuit, AcirTransformationMap) { + let acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); + if MAX_OPTIMIZER_PASSES == 0 { - let acir_opcode_positions = (0..acir.opcodes.len()).collect::>(); - let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); - return (acir, transformation_map); + return (acir, AcirTransformationMap::new(&acir_opcode_positions)); } + let mut pass = 0; let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes); let mut prev_acir = acir; + let mut prev_acir_opcode_positions = acir_opcode_positions; // For most test programs it would be enough to only loop `transform_internal`, // but some of them don't stabilize unless we also repeat the backend agnostic optimizations. let (mut acir, acir_opcode_positions) = loop { - let (acir, acir_opcode_positions) = optimize_internal(prev_acir); + let (acir, acir_opcode_positions) = + optimize_internal(prev_acir, prev_acir_opcode_positions); // Stop if we have already done at least one transform and an extra optimization changed nothing. if pass > 0 && prev_opcodes_hash == fxhash::hash64(&acir.opcodes) { @@ -114,6 +117,7 @@ pub fn compile( pass += 1; prev_acir = acir; prev_opcodes_hash = opcodes_hash; + prev_acir_opcode_positions = acir_opcode_positions; }; let transformation_map = AcirTransformationMap::new(&acir_opcode_positions); diff --git a/acvm-repo/acvm/src/compiler/optimizers/mod.rs b/acvm-repo/acvm/src/compiler/optimizers/mod.rs index f86bf500998..1b743227acf 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/mod.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/mod.rs @@ -21,7 +21,11 @@ use super::{transform_assert_messages, AcirTransformationMap}; /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] independent optimizations to a [`Circuit`]. pub fn optimize(acir: Circuit) -> (Circuit, AcirTransformationMap) { - let (mut acir, new_opcode_positions) = optimize_internal(acir); + // Track original acir opcode positions throughout the transformation passes of the compilation + // by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert) + let acir_opcode_positions = (0..acir.opcodes.len()).collect(); + + let (mut acir, new_opcode_positions) = optimize_internal(acir, acir_opcode_positions); let transformation_map = AcirTransformationMap::new(&new_opcode_positions); @@ -31,12 +35,13 @@ pub fn optimize(acir: Circuit) -> (Circuit, AcirTransformati } /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] independent optimizations to a [`Circuit`]. +/// +/// Accepts an injected `acir_opcode_positions` to allow optimizations to be applied in a loop. #[tracing::instrument(level = "trace", name = "optimize_acir" skip(acir))] -pub(super) fn optimize_internal(acir: Circuit) -> (Circuit, Vec) { - // Track original acir opcode positions throughout the transformation passes of the compilation - // by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert) - let acir_opcode_positions = (0..acir.opcodes.len()).collect(); - +pub(super) fn optimize_internal( + acir: Circuit, + acir_opcode_positions: Vec, +) -> (Circuit, Vec) { if acir.opcodes.len() == 1 && matches!(acir.opcodes[0], Opcode::BrilligCall { .. }) { info!("Program is fully unconstrained, skipping optimization pass"); return (acir, acir_opcode_positions); diff --git a/test_programs/noir_test_success/assert_message/Nargo.toml b/test_programs/noir_test_success/assert_message/Nargo.toml new file mode 100644 index 00000000000..667035339bd --- /dev/null +++ b/test_programs/noir_test_success/assert_message/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "assert_message" +type = "bin" +authors = [""] +compiler_version = ">=0.23.0" + +[dependencies] diff --git a/test_programs/noir_test_success/assert_message/src/main.nr b/test_programs/noir_test_success/assert_message/src/main.nr new file mode 100644 index 00000000000..073e42daf4a --- /dev/null +++ b/test_programs/noir_test_success/assert_message/src/main.nr @@ -0,0 +1,15 @@ +// Have to use inputs otherwise the ACIR gets optimized away due to constants. +// With the original ACIR the optimizations can rearrange or merge opcodes, +// which might end up getting out of sync with assertion messages. +#[test(should_fail_with = "first")] +fn test_assert_message_preserved_during_optimization(a: Field, b: Field, c: Field) { + if (a + b) * (a - b) != c { + assert((a + b) * (a - b) == c, "first"); + assert((a - b) * (a + b) == c, "second"); + assert((a + b) * (a - b) == c, "third"); + assert((2 * a + b) * (a - b / 2) == c * c, "fourth"); + } else { + // The fuzzer might have generated a passing test. + assert((a + b) * (a - b) != c, "first"); + } +} diff --git a/test_programs/noir_test_success/fuzzer_checks/src/main.nr b/test_programs/noir_test_success/fuzzer_checks/src/main.nr index 2b928db092e..70e5e29b855 100644 --- a/test_programs/noir_test_success/fuzzer_checks/src/main.nr +++ b/test_programs/noir_test_success/fuzzer_checks/src/main.nr @@ -1,6 +1,9 @@ - #[test(should_fail_with = "42 is not allowed")] fn finds_magic_value(x: u32) { let x = x as u64; - assert(2 * x != 42, "42 is not allowed"); + if x == 21 { + assert(2 * x != 42, "42 is not allowed"); + } else { + assert(2 * x == 42, "42 is not allowed"); + } } diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 7087c0de64e..e3a264359e4 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -9,7 +9,7 @@ use acvm::{ AcirField, BlackBoxFunctionSolver, FieldElement, }; use noirc_abi::Abi; -use noirc_driver::{compile_no_check, CompileError, CompileOptions}; +use noirc_driver::{compile_no_check, CompileError, CompileOptions, DEFAULT_EXPRESSION_WIDTH}; use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; use noirc_frontend::hir::{def_map::TestFunction, Context}; use noirc_printable_type::ForeignCallError; @@ -29,6 +29,7 @@ use crate::{ use super::execute_program; +#[derive(Debug)] pub enum TestStatus { Pass, Fail { message: String, error_diagnostic: Option }, @@ -62,6 +63,10 @@ pub fn run_test>( match compile_no_check(context, config, test_function.get_id(), None, false) { Ok(compiled_program) => { + // Do the same optimizations as `compile_cmd`. + let target_width = config.expression_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH); + let compiled_program = crate::ops::transform_program(compiled_program, target_width); + 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. @@ -81,9 +86,9 @@ pub fn run_test>( let status = test_status_program_compile_pass( test_function, - compiled_program.abi, - compiled_program.debug, - circuit_execution, + &compiled_program.abi, + &compiled_program.debug, + &circuit_execution, ); let ignore_foreign_call_failures = @@ -118,11 +123,14 @@ pub fn run_test>( use proptest::test_runner::TestRunner; let runner = TestRunner::default(); + let abi = compiled_program.abi.clone(); + let debug = compiled_program.debug.clone(); + let executor = |program: &Program, initial_witness: WitnessMap| -> Result, String> { - execute_program( + let circuit_execution = execute_program( program, initial_witness, blackbox_solver, @@ -132,9 +140,23 @@ pub fn run_test>( root_path.clone(), package_name.clone(), ), - ) - .map_err(|err| err.to_string()) + ); + + let status = test_status_program_compile_pass( + test_function, + &abi, + &debug, + &circuit_execution, + ); + + if let TestStatus::Fail { message, error_diagnostic: _ } = status { + Err(message) + } else { + // The fuzzer doesn't care about the actual result. + Ok(WitnessStack::default()) + } }; + let fuzzer = FuzzedExecutor::new(compiled_program.into(), executor, runner); let result = fuzzer.fuzz(); @@ -174,9 +196,9 @@ fn test_status_program_compile_fail(err: CompileError, test_function: &TestFunct /// passed/failed to determine the test status. fn test_status_program_compile_pass( test_function: &TestFunction, - abi: Abi, - debug: Vec, - circuit_execution: Result, NargoError>, + abi: &Abi, + debug: &[DebugInfo], + circuit_execution: &Result, NargoError>, ) -> TestStatus { let circuit_execution_err = match circuit_execution { // Circuit execution was successful; ie no errors or unsatisfied constraints @@ -196,7 +218,7 @@ fn test_status_program_compile_pass( // If we reach here, then the circuit execution failed. // // Check if the function should have passed - let diagnostic = try_to_diagnose_runtime_error(&circuit_execution_err, &abi, &debug); + let diagnostic = try_to_diagnose_runtime_error(circuit_execution_err, abi, debug); let test_should_have_passed = !test_function.should_fail(); if test_should_have_passed { return TestStatus::Fail {