Merge branch 'master' into jf/fix-die-rc

.aztec-sync-commit

@@ -1 +1 @@
-b8bace9a00c3a8eb93f42682e8cbfa351fc5238c
+1bfc15e08873a1f0f3743e259f418b70426b3f25

.github/workflows/test-js-packages.yml

@@ -478,6 +478,9 @@ jobs:
 
       - name: Install Foundry
         uses: foundry-rs/[email protected]
+        with:
+          version: nightly-8660e5b941fe7f4d67e246cfd3dafea330fb53b1
+
 
       - name: Install `bb`
         run: |

compiler/noirc_evaluator/src/acir/acir_variable.rs

@@ -578,7 +578,7 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
         let numeric_type = match typ {
             AcirType::NumericType(numeric_type) => numeric_type,
             AcirType::Array(_, _) => {
-                todo!("cannot divide arrays. This should have been caught by the frontend")
+                unreachable!("cannot divide arrays. This should have been caught by the frontend")
             }
         };
         match numeric_type {
@@ -1084,11 +1084,22 @@ impl<F: AcirField, B: BlackBoxFunctionSolver<F>> AcirContext<F, B> {
         &mut self,
         lhs: AcirVar,
         rhs: AcirVar,
+        typ: AcirType,
         bit_size: u32,
         predicate: AcirVar,
     ) -> Result<AcirVar, RuntimeError> {
-        let (_, remainder) = self.euclidean_division_var(lhs, rhs, bit_size, predicate)?;
-        Ok(remainder)
+        let numeric_type = match typ {
+            AcirType::NumericType(numeric_type) => numeric_type,
+            AcirType::Array(_, _) => {
+                unreachable!("cannot modulo arrays. This should have been caught by the frontend")
+            }
+        };
+
+        let (_, remainder_var) = match numeric_type {
+            NumericType::Signed { bit_size } => self.signed_division_var(lhs, rhs, bit_size)?,
+            _ => self.euclidean_division_var(lhs, rhs, bit_size, predicate)?,
+        };
+        Ok(remainder_var)
     }
 
     /// Constrains the `AcirVar` variable to be of type `NumericType`.

compiler/noirc_evaluator/src/acir/mod.rs

@@ -1968,6 +1968,7 @@ impl<'a> Context<'a> {
             BinaryOp::Mod => self.acir_context.modulo_var(
                 lhs,
                 rhs,
+                binary_type.clone(),
                 bit_count,
                 self.current_side_effects_enabled_var,
             ),

compiler/noirc_evaluator/src/ssa/ir/dom.rs

@@ -119,6 +119,29 @@ impl DominatorTree {
         }
     }
 
+    /// Walk up the dominator tree until we find a block for which `f` returns `Some` value.
+    /// Otherwise return `None` when we reach the top.
+    ///
+    /// Similar to `Iterator::filter_map` but only returns the first hit.
+    pub(crate) fn find_map_dominator<T>(
+        &self,
+        mut block_id: BasicBlockId,
+        f: impl Fn(BasicBlockId) -> Option<T>,
+    ) -> Option<T> {
+        if !self.is_reachable(block_id) {
+            return None;
+        }
+        loop {
+            if let Some(value) = f(block_id) {
+                return Some(value);
+            }
+            block_id = match self.immediate_dominator(block_id) {
+                Some(immediate_dominator) => immediate_dominator,
+                None => return None,
+            }
+        }
+    }
+
     /// Allocate and compute a dominator tree from a pre-computed control flow graph and
     /// post-order counterpart.
     pub(crate) fn with_cfg_and_post_order(cfg: &ControlFlowGraph, post_order: &PostOrder) -> Self {
@@ -448,4 +471,22 @@ mod tests {
         assert!(dt.dominates(block2_id, block1_id));
         assert!(dt.dominates(block2_id, block2_id));
     }
+
+    #[test]
+    fn test_find_map_dominator() {
+        let (dt, b0, b1, b2, _b3) = unreachable_node_setup();
+
+        assert_eq!(
+            dt.find_map_dominator(b2, |b| if b == b0 { Some("root") } else { None }),
+            Some("root")
+        );
+        assert_eq!(
+            dt.find_map_dominator(b1, |b| if b == b0 { Some("unreachable") } else { None }),
+            None
+        );
+        assert_eq!(
+            dt.find_map_dominator(b1, |b| if b == b1 { Some("not part of tree") } else { None }),
+            None
+        );
+    }
 }

compiler/noirc_evaluator/src/ssa/ir/instruction.rs

@@ -45,8 +45,7 @@ pub(crate) type InstructionId = Id<Instruction>;
 /// - Opcodes which the IR knows the target machine has
 /// special support for. (LowLevel)
 /// - Opcodes which have no function definition in the
-/// source code and must be processed by the IR. An example
-/// of this is println.
+/// source code and must be processed by the IR.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
 pub(crate) enum Intrinsic {
     ArrayLen,
@@ -112,6 +111,9 @@ impl Intrinsic {
     /// Returns whether the `Intrinsic` has side effects.
     ///
     /// If there are no side effects then the `Intrinsic` can be removed if the result is unused.
+    ///
+    /// An example of a side effect is increasing the reference count of an array, but functions
+    /// which can fail due to implicit constraints are also considered to have a side effect.
     pub(crate) fn has_side_effects(&self) -> bool {
         match self {
             Intrinsic::AssertConstant
@@ -152,6 +154,39 @@ impl Intrinsic {
         }
     }
 
+    /// Intrinsics which only have a side effect due to the chance that
+    /// they can fail a constraint can be deduplicated.
+    pub(crate) fn can_be_deduplicated(&self, deduplicate_with_predicate: bool) -> bool {
+        match self {
+            // These apply a constraint in the form of ACIR opcodes, but they can be deduplicated
+            // if the inputs are the same. If they depend on a side effect variable (e.g. because
+            // they were in an if-then-else) then `handle_instruction_side_effects` in `flatten_cfg`
+            // will have attached the condition variable to their inputs directly, so they don't
+            // directly depend on the corresponding `enable_side_effect` instruction any more.
+            // However, to conform with the expectations of `Instruction::can_be_deduplicated` and
+            // `constant_folding` we only use this information if the caller shows interest in it.
+            Intrinsic::ToBits(_)
+            | Intrinsic::ToRadix(_)
+            | Intrinsic::BlackBox(
+                BlackBoxFunc::MultiScalarMul
+                | BlackBoxFunc::EmbeddedCurveAdd
+                | BlackBoxFunc::RecursiveAggregation,
+            ) => deduplicate_with_predicate,
+
+            // Operations that remove items from a slice don't modify the slice, they just assert it's non-empty.
+            Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceRemove => {
+                deduplicate_with_predicate
+            }
+
+            Intrinsic::AssertConstant
+            | Intrinsic::StaticAssert
+            | Intrinsic::ApplyRangeConstraint
+            | Intrinsic::AsWitness => deduplicate_with_predicate,
+
+            _ => !self.has_side_effects(),
+        }
+    }
+
     /// Lookup an Intrinsic by name and return it if found.
     /// If there is no such intrinsic by that name, None is returned.
     pub(crate) fn lookup(name: &str) -> Option<Intrinsic> {
@@ -245,7 +280,7 @@ pub(crate) enum Instruction {
     /// - `code1` will have side effects iff `condition1` evaluates to `true`
     ///
     /// This instruction is only emitted after the cfg flattening pass, and is used to annotate
-    /// instruction regions with an condition that corresponds to their position in the CFG's
+    /// instruction regions with a condition that corresponds to their position in the CFG's
     /// if-branching structure.
     EnableSideEffectsIf { condition: ValueId },
 
@@ -327,10 +362,53 @@ impl Instruction {
         matches!(self.result_type(), InstructionResultType::Unknown)
     }
 
+    /// Indicates if the instruction has a side effect, ie. it can fail, or it interacts with memory.
+    ///
+    /// This is similar to `can_be_deduplicated`, but it doesn't depend on whether the caller takes
+    /// constraints into account, because it might not use it to isolate the side effects across branches.
+    pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool {
+        use Instruction::*;
+
+        match self {
+            // These either have side-effects or interact with memory
+            EnableSideEffectsIf { .. }
+            | Allocate
+            | Load { .. }
+            | Store { .. }
+            | IncrementRc { .. }
+            | DecrementRc { .. } => true,
+
+            Call { func, .. } => match dfg[*func] {
+                Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(),
+                _ => true, // Be conservative and assume other functions can have side effects.
+            },
+
+            // These can fail.
+            Constrain(..) | RangeCheck { .. } => true,
+
+            // This should never be side-effectful
+            MakeArray { .. } => false,
+
+            // These can have different behavior depending on the EnableSideEffectsIf context.
+            Binary(_)
+            | Cast(_, _)
+            | Not(_)
+            | Truncate { .. }
+            | IfElse { .. }
+            | ArrayGet { .. }
+            | ArraySet { .. } => self.requires_acir_gen_predicate(dfg),
+        }
+    }
+
     /// Indicates if the instruction can be safely replaced with the results of another instruction with the same inputs.
     /// If `deduplicate_with_predicate` is set, we assume we're deduplicating with the instruction
     /// and its predicate, rather than just the instruction. Setting this means instructions that
     /// rely on predicates can be deduplicated as well.
+    ///
+    /// Some instructions get the predicate attached to their inputs by `handle_instruction_side_effects` in `flatten_cfg`.
+    /// These can be deduplicated because they implicitly depend on the predicate, not only when the caller uses the
+    /// predicate variable as a key to cache results. However, to avoid tight coupling between passes, we make the deduplication
+    /// conditional on whether the caller wants the predicate to be taken into account or not.
     pub(crate) fn can_be_deduplicated(
         &self,
         dfg: &DataFlowGraph,
@@ -348,7 +426,9 @@ impl Instruction {
             | DecrementRc { .. } => false,
 
             Call { func, .. } => match dfg[*func] {
-                Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(),
+                Value::Intrinsic(intrinsic) => {
+                    intrinsic.can_be_deduplicated(deduplicate_with_predicate)
+                }
                 _ => false,
             },
 
@@ -421,6 +501,7 @@ impl Instruction {
                 // Explicitly allows removal of unused ec operations, even if they can fail
                 Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul))
                 | Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => true,
+
                 Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(),
 
                 // All foreign functions are treated as having side effects.
@@ -436,7 +517,7 @@ impl Instruction {
         }
     }
 
-    /// If true the instruction will depends on enable_side_effects context during acir-gen
+    /// If true the instruction will depend on `enable_side_effects` context during acir-gen.
     pub(crate) fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool {
         match self {
             Instruction::Binary(binary)

compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs

@@ -45,17 +45,17 @@ pub(super) fn simplify_call(
         _ => return SimplifyResult::None,
     };
 
+    let return_type = ctrl_typevars.and_then(|return_types| return_types.first().cloned());
+
     let constant_args: Option<Vec<_>> =
         arguments.iter().map(|value_id| dfg.get_numeric_constant(*value_id)).collect();
 
-    match intrinsic {
+    let simplified_result = match intrinsic {
         Intrinsic::ToBits(endian) => {
             // TODO: simplify to a range constraint if `limb_count == 1`
-            if let (Some(constant_args), Some(return_type)) =
-                (constant_args, ctrl_typevars.map(|return_types| return_types.first().cloned()))
-            {
+            if let (Some(constant_args), Some(return_type)) = (constant_args, return_type.clone()) {
                 let field = constant_args[0];
-                let limb_count = if let Some(Type::Array(_, array_len)) = return_type {
+                let limb_count = if let Type::Array(_, array_len) = return_type {
                     array_len as u32
                 } else {
                     unreachable!("ICE: Intrinsic::ToRadix return type must be array")
@@ -67,12 +67,10 @@ pub(super) fn simplify_call(
         }
         Intrinsic::ToRadix(endian) => {
             // TODO: simplify to a range constraint if `limb_count == 1`
-            if let (Some(constant_args), Some(return_type)) =
-                (constant_args, ctrl_typevars.map(|return_types| return_types.first().cloned()))
-            {
+            if let (Some(constant_args), Some(return_type)) = (constant_args, return_type.clone()) {
                 let field = constant_args[0];
                 let radix = constant_args[1].to_u128() as u32;
-                let limb_count = if let Some(Type::Array(_, array_len)) = return_type {
+                let limb_count = if let Type::Array(_, array_len) = return_type {
                     array_len as u32
                 } else {
                     unreachable!("ICE: Intrinsic::ToRadix return type must be array")
@@ -330,7 +328,7 @@ pub(super) fn simplify_call(
         }
         Intrinsic::FromField => {
             let incoming_type = Type::field();
-            let target_type = ctrl_typevars.unwrap().remove(0);
+            let target_type = return_type.clone().unwrap();
 
             let truncate = Instruction::Truncate {
                 value: arguments[0],
@@ -352,8 +350,8 @@ pub(super) fn simplify_call(
         Intrinsic::AsWitness => SimplifyResult::None,
         Intrinsic::IsUnconstrained => SimplifyResult::None,
         Intrinsic::DerivePedersenGenerators => {
-            if let Some(Type::Array(_, len)) = ctrl_typevars.unwrap().first() {
-                simplify_derive_generators(dfg, arguments, *len as u32, block, call_stack)
+            if let Some(Type::Array(_, len)) = return_type.clone() {
+                simplify_derive_generators(dfg, arguments, len as u32, block, call_stack)
             } else {
                 unreachable!("Derive Pedersen Generators must return an array");
             }
@@ -370,7 +368,19 @@ pub(super) fn simplify_call(
         }
         Intrinsic::ArrayRefCount => SimplifyResult::None,
         Intrinsic::SliceRefCount => SimplifyResult::None,
+    };
+
+    if let (Some(expected_types), SimplifyResult::SimplifiedTo(result)) =
+        (return_type, &simplified_result)
+    {
+        assert_eq!(
+            dfg.type_of_value(*result),
+            expected_types,
+            "Simplification should not alter return type"
+        );
     }
+
+    simplified_result
 }
 
 /// Slices have a tuple structure (slice length, slice contents) to enable logic

compiler/noirc_evaluator/src/ssa/ir/instruction/call/blackbox.rs

@@ -48,7 +48,7 @@ pub(super) fn simplify_ec_add(
 
             let result_x = dfg.make_constant(result_x, Type::field());
             let result_y = dfg.make_constant(result_y, Type::field());
-            let result_is_infinity = dfg.make_constant(result_is_infinity, Type::bool());
+            let result_is_infinity = dfg.make_constant(result_is_infinity, Type::field());
 
             let typ = Type::Array(Arc::new(vec![Type::field()]), 3);
 
@@ -107,7 +107,7 @@ pub(super) fn simplify_msm(
 
             let result_x = dfg.make_constant(result_x, Type::field());
             let result_y = dfg.make_constant(result_y, Type::field());
-            let result_is_infinity = dfg.make_constant(result_is_infinity, Type::bool());
+            let result_is_infinity = dfg.make_constant(result_is_infinity, Type::field());
 
             let elements = im::vector![result_x, result_y, result_is_infinity];
             let typ = Type::Array(Arc::new(vec![Type::field()]), 3);