Merge 2c99d66 into bf4176f

.noir-sync-commit

@@ -1 +1 @@
-13856a121125b1ccca15919942081a5d157d280e
+852c87ae9ecdd441ee4c2ab3e78e86b2da07d8a4

noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs

@@ -96,28 +96,28 @@ 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:")
-    .run_pass(Ssa::simplify_cfg, "After Simplifying:")
+    .run_pass(Ssa::mem2reg, "After Mem2Reg (1st):")
+    .run_pass(Ssa::simplify_cfg, "After Simplifying (1st):")
     .run_pass(Ssa::as_slice_optimization, "After `as_slice` optimization")
     .try_run_pass(
         Ssa::evaluate_static_assert_and_assert_constant,
         "After `static_assert` and `assert_constant`:",
     )?
     .try_run_pass(Ssa::unroll_loops_iteratively, "After Unrolling:")?
-    .run_pass(Ssa::simplify_cfg, "After Simplifying:")
+    .run_pass(Ssa::simplify_cfg, "After Simplifying (2nd):")
     .run_pass(Ssa::flatten_cfg, "After Flattening:")
     .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:")
     // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores
-    .run_pass(Ssa::mem2reg, "After Mem2Reg:")
+    .run_pass(Ssa::mem2reg, "After Mem2Reg (2nd):")
     // Run the inlining pass again to handle functions with `InlineType::NoPredicates`.
     // Before flattening is run, we treat functions marked with the `InlineType::NoPredicates` as an entry point.
     // This pass must come immediately following `mem2reg` as the succeeding passes
     // 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:")

noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs

@@ -28,14 +28,21 @@ impl Ssa {
 
 impl Function {
     pub(crate) fn array_set_optimization(&mut self) {
+        if matches!(self.runtime(), RuntimeType::Brillig(_)) {
+            // Brillig is supposed to use refcounting to decide whether to mutate an array;
+            // array mutation was only meant for ACIR. We could use it with Brillig as well,
+            // but then some of the optimizations that we can do in ACIR around shared
+            // references have to be skipped, which makes it more cumbersome.
+            return;
+        }
+
         let reachable_blocks = self.reachable_blocks();
 
         if !self.runtime().is_entry_point() {
             assert_eq!(reachable_blocks.len(), 1, "Expected there to be 1 block remaining in Acir function for array_set optimization");
         }
 
-        let mut context =
-            Context::new(&self.dfg, matches!(self.runtime(), RuntimeType::Brillig(_)));
+        let mut context = Context::new(&self.dfg);
 
         for block in reachable_blocks.iter() {
             context.analyze_last_uses(*block);
@@ -50,20 +57,18 @@ impl Function {
 
 struct Context<'f> {
     dfg: &'f DataFlowGraph,
-    is_brillig_runtime: bool,
     array_to_last_use: HashMap<ValueId, InstructionId>,
     instructions_that_can_be_made_mutable: HashSet<InstructionId>,
     // Mapping of an array that comes from a load and whether the address
-    // it was loaded from is a reference parameter.
+    // it was loaded from is a reference parameter passed to the block.
     arrays_from_load: HashMap<ValueId, bool>,
     inner_nested_arrays: HashMap<ValueId, InstructionId>,
 }
 
 impl<'f> Context<'f> {
-    fn new(dfg: &'f DataFlowGraph, is_brillig_runtime: bool) -> Self {
+    fn new(dfg: &'f DataFlowGraph) -> Self {
         Context {
             dfg,
-            is_brillig_runtime,
             array_to_last_use: HashMap::default(),
             instructions_that_can_be_made_mutable: HashSet::default(),
             arrays_from_load: HashMap::default(),
@@ -85,43 +90,41 @@ impl<'f> Context<'f> {
                         self.instructions_that_can_be_made_mutable.remove(&existing);
                     }
                 }
-                Instruction::ArraySet { array, value, .. } => {
+                Instruction::ArraySet { array, .. } => {
                     let array = self.dfg.resolve(*array);
 
                     if let Some(existing) = self.array_to_last_use.insert(array, *instruction_id) {
                         self.instructions_that_can_be_made_mutable.remove(&existing);
                     }
-                    if self.is_brillig_runtime {
-                        let value = self.dfg.resolve(*value);
-
-                        if let Some(existing) = self.inner_nested_arrays.get(&value) {
-                            self.instructions_that_can_be_made_mutable.remove(existing);
-                        }
-                        let result = self.dfg.instruction_results(*instruction_id)[0];
-                        self.inner_nested_arrays.insert(result, *instruction_id);
-                    }
 
                     // If the array we are setting does not come from a load we can safely mark it mutable.
                     // If the array comes from a load we may potentially being mutating an array at a reference
                     // that is loaded from by other values.
                     let terminator = self.dfg[block_id].unwrap_terminator();
+
                     // If we are in a return block we are not concerned about the array potentially being mutated again.
                     let is_return_block =
                         matches!(terminator, TerminatorInstruction::Return { .. });
+
                     // We also want to check that the array is not part of the terminator arguments, as this means it is used again.
-                    let mut array_in_terminator = false;
+                    let mut is_array_in_terminator = false;
                     terminator.for_each_value(|value| {
-                        if value == array {
-                            array_in_terminator = true;
+                        // The terminator can contain original IDs, while the SSA has replaced the array value IDs; we need to resolve to compare.
+                        if !is_array_in_terminator && self.dfg.resolve(value) == array {
+                            is_array_in_terminator = true;
                         }
                     });
-                    if let Some(is_from_param) = self.arrays_from_load.get(&array) {
+
+                    let can_mutate = if let Some(is_from_param) = self.arrays_from_load.get(&array)
+                    {
                         // If the array was loaded from a reference parameter, we cannot
                         // safely mark that array mutable as it may be shared by another value.
-                        if !is_from_param && is_return_block {
-                            self.instructions_that_can_be_made_mutable.insert(*instruction_id);
-                        }
-                    } else if !array_in_terminator {
+                        !is_from_param && is_return_block
+                    } else {
+                        !is_array_in_terminator
+                    };
+
+                    if can_mutate {
                         self.instructions_that_can_be_made_mutable.insert(*instruction_id);
                     }
                 }

noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

@@ -40,54 +40,47 @@ use fxhash::FxHashMap as HashMap;
 impl Ssa {
     /// Loop unrolling can return errors, since ACIR functions need to be fully unrolled.
     /// This meta-pass will keep trying to unroll loops and simplifying the SSA until no more errors are found.
+    #[tracing::instrument(level = "trace", skip(ssa))]
     pub(crate) fn unroll_loops_iteratively(mut ssa: Ssa) -> Result<Ssa, RuntimeError> {
-        // Try to unroll loops first:
-        let mut unroll_errors;
-        (ssa, unroll_errors) = ssa.try_to_unroll_loops();
-
-        // Keep unrolling until no more errors are found
-        while !unroll_errors.is_empty() {
-            let prev_unroll_err_count = unroll_errors.len();
-
-            // Simplify the SSA before retrying
-
-            // Do a mem2reg after the last unroll to aid simplify_cfg
-            ssa = ssa.mem2reg();
-            ssa = ssa.simplify_cfg();
-            // Do another mem2reg after simplify_cfg to aid the next unroll
-            ssa = ssa.mem2reg();
-
-            // Unroll again
-            (ssa, unroll_errors) = ssa.try_to_unroll_loops();
-            // If we didn't manage to unroll any more loops, exit
-            if unroll_errors.len() >= prev_unroll_err_count {
-                return Err(unroll_errors.swap_remove(0));
+        let acir_functions = ssa.functions.iter_mut().filter(|(_, func)| {
+            // Loop unrolling in brillig can lead to a code explosion currently. This can
+            // also be true for ACIR, but we have no alternative to unrolling in ACIR.
+            // Brillig also generally prefers smaller code rather than faster code.
+            !matches!(func.runtime(), RuntimeType::Brillig(_))
+        });
+
+        for (_, function) in acir_functions {
+            // Try to unroll loops first:
+            let mut unroll_errors = function.try_to_unroll_loops();
+
+            // Keep unrolling until no more errors are found
+            while !unroll_errors.is_empty() {
+                let prev_unroll_err_count = unroll_errors.len();
+
+                // Simplify the SSA before retrying
+
+                // Do a mem2reg after the last unroll to aid simplify_cfg
+                function.mem2reg();
+                function.simplify_function();
+                // Do another mem2reg after simplify_cfg to aid the next unroll
+                function.mem2reg();
+
+                // Unroll again
+                unroll_errors = function.try_to_unroll_loops();
+                // If we didn't manage to unroll any more loops, exit
+                if unroll_errors.len() >= prev_unroll_err_count {
+                    return Err(unroll_errors.swap_remove(0));
+                }
             }
         }
-        Ok(ssa)
-    }
 
-    /// Tries to unroll all loops in each SSA function.
-    /// If any loop cannot be unrolled, it is left as-is or in a partially unrolled state.
-    /// Returns the ssa along with all unrolling errors encountered
-    #[tracing::instrument(level = "trace", skip(self))]
-    pub(crate) fn try_to_unroll_loops(mut self) -> (Ssa, Vec<RuntimeError>) {
-        let mut errors = vec![];
-        for function in self.functions.values_mut() {
-            function.try_to_unroll_loops(&mut errors);
-        }
-        (self, errors)
+        Ok(ssa)
     }
 }
 
 impl Function {
-    pub(crate) fn try_to_unroll_loops(&mut self, errors: &mut Vec<RuntimeError>) {
-        // Loop unrolling in brillig can lead to a code explosion currently. This can
-        // also be true for ACIR, but we have no alternative to unrolling in ACIR.
-        // Brillig also generally prefers smaller code rather than faster code.
-        if !matches!(self.runtime(), RuntimeType::Brillig(_)) {
-            errors.extend(find_all_loops(self).unroll_each_loop(self));
-        }
+    fn try_to_unroll_loops(&mut self) -> Vec<RuntimeError> {
+        find_all_loops(self).unroll_each_loop(self)
     }
 }
 
@@ -507,11 +500,26 @@ impl<'f> LoopIteration<'f> {
 
 #[cfg(test)]
 mod tests {
-    use crate::ssa::{
-        function_builder::FunctionBuilder,
-        ir::{instruction::BinaryOp, map::Id, types::Type},
+    use crate::{
+        errors::RuntimeError,
+        ssa::{
+            function_builder::FunctionBuilder,
+            ir::{instruction::BinaryOp, map::Id, types::Type},
+        },
     };
 
+    use super::Ssa;
+
+    /// Tries to unroll all loops in each SSA function.
+    /// If any loop cannot be unrolled, it is left as-is or in a partially unrolled state.
+    fn try_to_unroll_loops(mut ssa: Ssa) -> (Ssa, Vec<RuntimeError>) {
+        let mut errors = vec![];
+        for function in ssa.functions.values_mut() {
+            errors.extend(function.try_to_unroll_loops());
+        }
+        (ssa, errors)
+    }
+
     #[test]
     fn unroll_nested_loops() {
         // fn main() {
@@ -630,7 +638,7 @@ mod tests {
         // }
         // The final block count is not 1 because unrolling creates some unnecessary jmps.
         // If a simplify cfg pass is ran afterward, the expected block count will be 1.
-        let (ssa, errors) = ssa.try_to_unroll_loops();
+        let (ssa, errors) = try_to_unroll_loops(ssa);
         assert_eq!(errors.len(), 0, "All loops should be unrolled");
         assert_eq!(ssa.main().reachable_blocks().len(), 5);
     }
@@ -680,7 +688,7 @@ mod tests {
         assert_eq!(ssa.main().reachable_blocks().len(), 4);
 
         // Expected that we failed to unroll the loop
-        let (_, errors) = ssa.try_to_unroll_loops();
+        let (_, errors) = try_to_unroll_loops(ssa);
         assert_eq!(errors.len(), 1, "Expected to fail to unroll loop");
     }
 }

noir/noir-repo/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);
     }

noir/noir-repo/tooling/nargo_cli/build.rs

@@ -237,7 +237,7 @@ 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: false,
+                vary_inliner: true,
                 min_inliner: INLINER_MIN_OVERRIDES
                     .iter()
                     .find(|(n, _)| *n == test_name.as_str())