Merge branch 'master' into mv/inliner-setting-brillig-reports

.github/critical_libraries_status/.gitignore

@@ -0,0 +1,3 @@
+.actual.jsonl
+.actual.jsonl.jq
+.failures.jsonl.jq

.github/workflows/reports.yml

@@ -321,16 +321,16 @@ jobs:
       fail-fast: false
       matrix:
         include:
-          # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, is_library: true }
+          # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts, cannot_execute: true }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner, num_runs: 5 }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail, num_runs: 5  }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset, num_runs: 5 }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private, num_runs: 5 }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public, num_runs: 5 }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge, num_runs: 5 }
-          #- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty }
-          #- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-single-tx }
-          #- project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty, num_runs: 5, cannot_execute: true }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-single-tx, num_runs: 1, flags: "--skip-brillig-constraints-check --skip-underconstrained-check", cannot_execute: true }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root, num_runs: 1, flags: "--skip-brillig-constraints-check --skip-underconstrained-check" }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge, num_runs: 5 }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root, num_runs: 5 }
 
@@ -374,10 +374,12 @@ jobs:
           chmod +x parse_time.sh
           cp /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh
           ./compilation_report.sh 1 ${{ matrix.project.num_runs }}
-      
+        env:
+          FLAGS: ${{ matrix.project.flags }}
+
       - name: Generate execution report
         working-directory: ./test-repo/${{ matrix.project.path }}
-        if: ${{ !matrix.project.is_library }}
+        if: ${{ !matrix.project.cannot_execute }}
         run: |
           mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh
           mv /home/runner/work/noir/noir/scripts/test_programs/parse_time.sh ./parse_time.sh
@@ -395,7 +397,7 @@ jobs:
       - name: Move execution report
         id: execution_report
         shell: bash
-        if: ${{ !matrix.project.is_library }}
+        if: ${{ !matrix.project.cannot_execute }}
         run: |
           PACKAGE_NAME=${{ matrix.project.path }}
           PACKAGE_NAME=$(basename $PACKAGE_NAME)
@@ -473,10 +475,16 @@ jobs:
           # TODO: Bring this report back under a flag. The `noir-contracts` report takes just under 30 minutes.
           # - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-contracts }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-inner }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail  }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-reset }
-          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/private-kernel-tail }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-private }
           - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-base-public }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-merge }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-empty, cannot_execute: true }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root-single-tx, flags: "--skip-brillig-constraints-check --skip-underconstrained-check", cannot_execute: true }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-block-root, flags: "--skip-brillig-constraints-check --skip-underconstrained-check" }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-merge }
+          - project: { repo: AztecProtocol/aztec-packages, path: noir-projects/noir-protocol-circuits/crates/rollup-root }
 
     name: External repo memory report - ${{ matrix.project.repo }}/${{ matrix.project.path }}
     steps:
@@ -515,9 +523,12 @@ jobs:
           ./memory_report.sh 1
           # Rename the memory report as the execution report is about to write to the same file
           cp memory_report.json compilation_memory_report.json
+        env:
+          FLAGS: ${{ matrix.project.flags }}
 
       - name: Generate execution memory report
         working-directory: ./test-repo/${{ matrix.project.path }}
+        if: ${{ !matrix.project.cannot_execute }}
         run: |
           ./memory_report.sh 1 1
 
@@ -540,6 +551,7 @@ jobs:
 
       - name: Move execution report
         id: execution_mem_report
+        if: ${{ !matrix.project.cannot_execute }}
         shell: bash
         run: |
           PACKAGE_NAME=${{ matrix.project.path }}

CRITICAL_NOIR_LIBRARIES

@@ -1,3 +1,4 @@
+https://github.com/noir-lang/noir_check_shuffle
 https://github.com/noir-lang/ec
 https://github.com/noir-lang/eddsa
 https://github.com/noir-lang/mimc

acvm-repo/acvm/src/compiler/mod.rs

@@ -16,10 +16,6 @@ pub use simulator::CircuitSimulator;
 use transformers::transform_internal;
 pub use transformers::{transform, MIN_EXPRESSION_WIDTH};
 
-/// We need multiple passes to stabilize the output.
-/// The value was determined by running tests.
-const MAX_OPTIMIZER_PASSES: usize = 3;
-
 /// This module moves and decomposes acir opcodes. The transformation map allows consumers of this module to map
 /// metadata they had about the opcodes to the new opcode structure generated after the transformation.
 #[derive(Debug)]
@@ -84,41 +80,10 @@ pub fn compile<F: AcirField>(
 ) -> (Circuit<F>, AcirTransformationMap) {
     let acir_opcode_positions = (0..acir.opcodes.len()).collect::<Vec<_>>();
 
-    if MAX_OPTIMIZER_PASSES == 0 {
-        return (acir, AcirTransformationMap::new(&acir_opcode_positions));
-    }
-
-    let mut pass = 0;
-    let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes);
-    let mut prev_acir = acir;
-    let mut prev_acir_opcode_positions = acir_opcode_positions;
-
-    // For most test programs it would be enough to only loop `transform_internal`,
-    // but some of them don't stabilize unless we also repeat the backend agnostic optimizations.
-    let (mut acir, acir_opcode_positions) = loop {
-        let (acir, acir_opcode_positions) =
-            optimize_internal(prev_acir, prev_acir_opcode_positions);
-
-        // Stop if we have already done at least one transform and an extra optimization changed nothing.
-        if pass > 0 && prev_opcodes_hash == fxhash::hash64(&acir.opcodes) {
-            break (acir, acir_opcode_positions);
-        }
-
-        let (acir, acir_opcode_positions) =
-            transform_internal(acir, expression_width, acir_opcode_positions);
-
-        let opcodes_hash = fxhash::hash64(&acir.opcodes);
-
-        // Stop if the output hasn't change in this loop or we went too long.
-        if pass == MAX_OPTIMIZER_PASSES - 1 || prev_opcodes_hash == opcodes_hash {
-            break (acir, acir_opcode_positions);
-        }
+    let (acir, acir_opcode_positions) = optimize_internal(acir, acir_opcode_positions);
 
-        pass += 1;
-        prev_acir = acir;
-        prev_opcodes_hash = opcodes_hash;
-        prev_acir_opcode_positions = acir_opcode_positions;
-    };
+    let (mut acir, acir_opcode_positions) =
+        transform_internal(acir, expression_width, acir_opcode_positions);
 
     let transformation_map = AcirTransformationMap::new(&acir_opcode_positions);
     acir.assert_messages = transform_assert_messages(acir.assert_messages, &transformation_map);

acvm-repo/acvm/src/compiler/transformers/mod.rs

@@ -14,12 +14,17 @@ mod csat;
 
 pub(crate) use csat::CSatTransformer;
 pub use csat::MIN_EXPRESSION_WIDTH;
+use tracing::info;
 
 use super::{
-    optimizers::MergeExpressionsOptimizer, transform_assert_messages, AcirTransformationMap,
-    MAX_OPTIMIZER_PASSES,
+    optimizers::{MergeExpressionsOptimizer, RangeOptimizer},
+    transform_assert_messages, AcirTransformationMap,
 };
 
+/// We need multiple passes to stabilize the output.
+/// The value was determined by running tests.
+const MAX_TRANSFORMER_PASSES: usize = 3;
+
 /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] specific optimizations to a [`Circuit`].
 pub fn transform<F: AcirField>(
     acir: Circuit<F>,
@@ -50,12 +55,18 @@ pub(super) fn transform_internal<F: AcirField>(
     expression_width: ExpressionWidth,
     mut acir_opcode_positions: Vec<usize>,
 ) -> (Circuit<F>, Vec<usize>) {
+    if acir.opcodes.len() == 1 && matches!(acir.opcodes[0], Opcode::BrilligCall { .. }) {
+        info!("Program is fully unconstrained, skipping transformation pass");
+        return (acir, acir_opcode_positions);
+    }
+
     // Allow multiple passes until we have stable output.
     let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes);
 
     // For most test programs it would be enough to loop here, but some of them
     // don't stabilize unless we also repeat the backend agnostic optimizations.
-    for _ in 0..MAX_OPTIMIZER_PASSES {
+    for _ in 0..MAX_TRANSFORMER_PASSES {
+        info!("Number of opcodes {}", acir.opcodes.len());
         let (new_acir, new_acir_opcode_positions) =
             transform_internal_once(acir, expression_width, acir_opcode_positions);
 
@@ -217,6 +228,12 @@ fn transform_internal_once<F: AcirField>(
         ..acir
     };
 
+    // The `MergeOptimizer` can merge two witnesses which have range opcodes applied to them
+    // so we run the `RangeOptimizer` afterwards to clear these up.
+    let range_optimizer = RangeOptimizer::new(acir);
+    let (acir, new_acir_opcode_positions) =
+        range_optimizer.replace_redundant_ranges(new_acir_opcode_positions);
+
     (acir, new_acir_opcode_positions)
 }
 

compiler/noirc_evaluator/Cargo.toml

@@ -29,6 +29,7 @@ chrono = "0.4.37"
 rayon.workspace = true
 cfg-if.workspace = true
 smallvec = { version = "1.13.2", features = ["serde"] }
+vec-collections = "0.4.3"
 
 [dev-dependencies]
 proptest.workspace = true

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

@@ -79,6 +79,7 @@ mod block;
 use std::collections::{BTreeMap, BTreeSet};
 
 use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
+use vec_collections::VecSet;
 
 use crate::ssa::{
     ir::{
@@ -619,7 +620,7 @@ impl<'f> PerFunctionContext<'f> {
                 // then those parameters also alias each other.
                 // We save parameters with repeat arguments to later mark those
                 // parameters as aliasing one another.
-                let mut arg_set: HashMap<ValueId, BTreeSet<ValueId>> = HashMap::default();
+                let mut arg_set = HashMap::default();
 
                 // Add an alias for each reference parameter
                 for (parameter, argument) in destination_parameters.iter().zip(arguments) {
@@ -632,7 +633,8 @@ impl<'f> PerFunctionContext<'f> {
                                 aliases.insert(*parameter);
 
                                 // Check if we have seen the same argument
-                                let seen_parameters = arg_set.entry(argument).or_default();
+                                let seen_parameters =
+                                    arg_set.entry(argument).or_insert_with(VecSet::empty);
                                 // Add the current parameter to the parameters we have seen for this argument.
                                 // The previous parameters and the current one alias one another.
                                 seen_parameters.insert(*parameter);

compiler/noirc_evaluator/src/ssa/opt/mem2reg/alias_set.rs

@@ -1,4 +1,4 @@
-use std::collections::BTreeSet;
+use vec_collections::{AbstractVecSet, VecSet};
 
 use crate::ssa::ir::value::ValueId;
 
@@ -10,7 +10,7 @@ use crate::ssa::ir::value::ValueId;
 /// "unknown which aliases this may refer to" - `None`.
 #[derive(Debug, Default, Clone)]
 pub(super) struct AliasSet {
-    aliases: Option<BTreeSet<ValueId>>,
+    aliases: Option<VecSet<[ValueId; 1]>>,
 }
 
 impl AliasSet {
@@ -19,20 +19,18 @@ impl AliasSet {
     }
 
     pub(super) fn known(value: ValueId) -> AliasSet {
-        let mut aliases = BTreeSet::new();
-        aliases.insert(value);
-        Self { aliases: Some(aliases) }
+        Self { aliases: Some(VecSet::single(value)) }
     }
 
-    pub(super) fn known_multiple(values: BTreeSet<ValueId>) -> AliasSet {
+    pub(super) fn known_multiple(values: VecSet<[ValueId; 1]>) -> AliasSet {
         Self { aliases: Some(values) }
     }
 
     /// In rare cases, such as when creating an empty array of references, the set of aliases for a
     /// particular value will be known to be zero, which is distinct from being unknown and
     /// possibly referring to any alias.
     pub(super) fn known_empty() -> AliasSet {
-        Self { aliases: Some(BTreeSet::new()) }
+        Self { aliases: Some(VecSet::empty()) }
     }
 
     pub(super) fn is_unknown(&self) -> bool {
@@ -44,19 +42,33 @@ impl AliasSet {
     pub(super) fn single_alias(&self) -> Option<ValueId> {
         self.aliases
             .as_ref()
-            .and_then(|aliases| (aliases.len() == 1).then(|| *aliases.first().unwrap()))
+            .and_then(|aliases| (aliases.len() == 1).then(|| *aliases.iter().next().unwrap()))
     }
 
     /// Unify this alias set with another. The result of this set is empty if either set is empty.
     /// Otherwise, it is the union of both alias sets.
     pub(super) fn unify(&mut self, other: &Self) {
         if let (Some(self_aliases), Some(other_aliases)) = (&mut self.aliases, &other.aliases) {
-            self_aliases.extend(other_aliases);
+            self_aliases.extend(other_aliases.iter().cloned());
         } else {
             self.aliases = None;
         }
     }
 
+    /// Returns true if calling `unify` would change something in this alias set.
+    ///
+    /// This is an optimization to avoid having to look up an entry ready to be modified in the [Block](crate::ssa::opt::mem2reg::block::Block),
+    /// because doing so would involve calling `Arc::make_mut` which clones the entry, ready for modification.
+    pub(super) fn should_unify(&self, other: &Self) -> bool {
+        if let (Some(self_aliases), Some(other_aliases)) = (&self.aliases, &other.aliases) {
+            // `unify` would extend `self_aliases` with `other_aliases`, so if `other_aliases` is a subset, then nothing would happen.
+            !other_aliases.is_subset(self_aliases)
+        } else {
+            // `unify` would set `aliases` to `None`, so if it's not `Some`, then nothing would happen.
+            self.aliases.is_some()
+        }
+    }
+
     /// Inserts a new alias into this set if it is not unknown
     pub(super) fn insert(&mut self, new_alias: ValueId) {
         if let Some(aliases) = &mut self.aliases {
@@ -82,6 +94,6 @@ impl AliasSet {
     /// The ordering is arbitrary (by lowest ValueId) so this method should only be
     /// used when you need an arbitrary ValueId from the alias set.
     pub(super) fn first(&self) -> Option<ValueId> {
-        self.aliases.as_ref().and_then(|aliases| aliases.first().copied())
+        self.aliases.as_ref().and_then(|aliases| aliases.iter().next().copied())
     }
 }