Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(test): Run test matrix on test_programs #6429

Merged
merged 13 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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):")
Expand All @@ -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:")
Expand Down
1 change: 1 addition & 0 deletions noir_stdlib/src/hash/poseidon/mod.nr
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ impl Hasher for PoseidonHasher {
result
}

#[inline_always]
fn write(&mut self, input: Field) {
self._state = self._state.push_back(input);
}
Expand Down
1 change: 0 additions & 1 deletion test_programs/execution_success/eddsa/src/main.nr
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
83 changes: 71 additions & 12 deletions tooling/nargo_cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
];

TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
/// Some tests are expected to have warnings
/// These should be fixed and removed from this list.
const TESTS_WITH_EXPECTED_WARNINGS: [&str; 2] = [
Expand Down Expand Up @@ -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,
Expand All @@ -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#"
Expand All @@ -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());
Expand Down Expand Up @@ -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),
},
);
}
Expand Down
Loading