From 68fa24a370b64b554d2e8afd87324861d3f102a4 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Dec 2024 18:07:24 +0000 Subject: [PATCH 1/5] add basic non determinism check --- compiler/noirc_driver/src/lib.rs | 4 ++++ tooling/nargo_cli/build.rs | 3 +++ tooling/nargo_cli/src/cli/compile_cmd.rs | 16 ++++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 9318e4d2b5c..1b311504b5c 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -150,6 +150,10 @@ pub struct CompileOptions { /// 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, + + /// Used internally to test for non-determinism in the compiler. + #[clap(long, hide = true)] + pub check_non_determinism: bool, } pub fn parse_expression_width(input: &str) -> Result { diff --git a/tooling/nargo_cli/build.rs b/tooling/nargo_cli/build.rs index 8db2c1786d8..003897489c4 100644 --- a/tooling/nargo_cli/build.rs +++ b/tooling/nargo_cli/build.rs @@ -215,6 +215,9 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner // Set the maximum increase so that part of the optimization is exercised (it might fail). nargo.arg("--max-bytecode-increase-percent"); nargo.arg("50"); + + // Check whether the test case is non-deterministic + nargo.arg("--check-non-determinism"); }} {test_content} diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 2ecf6959a94..e47b82ad791 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -216,6 +216,21 @@ fn compile_programs( cached_program, )?; + if compile_options.check_non_determinism { + dbg!("got here"); + let (program_two, _) = compile_program( + file_manager, + parsed_files, + workspace, + package, + compile_options, + load_cached_program(package), + )?; + if fxhash::hash64(&program) != fxhash::hash64(&program_two) { + panic!("Non deterministic result compiling {}", package.name); + } + } + // Choose the target width for the final, backend specific transformation. let target_width = get_target_width(package.expression_width, compile_options.expression_width); @@ -234,6 +249,7 @@ fn compile_programs( return Ok(((), warnings)); } } + // Run ACVM optimizations and set the target width. let program = nargo::ops::transform_program(program, target_width); // Check solvability. From cc7009fc6604a57f4aab2b3901754d7b4db1c7fb Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 17 Dec 2024 18:09:28 +0000 Subject: [PATCH 2/5] remove dbg --- tooling/nargo_cli/src/cli/compile_cmd.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index e47b82ad791..f718021c351 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -217,7 +217,6 @@ fn compile_programs( )?; if compile_options.check_non_determinism { - dbg!("got here"); let (program_two, _) = compile_program( file_manager, parsed_files, @@ -249,7 +248,6 @@ fn compile_programs( return Ok(((), warnings)); } } - // Run ACVM optimizations and set the target width. let program = nargo::ops::transform_program(program, target_width); // Check solvability. From af1eec6446fd49acf8fea02484b88f3d0283b340 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 18 Dec 2024 09:34:51 -0500 Subject: [PATCH 3/5] fix: Remove all non determinism (#6852) --- .../src/brillig/brillig_ir/artifact.rs | 8 ++++---- compiler/noirc_evaluator/src/ssa/opt/unrolling.rs | 6 ++++-- compiler/noirc_frontend/src/ast/statement.rs | 13 ++++++------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index be4a6b84bc1..4a8e1cdcad3 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -27,7 +27,7 @@ pub(crate) struct GeneratedBrillig { pub(crate) locations: BTreeMap, pub(crate) error_types: BTreeMap, pub(crate) name: String, - pub(crate) procedure_locations: HashMap, + pub(crate) procedure_locations: BTreeMap, } #[derive(Default, Debug, Clone)] @@ -61,13 +61,13 @@ pub(crate) struct BrilligArtifact { /// This is created as artifacts are linked together and allows us to determine /// which opcodes originate from reusable procedures.s /// The range is inclusive for both start and end opcode locations. - pub(crate) procedure_locations: HashMap, + pub(crate) procedure_locations: BTreeMap, } /// A pointer to a location in the opcode. pub(crate) type OpcodeLocation = usize; -#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)] pub(crate) enum LabelType { /// Labels for the entry point bytecode Entrypoint, @@ -97,7 +97,7 @@ impl std::fmt::Display for LabelType { /// /// It is assumed that an entity will keep a map /// of labels to Opcode locations. -#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)] pub(crate) struct Label { pub(crate) label_type: LabelType, pub(crate) section: Option, diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 2a272236195..ab4256197b9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -18,6 +18,8 @@ //! //! When unrolling ACIR code, we remove reference count instructions because they are //! only used by Brillig bytecode. +use std::collections::BTreeSet; + use acvm::{acir::AcirField, FieldElement}; use im::HashSet; @@ -117,7 +119,7 @@ pub(super) struct Loop { back_edge_start: BasicBlockId, /// All the blocks contained within the loop, including `header` and `back_edge_start`. - pub(super) blocks: HashSet, + pub(super) blocks: BTreeSet, } pub(super) struct Loops { @@ -238,7 +240,7 @@ impl Loop { back_edge_start: BasicBlockId, cfg: &ControlFlowGraph, ) -> Self { - let mut blocks = HashSet::default(); + let mut blocks = BTreeSet::default(); blocks.insert(header); let mut insert = |block, stack: &mut Vec| { diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 0ce21f9f798..bd65fc33377 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -1,5 +1,4 @@ use std::fmt::Display; -use std::sync::atomic::{AtomicU32, Ordering}; use acvm::acir::AcirField; use acvm::FieldElement; @@ -841,10 +840,9 @@ impl ForRange { block: Expression, for_loop_span: Span, ) -> Statement { - /// Counter used to generate unique names when desugaring - /// code in the parser requires the creation of fresh variables. - /// The parser is stateless so this is a static global instead. - static UNIQUE_NAME_COUNTER: AtomicU32 = AtomicU32::new(0); + // Counter used to generate unique names when desugaring + // code in the parser requires the creation of fresh variables. + let mut unique_name_counter: u32 = 0; match self { ForRange::Range(..) => { @@ -855,7 +853,8 @@ impl ForRange { let start_range = ExpressionKind::integer(FieldElement::zero()); let start_range = Expression::new(start_range, array_span); - let next_unique_id = UNIQUE_NAME_COUNTER.fetch_add(1, Ordering::Relaxed); + let next_unique_id = unique_name_counter; + unique_name_counter += 1; let array_name = format!("$i{next_unique_id}"); let array_span = array.span; let array_ident = Ident::new(array_name, array_span); @@ -888,7 +887,7 @@ impl ForRange { })); let end_range = Expression::new(end_range, array_span); - let next_unique_id = UNIQUE_NAME_COUNTER.fetch_add(1, Ordering::Relaxed); + let next_unique_id = unique_name_counter; let index_name = format!("$i{next_unique_id}"); let fresh_identifier = Ident::new(index_name.clone(), array_span); From 2bb1bd344cf8308bb5a3062079d96d43f0a6da25 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 18 Dec 2024 09:35:34 -0500 Subject: [PATCH 4/5] Update compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs --- compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 4a8e1cdcad3..3993e607395 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -97,7 +97,7 @@ impl std::fmt::Display for LabelType { /// /// It is assumed that an entity will keep a map /// of labels to Opcode locations. -#[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)] +#[derive(Debug, Clone, Eq, PartialEq, Hash)] pub(crate) struct Label { pub(crate) label_type: LabelType, pub(crate) section: Option, From 067c5892c5674ef7daae8898e6fba8123acdd9a5 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 18 Dec 2024 09:35:47 -0500 Subject: [PATCH 5/5] Update compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs --- compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 3993e607395..3654a95a03f 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -67,7 +67,7 @@ pub(crate) struct BrilligArtifact { /// A pointer to a location in the opcode. pub(crate) type OpcodeLocation = usize; -#[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)] +#[derive(Debug, Clone, Eq, PartialEq, Hash)] pub(crate) enum LabelType { /// Labels for the entry point bytecode Entrypoint,