fix(ssa): Do not unroll loop with break

.github/benchmark_projects.yml

@@ -45,7 +45,7 @@ projects:
     path: noir-projects/noir-protocol-circuits/crates/rollup-base-public
     num_runs: 5
     timeout: 15
-    compilation-timeout: 15
+    compilation-timeout: 20
     execution-timeout: 0.75
     compilation-memory-limit: 1500
     execution-memory-limit: 500
@@ -65,7 +65,7 @@ projects:
     cannot_execute: true
     num_runs: 1
     timeout: 60
-    compilation-timeout: 150
+    compilation-timeout: 180
     compilation-memory-limit: 8000
   rollup-block-root:
     repo: AztecProtocol/aztec-packages

EXTERNAL_NOIR_LIBRARIES.yml

@@ -82,7 +82,7 @@ libraries:
     repo: AztecProtocol/aztec-packages
     ref: *AZ_COMMIT
     path: noir-projects/noir-contracts
-    timeout: 90
+    timeout: 100
     critical: false
   blob:
     repo: AztecProtocol/aztec-packages

compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs

@@ -127,6 +127,21 @@ impl Loop {
     fn get_induction_variable(&self, function: &Function) -> ValueId {
         function.dfg.block_parameters(self.header)[0]
     }
+
+    // Check if the loop will be fully executed by checking the number of predecessors of the loop exit
+    // Our SSA code generation restricts loops to having one exit block except in the case of a `break`.
+    // If a loop can have several exit blocks, we would need to update this function
+    pub(super) fn is_fully_executed(&self, cfg: &ControlFlowGraph) -> bool {
+        let Some(header) = self.blocks.first() else {
+            return true;
+        };
+        for block in cfg.successors(*header) {
+            if !self.blocks.contains(&block) {
+                return cfg.predecessors(block).len() == 1;
+            }
+        }
+        true
+    }
 }
 
 struct LoopInvariantContext<'f> {
@@ -194,20 +209,6 @@ impl<'f> LoopInvariantContext<'f> {
         self.current_pre_header.expect("ICE: Pre-header block should have been set")
     }
 
-    // Check if the loop will be fully executed by checking the number of predecessors of the loop exit
-    // If a loop can have several exit blocks, we would need to update this function
-    fn is_fully_executed(&self, loop_: &Loop) -> bool {
-        let Some(header) = loop_.blocks.first() else {
-            return true;
-        };
-        for block in self.inserter.function.dfg[*header].successors() {
-            if !loop_.blocks.contains(&block) {
-                return self.cfg.predecessors(block).len() == 1;
-            }
-        }
-        true
-    }
-
     fn hoist_loop_invariants(&mut self, loop_: &Loop) {
         self.set_values_defined_in_loop(loop_);
 
@@ -298,7 +299,7 @@ impl<'f> LoopInvariantContext<'f> {
         // set the new current induction variable.
         self.current_induction_variables.clear();
         self.set_induction_var_bounds(loop_, true);
-        self.no_break = self.is_fully_executed(loop_);
+        self.no_break = loop_.is_fully_executed(&self.cfg);
 
         for block in loop_.blocks.iter() {
             let params = self.inserter.function.dfg.block_parameters(*block);

compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

@@ -666,7 +666,9 @@
     /// of unrolled instructions times the number of iterations would result in smaller bytecode
     /// than if we keep the loops with their overheads.
     fn is_small_loop(&self, function: &Function, cfg: &ControlFlowGraph) -> bool {
-        self.boilerplate_stats(function, cfg).map(|s| s.is_small()).unwrap_or_default()
+        self.boilerplate_stats(function, cfg)
+            .map(|s| s.is_small() && self.is_fully_executed(cfg))
+            .unwrap_or_default()
     }
 
     /// Collect boilerplate stats if we can figure out the upper and lower bounds of the loop,
@@ -1039,7 +1041,7 @@
     use super::{BoilerplateStats, Loops, is_new_size_ok};
 
     /// 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>) {
@@ -1483,4 +1485,52 @@
     fn test_is_new_size_ok(old: usize, new: usize, max: i32, ok: bool) {
         assert_eq!(is_new_size_ok(old, new, max), ok);
     }
+
+    #[test]
+    fn do_not_unroll_loop_with_break() {
+        // One of the loop header's (b1) successors (b3) has multiple predecessors (b1 and b4).
+        // This logic is how we identify a loop with a break expression.
+        // We do not support unrolling these types of loops.
+        let src = r#"
+        brillig(inline) predicate_pure fn main f0 {
+          b0():
+            jmp b1(u32 0)
+          b1(v0: u32):
+            v3 = lt v0, u32 5
+            jmpif v3 then: b2, else: b3
+          b2():
+            jmpif u1 1 then: b4, else: b5
+          b3():
+            return u1 1
+          b4():
+            jmp b3()
+          b5():
+            v6 = unchecked_add v0, u32 1
+            jmp b1(v6)
+        }
+        "#;
+        let ssa = Ssa::from_str(src).unwrap();
+        let (ssa, errors) = try_unroll_loops(ssa);
+        assert_eq!(errors.len(), 0, "All loops should be unrolled");
+
+        // The SSA is expected to be unchanged
+        assert_ssa_snapshot!(ssa, @r#"
+        brillig(inline) predicate_pure fn main f0 {
+          b0():
+            jmp b1(u32 0)
+          b1(v0: u32):
+            v3 = lt v0, u32 5
+            jmpif v3 then: b2, else: b3
+          b2():
+            jmpif u1 1 then: b4, else: b5
+          b3():
+            return u1 1
+          b4():
+            jmp b3()
+          b5():
+            v6 = unchecked_add v0, u32 1
+            jmp b1(v6)
+        }
+        "#);
+    }
 }

test_programs/execution_success/loop/src/main.nr

@@ -6,6 +6,11 @@ fn main(six_as_u32: u32) {
     assert_eq(loop_incl(3), six_as_u32);
     assert(plain_loop() == six_as_u32);
     assert(never_loop() == 0);
+
+    // Safety: testing context
+    unsafe {
+        assert(basic_break() == true)
+    }
 }
 
 fn loop_excl(x: u32) -> u32 {
@@ -39,3 +44,10 @@ fn never_loop() -> u32 {
     }
     sum
 }
+
+unconstrained fn basic_break() -> bool {
+    for idx_e in 0..5 {
+        if (idx_e < 5) { break; };
+    }
+    true
+}

test_programs/execution_success/loop_small_break/Nargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "loop_small_break"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/execution_success/loop_small_break/Prover.toml

@@ -0,0 +1 @@
+x = 5

test_programs/execution_success/loop_small_break/src/main.nr

@@ -0,0 +1,13 @@
+// Regression for issue #7359 (https://github.com/noir-lang/noir/issues/7359)
+// We want the loop to be small enough that the compiler may attempt to unroll it.
+unconstrained fn main(x: Field) {
+    let mut count = 0;
+
+    for i in 0..1 {
+        if x == 5 {
+            count = i;
+            break;
+        }
+    }
+    assert(count == 0);
+}