From 82046be7790bb7128f481ace94b8a6841f76c88d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 12 Nov 2024 12:22:03 +0000 Subject: [PATCH] chore(test): Run test matrix on test_programs (#6429) Co-authored-by: Tom French Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa.rs | 4 +- noir_stdlib/src/hash/poseidon/mod.nr | 1 + .../execution_success/eddsa/src/main.nr | 1 - tooling/nargo_cli/build.rs | 83 ++++++++++++++++--- 4 files changed, 74 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 0c4e42f09ef..0e5635f9ebb 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -97,7 +97,7 @@ pub(crate) fn optimize_into_acir( .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:") + .run_pass(|ssa| ssa.inline_functions(options.inliner_aggressiveness), "After 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):") @@ -118,7 +118,7 @@ pub(crate) fn optimize_into_acir( // may create an SSA which inlining fails to handle. .run_pass( |ssa| ssa.inline_functions_with_no_predicates(options.inliner_aggressiveness), - "After Inlining:", + "After Inlining (2nd):", ) .run_pass(Ssa::remove_if_else, "After Remove IfElse:") .run_pass(Ssa::fold_constants, "After Constant Folding:") diff --git a/noir_stdlib/src/hash/poseidon/mod.nr b/noir_stdlib/src/hash/poseidon/mod.nr index 6f0e461a610..e2e47959024 100644 --- a/noir_stdlib/src/hash/poseidon/mod.nr +++ b/noir_stdlib/src/hash/poseidon/mod.nr @@ -346,6 +346,7 @@ impl Hasher for PoseidonHasher { result } + #[inline_always] fn write(&mut self, input: Field) { self._state = self._state.push_back(input); } diff --git a/test_programs/execution_success/eddsa/src/main.nr b/test_programs/execution_success/eddsa/src/main.nr index 48e1e4af9fb..d4c3664f0c9 100644 --- a/test_programs/execution_success/eddsa/src/main.nr +++ b/test_programs/execution_success/eddsa/src/main.nr @@ -2,7 +2,6 @@ use std::compat; use std::ec::consts::te::baby_jubjub; use std::ec::tecurve::affine::Point as TEPoint; use std::eddsa::{eddsa_poseidon_verify, eddsa_to_pub, eddsa_verify}; -use std::hash; use std::hash::poseidon2::Poseidon2Hasher; fn main(msg: pub Field, _priv_key_a: Field, _priv_key_b: Field) { diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index eee10ede8c4..984419fe388 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -59,6 +59,12 @@ const IGNORED_BRILLIG_TESTS: [&str; 11] = [ "is_unconstrained", ]; +/// Tests which aren't expected to work with the default inliner cases. +const INLINER_MIN_OVERRIDES: [(&str, i64); 1] = [ + // 0 works if PoseidonHasher::write is tagged as `inline_always`, otherwise 22. + ("eddsa", 0), +]; + /// Some tests are expected to have warnings /// These should be fixed and removed from this list. const TESTS_WITH_EXPECTED_WARNINGS: [&str; 2] = [ @@ -94,12 +100,42 @@ struct MatrixConfig { vary_brillig: bool, // Only seems to have an effect on the `execute_success` cases. vary_inliner: bool, + // If there is a non-default minimum inliner aggressiveness to use with the brillig tests. + min_inliner: i64, +} + +// Enum to be able to preserve readable test labels and also compare to numbers. +enum Inliner { + Min, + Default, + Max, + Custom(i64), +} + +impl Inliner { + fn value(&self) -> i64 { + match self { + Inliner::Min => i64::MIN, + Inliner::Default => 0, + Inliner::Max => i64::MAX, + Inliner::Custom(i) => *i, + } + } + fn label(&self) -> String { + match self { + Inliner::Min => "i64::MIN".to_string(), + Inliner::Default => "0".to_string(), + Inliner::Max => "i64::MAX".to_string(), + Inliner::Custom(i) => i.to_string(), + } + } } -/// Generate all test cases for a given test directory. -/// These will be executed serially, but independently from other test directories. -/// Running multiple tests on the same directory concurrently risks overriding each -/// others compilation artifacts. +/// Generate all test cases for a given test name (expected to be unique for the test directory), +/// based on the matrix configuration. These will be executed serially, but concurrently with +/// other test directories. Running multiple tests on the same directory would risk overriding +/// each others compilation artifacts, which is why this method injects a mutex shared by +/// all cases in the test matrix, as long as the test name and directory has a 1-to-1 relationship. fn generate_test_cases( test_file: &mut File, test_name: &str, @@ -108,11 +144,32 @@ fn generate_test_cases( test_content: &str, matrix_config: &MatrixConfig, ) { + let brillig_cases = if matrix_config.vary_brillig { vec![false, true] } else { vec![false] }; + let inliner_cases = if matrix_config.vary_inliner { + let mut cases = vec![Inliner::Min, Inliner::Default, Inliner::Max]; + if !cases.iter().any(|c| c.value() == matrix_config.min_inliner) { + cases.push(Inliner::Custom(matrix_config.min_inliner)); + } + cases + } else { + vec![Inliner::Default] + }; + + // We can't use a `#[test_matrix(brillig_cases, inliner_cases)` if we only want to limit the + // aggressiveness range for the brillig tests, and let them go full range on the ACIR case. + let mut test_cases = Vec::new(); + for brillig in &brillig_cases { + for inliner in &inliner_cases { + if *brillig && inliner.value() < matrix_config.min_inliner { + continue; + } + test_cases.push(format!("#[test_case::test_case({brillig}, {})]", inliner.label())); + } + } + let test_cases = test_cases.join("\n"); + + // Use a common mutex for all test cases. let mutex_name = format! {"TEST_MUTEX_{}", test_name.to_uppercase()}; - let brillig_cases = if matrix_config.vary_brillig { "[false, true]" } else { "[false]" }; - let _inliner_cases = if matrix_config.vary_inliner { "[i64::MIN, 0, i64::MAX]" } else { "[0]" }; - // TODO (#6429): Remove this once the failing tests are fixed. - let inliner_cases = "[i64::MAX]"; write!( test_file, r#" @@ -121,10 +178,7 @@ lazy_static::lazy_static! {{ static ref {mutex_name}: std::sync::Mutex<()> = std::sync::Mutex::new(()); }} -#[test_case::test_matrix( - {brillig_cases}, - {inliner_cases} -)] +{test_cases} fn test_{test_name}(force_brillig: bool, inliner_aggressiveness: i64) {{ // Ignore poisoning errors if some of the matrix cases failed. let _guard = {mutex_name}.lock().unwrap_or_else(|e| e.into_inner()); @@ -171,6 +225,11 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) &MatrixConfig { vary_brillig: !IGNORED_BRILLIG_TESTS.contains(&test_name.as_str()), vary_inliner: true, + min_inliner: INLINER_MIN_OVERRIDES + .iter() + .find(|(n, _)| *n == test_name.as_str()) + .map(|(_, i)| *i) + .unwrap_or(i64::MIN), }, ); }