chore(ci): Add non determinism check and fixes

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<i32>,
+
+    /// 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<ExpressionWidth, std::io::Error> {

compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs

@@ -27,7 +27,7 @@ pub(crate) struct GeneratedBrillig<F> {
     pub(crate) locations: BTreeMap<OpcodeLocation, CallStack>,
     pub(crate) error_types: BTreeMap<ErrorSelector, ErrorType>,
     pub(crate) name: String,
-    pub(crate) procedure_locations: HashMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
+    pub(crate) procedure_locations: BTreeMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
 }
 
 #[derive(Default, Debug, Clone)]
@@ -61,13 +61,13 @@ pub(crate) struct BrilligArtifact<F> {
     /// 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<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
+    pub(crate) procedure_locations: BTreeMap<ProcedureId, (OpcodeLocation, OpcodeLocation)>,
 }
 
 /// 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<usize>,

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 @@
     back_edge_start: BasicBlockId,
 
     /// All the blocks contained within the loop, including `header` and `back_edge_start`.
-    pub(super) blocks: HashSet<BasicBlockId>,
+    pub(super) blocks: BTreeSet<BasicBlockId>,
 }
 
 pub(super) struct Loops {
@@ -238,7 +240,7 @@
         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<BasicBlockId>| {
@@ -1008,7 +1010,7 @@
     use super::{is_new_size_ok, BoilerplateStats, Loops};
 
     /// 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<RuntimeError>) {

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);
 

tooling/nargo_cli/build.rs

@@ -180,7 +180,7 @@
     // We need to isolate test cases in the same group, otherwise they overwrite each other's artifacts.
     // On CI we use `cargo nextest`, which runs tests in different processes; for this we use a file lock.
     // Locally we might be using `cargo test`, which run tests in the same process; in this case the file lock
     // wouldn't work, becuase the process itself has the lock, and it looks like it can have N instances without
     // any problems; for this reason we also use a `Mutex`.
     let mutex_name = format! {"TEST_MUTEX_{}", test_name.to_uppercase()};
     write!(
@@ -215,6 +215,9 @@
         // 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}

tooling/nargo_cli/src/cli/compile_cmd.rs

@@ -74,7 +74,7 @@
 fn watch_workspace(workspace: &Workspace, compile_options: &CompileOptions) -> notify::Result<()> {
     let (tx, rx) = std::sync::mpsc::channel();
 
     // No specific tickrate, max debounce time 1 seconds
     let mut debouncer = new_debouncer(Duration::from_secs(1), None, tx)?;
 
     // Add a path to be watched. All files and directories at that path and
@@ -82,7 +82,7 @@
     debouncer.watcher().watch(&workspace.root_dir, RecursiveMode::Recursive)?;
 
     let mut screen = std::io::stdout();
     write!(screen, "{}", termion::cursor::Save).unwrap();
     screen.flush().unwrap();
     let _ = compile_workspace_full(workspace, compile_options);
     for res in rx {
@@ -103,7 +103,7 @@
         });
 
         if noir_files_modified {
             write!(screen, "{}{}", termion::cursor::Restore, termion::clear::AfterCursor).unwrap();
             screen.flush().unwrap();
             let _ = compile_workspace_full(workspace, compile_options);
         }
@@ -216,6 +216,20 @@
             cached_program,
         )?;
 
+        if compile_options.check_non_determinism {
+            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);