merge conflicts w/ master

Cargo.toml

@@ -157,6 +157,8 @@ proptest-derive = "0.4.0"
 rayon = "1.8.0"
 sha2 = { version = "0.10.6", features = ["compress"] }
 sha3 = "0.10.6"
+strum = "0.24"
+strum_macros = "0.24"
 
 im = { version = "15.1", features = ["serde"] }
 tracing = "0.1.40"

acvm-repo/acir/Cargo.toml

@@ -24,11 +24,11 @@ flate2.workspace = true
 bincode.workspace = true
 base64.workspace = true
 serde-big-array = "0.5.1"
+strum = { workspace = true }
+strum_macros = { workspace = true }
 
 [dev-dependencies]
 serde_json = "1.0"
-strum = "0.24"
-strum_macros = "0.24"
 serde-reflection = "0.3.6"
 serde-generate = "0.25.1"
 fxhash.workspace = true

acvm-repo/acir/src/circuit/black_box_functions.rs

@@ -4,12 +4,10 @@
 //! implemented in more basic constraints.
 
 use serde::{Deserialize, Serialize};
-#[cfg(test)]
 use strum_macros::EnumIter;
 
 #[allow(clippy::upper_case_acronyms)]
-#[derive(Clone, Debug, Hash, Copy, PartialEq, Eq, Serialize, Deserialize)]
-#[cfg_attr(test, derive(EnumIter))]
+#[derive(Clone, Debug, Hash, Copy, PartialEq, Eq, Serialize, Deserialize, EnumIter)]
 pub enum BlackBoxFunc {
     /// Ciphers (encrypts) the provided plaintext using AES128 in CBC mode,
     /// padding the input using PKCS#7.

acvm-repo/acvm/Cargo.toml

@@ -25,22 +25,19 @@ acvm_blackbox_solver.workspace = true
 indexmap = "1.7.0"
 
 [features]
-bn254 = [
-    "acir/bn254",
-    "brillig_vm/bn254",
-    "acvm_blackbox_solver/bn254",
-]
+bn254 = ["acir/bn254", "brillig_vm/bn254", "acvm_blackbox_solver/bn254"]
 bls12_381 = [
     "acir/bls12_381",
     "brillig_vm/bls12_381",
     "acvm_blackbox_solver/bls12_381",
 ]
 
 [dev-dependencies]
-ark-bls12-381 = { version = "^0.4.0", default-features = false, features = ["curve"] }
+ark-bls12-381 = { version = "^0.4.0", default-features = false, features = [
+    "curve",
+] }
 ark-bn254.workspace = true
 bn254_blackbox_solver.workspace = true
 proptest.workspace = true
 zkhash = { version = "^0.2.0", default-features = false }
 num-bigint.workspace = true
-

acvm-repo/blackbox_solver/src/bigint.rs

@@ -97,3 +97,51 @@ impl BigIntSolver {
         Ok(())
     }
 }
+
+/// Wrapper over the generic bigint solver to automatically assign bigint IDs.
+#[derive(Default, Debug, Clone, PartialEq, Eq)]
+pub struct BigIntSolverWithId {
+    solver: BigIntSolver,
+    last_id: u32,
+}
+
+impl BigIntSolverWithId {
+    pub fn create_bigint_id(&mut self) -> u32 {
+        let output = self.last_id;
+        self.last_id += 1;
+        output
+    }
+
+    pub fn bigint_from_bytes(
+        &mut self,
+        inputs: &[u8],
+        modulus: &[u8],
+    ) -> Result<u32, BlackBoxResolutionError> {
+        let id = self.create_bigint_id();
+        self.solver.bigint_from_bytes(inputs, modulus, id)?;
+        Ok(id)
+    }
+
+    pub fn bigint_to_bytes(&self, input: u32) -> Result<Vec<u8>, BlackBoxResolutionError> {
+        self.solver.bigint_to_bytes(input)
+    }
+
+    pub fn bigint_op(
+        &mut self,
+        lhs: u32,
+        rhs: u32,
+        func: BlackBoxFunc,
+    ) -> Result<u32, BlackBoxResolutionError> {
+        let modulus_lhs = self.solver.get_modulus(lhs, func)?;
+        let modulus_rhs = self.solver.get_modulus(rhs, func)?;
+        if modulus_lhs != modulus_rhs {
+            return Err(BlackBoxResolutionError::Failed(
+                func,
+                "moduli should be identical in BigInt operation".to_string(),
+            ));
+        }
+        let id = self.create_bigint_id();
+        self.solver.bigint_op(lhs, rhs, id, func)?;
+        Ok(id)
+    }
+}

acvm-repo/blackbox_solver/src/lib.rs

@@ -18,7 +18,7 @@ mod hash;
 mod logic;
 
 pub use aes128::aes128_encrypt;
-pub use bigint::BigIntSolver;
+pub use bigint::{BigIntSolver, BigIntSolverWithId};
 pub use curve_specific_solver::{BlackBoxFunctionSolver, StubbedBlackBoxSolver};
 pub use ecdsa::{ecdsa_secp256k1_verify, ecdsa_secp256r1_verify};
 pub use hash::{blake2s, blake3, keccakf1600, sha256_compression};

acvm-repo/brillig_vm/src/black_box.rs

@@ -1,9 +1,8 @@
 use acir::brillig::{BlackBoxOp, HeapArray, HeapVector, IntegerBitSize};
 use acir::{AcirField, BlackBoxFunc};
-use acvm_blackbox_solver::BigIntSolver;
 use acvm_blackbox_solver::{
     aes128_encrypt, blake2s, blake3, ecdsa_secp256k1_verify, ecdsa_secp256r1_verify, keccakf1600,
-    sha256_compression, BlackBoxFunctionSolver, BlackBoxResolutionError,
+    sha256_compression, BigIntSolverWithId, BlackBoxFunctionSolver, BlackBoxResolutionError,
 };
 use num_bigint::BigUint;
 use num_traits::Zero;
@@ -39,11 +38,13 @@ fn to_value_vec<F: AcirField>(input: &[u8]) -> Vec<MemoryValue<F>> {
     input.iter().map(|&x| x.into()).collect()
 }
 
+pub(crate) type BrilligBigIntSolver = BigIntSolverWithId;
+
 pub(crate) fn evaluate_black_box<F: AcirField, Solver: BlackBoxFunctionSolver<F>>(
     op: &BlackBoxOp,
     solver: &Solver,
     memory: &mut Memory<F>,
-    bigint_solver: &mut BrilligBigintSolver,
+    bigint_solver: &mut BrilligBigIntSolver,
 ) -> Result<(), BlackBoxResolutionError> {
     match op {
         BlackBoxOp::AES128Encrypt { inputs, iv, key, outputs } => {
@@ -56,7 +57,7 @@ pub(crate) fn evaluate_black_box<F: AcirField, Solver: BlackBoxFunctionSolver<F>
             })?;
             let key: [u8; 16] =
                 to_u8_vec(read_heap_array(memory, key)).try_into().map_err(|_| {
-                    BlackBoxResolutionError::Failed(bb_func, "Invalid ley length".to_string())
+                    BlackBoxResolutionError::Failed(bb_func, "Invalid key length".to_string())
                 })?;
             let ciphertext = aes128_encrypt(&inputs, iv, key)?;
 
@@ -353,54 +354,6 @@ pub(crate) fn evaluate_black_box<F: AcirField, Solver: BlackBoxFunctionSolver<F>
     }
 }
 
-/// Wrapper over the generic bigint solver to automatically assign bigint ids in brillig
-#[derive(Default, Debug, Clone, PartialEq, Eq)]
-pub(crate) struct BrilligBigintSolver {
-    bigint_solver: BigIntSolver,
-    last_id: u32,
-}
-
-impl BrilligBigintSolver {
-    pub(crate) fn create_bigint_id(&mut self) -> u32 {
-        let output = self.last_id;
-        self.last_id += 1;
-        output
-    }
-
-    pub(crate) fn bigint_from_bytes(
-        &mut self,
-        inputs: &[u8],
-        modulus: &[u8],
-    ) -> Result<u32, BlackBoxResolutionError> {
-        let id = self.create_bigint_id();
-        self.bigint_solver.bigint_from_bytes(inputs, modulus, id)?;
-        Ok(id)
-    }
-
-    pub(crate) fn bigint_to_bytes(&self, input: u32) -> Result<Vec<u8>, BlackBoxResolutionError> {
-        self.bigint_solver.bigint_to_bytes(input)
-    }
-
-    pub(crate) fn bigint_op(
-        &mut self,
-        lhs: u32,
-        rhs: u32,
-        func: BlackBoxFunc,
-    ) -> Result<u32, BlackBoxResolutionError> {
-        let modulus_lhs = self.bigint_solver.get_modulus(lhs, func)?;
-        let modulus_rhs = self.bigint_solver.get_modulus(rhs, func)?;
-        if modulus_lhs != modulus_rhs {
-            return Err(BlackBoxResolutionError::Failed(
-                func,
-                "moduli should be identical in BigInt operation".to_string(),
-            ));
-        }
-        let id = self.create_bigint_id();
-        self.bigint_solver.bigint_op(lhs, rhs, id, func)?;
-        Ok(id)
-    }
-}
-
 fn black_box_function_from_op(op: &BlackBoxOp) -> BlackBoxFunc {
     match op {
         BlackBoxOp::AES128Encrypt { .. } => BlackBoxFunc::AES128Encrypt,

acvm-repo/brillig_vm/src/lib.rs

@@ -17,7 +17,7 @@ use acir::brillig::{
 use acir::AcirField;
 use acvm_blackbox_solver::BlackBoxFunctionSolver;
 use arithmetic::{evaluate_binary_field_op, evaluate_binary_int_op, BrilligArithmeticError};
-use black_box::{evaluate_black_box, BrilligBigintSolver};
+use black_box::{evaluate_black_box, BrilligBigIntSolver};
 
 // Re-export `brillig`.
 pub use acir::brillig;
@@ -95,7 +95,7 @@ pub struct VM<'a, F, B: BlackBoxFunctionSolver<F>> {
     /// The solver for blackbox functions
     black_box_solver: &'a B,
     // The solver for big integers
-    bigint_solver: BrilligBigintSolver,
+    bigint_solver: BrilligBigIntSolver,
     // Flag that determines whether we want to profile VM.
     profiling_active: bool,
     // Samples for profiling the VM execution.

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

@@ -131,7 +131,7 @@
 //!   v11 = mul v4, Field 12
 //!   v12 = add v10, v11
 //!   store v12 at v5         (new store)
-use fxhash::FxHashMap as HashMap;
+use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
 
 use acvm::{acir::AcirField, acir::BlackBoxFunc, FieldElement};
 use iter_extended::vecmap;
@@ -201,6 +201,15 @@ struct Context<'f> {
     /// When processing a block, we pop this stack to get its arguments
     /// and at the end we push the arguments for his successor
     arguments_stack: Vec<Vec<ValueId>>,
+
+    /// Stores all allocations local to the current branch.
+    ///
+    /// Since these branches are local to the current branch (i.e. only defined within one branch of
+    /// an if expression), they should not be merged with their previous value or stored value in
+    /// the other branch since there is no such value.
+    ///
+    /// The `ValueId` here is that which is returned by the allocate instruction.
+    local_allocations: HashSet<ValueId>,
 }
 
 #[derive(Clone)]
@@ -211,6 +220,8 @@ struct ConditionalBranch {
     old_condition: ValueId,
     // The condition of the branch
     condition: ValueId,
+    // The allocations accumulated when processing the branch
+    local_allocations: HashSet<ValueId>,
 }
 
 struct ConditionalContext {
@@ -243,6 +254,7 @@ fn flatten_function_cfg(function: &mut Function, no_predicates: &HashMap<Functio
         slice_sizes: HashMap::default(),
         condition_stack: Vec::new(),
         arguments_stack: Vec::new(),
+        local_allocations: HashSet::default(),
     };
     context.flatten(no_predicates);
 }
@@ -317,7 +329,6 @@ impl<'f> Context<'f> {
         // If this is not a separate variable, clippy gets confused and says the to_vec is
         // unnecessary, when removing it actually causes an aliasing/mutability error.
         let instructions = self.inserter.function.dfg[block].instructions().to_vec();
-        let mut previous_allocate_result = None;
 
         for instruction in instructions.iter() {
             if self.is_no_predicate(no_predicates, instruction) {
@@ -332,10 +343,10 @@ impl<'f> Context<'f> {
                     None,
                     im::Vector::new(),
                 );
-                self.push_instruction(*instruction, &mut previous_allocate_result);
+                self.push_instruction(*instruction);
                 self.insert_current_side_effects_enabled();
             } else {
-                self.push_instruction(*instruction, &mut previous_allocate_result);
+                self.push_instruction(*instruction);
             }
         }
     }
@@ -405,10 +416,12 @@ impl<'f> Context<'f> {
         let old_condition = *condition;
         let then_condition = self.inserter.resolve(old_condition);
 
+        let old_allocations = std::mem::take(&mut self.local_allocations);
         let branch = ConditionalBranch {
             old_condition,
             condition: self.link_condition(then_condition),
             last_block: *then_destination,
+            local_allocations: old_allocations,
         };
         let cond_context = ConditionalContext {
             condition: then_condition,
@@ -435,11 +448,14 @@ impl<'f> Context<'f> {
         );
         let else_condition = self.link_condition(else_condition);
 
+        let old_allocations = std::mem::take(&mut self.local_allocations);
         let else_branch = ConditionalBranch {
             old_condition: cond_context.then_branch.old_condition,
             condition: else_condition,
             last_block: *block,
+            local_allocations: old_allocations,
         };
+        cond_context.then_branch.local_allocations.clear();
         cond_context.else_branch = Some(else_branch);
         self.condition_stack.push(cond_context);
 
@@ -461,6 +477,7 @@ impl<'f> Context<'f> {
         }
 
         let mut else_branch = cond_context.else_branch.unwrap();
+        self.local_allocations = std::mem::take(&mut else_branch.local_allocations);
         else_branch.last_block = *block;
         cond_context.else_branch = Some(else_branch);
 
@@ -593,22 +610,19 @@ impl<'f> Context<'f> {
     /// `previous_allocate_result` should only be set to the result of an allocate instruction
     /// if that instruction was the instruction immediately previous to this one - if there are
     /// any instructions in between it should be None.
-    fn push_instruction(
-        &mut self,
-        id: InstructionId,
-        previous_allocate_result: &mut Option<ValueId>,
-    ) {
+    fn push_instruction(&mut self, id: InstructionId) {
         let (instruction, call_stack) = self.inserter.map_instruction(id);
-        let instruction = self.handle_instruction_side_effects(
-            instruction,
-            call_stack.clone(),
-            *previous_allocate_result,
-        );
+        let instruction = self.handle_instruction_side_effects(instruction, call_stack.clone());
 
         let instruction_is_allocate = matches!(&instruction, Instruction::Allocate);
         let entry = self.inserter.function.entry_block();
         let results = self.inserter.push_instruction_value(instruction, id, entry, call_stack);
-        *previous_allocate_result = instruction_is_allocate.then(|| results.first());
+
+        // Remember an allocate was created local to this branch so that we do not try to merge store
+        // values across branches for it later.
+        if instruction_is_allocate {
+            self.local_allocations.insert(results.first());
+        }
     }
 
     /// If we are currently in a branch, we need to modify constrain instructions
@@ -621,7 +635,6 @@ impl<'f> Context<'f> {
         &mut self,
         instruction: Instruction,
         call_stack: CallStack,
-        previous_allocate_result: Option<ValueId>,
     ) -> Instruction {
         if let Some(condition) = self.get_last_condition() {
             match instruction {
@@ -652,7 +665,7 @@ impl<'f> Context<'f> {
                 Instruction::Store { address, value } => {
                     // If this instruction immediately follows an allocate, and stores to that
                     // address there is no previous value to load and we don't need a merge anyway.
-                    if Some(address) == previous_allocate_result {
+                    if self.local_allocations.contains(&address) {
                         Instruction::Store { address, value }
                     } else {
                         // Instead of storing `value`, store `if condition { value } else { previous_value }`