Merge branch 'master' into 6444-fix-slice-dynamic-index

acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs

@@ -1,7 +1,12 @@
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 
 use acir::{
-    circuit::{brillig::BrilligInputs, directives::Directive, opcodes::BlockId, Circuit, Opcode},
+    circuit::{
+        brillig::{BrilligInputs, BrilligOutputs},
+        directives::Directive,
+        opcodes::BlockId,
+        Circuit, Opcode,
+    },
     native_types::{Expression, Witness},
     AcirField,
 };
@@ -72,23 +77,31 @@ impl MergeExpressionsOptimizer {
                     if let (Opcode::AssertZero(expr_define), Opcode::AssertZero(expr_use)) =
                         (opcode.clone(), second_gate)
                     {
-                        if let Some(expr) = Self::merge(&expr_use, &expr_define, w) {
-                            // sanity check
-                            assert!(i < b);
-                            modified_gates.insert(b, Opcode::AssertZero(expr));
-                            to_keep = false;
-                            // Update the 'used_witness' map to account for the merge.
-                            for w2 in CircuitSimulator::expr_wit(&expr_define) {
-                                if !circuit_inputs.contains(&w2) {
-                                    let mut v = used_witness[&w2].clone();
-                                    v.insert(b);
-                                    v.remove(&i);
-                                    used_witness.insert(w2, v);
+                        // We cannot merge an expression into an earlier opcode, because this
+                        // would break the 'execution ordering' of the opcodes
+                        // This case can happen because a previous merge would change an opcode
+                        // and eliminate a witness from it, giving new opportunities for this
+                        // witness to be used in only two expressions
+                        // TODO: the missed optimization for the i>b case can be handled by
+                        // - doing this pass again until there is no change, or
+                        // - merging 'b' into 'i' instead
+                        if i < b {
+                            if let Some(expr) = Self::merge(&expr_use, &expr_define, w) {
+                                modified_gates.insert(b, Opcode::AssertZero(expr));
+                                to_keep = false;
+                                // Update the 'used_witness' map to account for the merge.
+                                for w2 in CircuitSimulator::expr_wit(&expr_define) {
+                                    if !circuit_inputs.contains(&w2) {
+                                        let mut v = used_witness[&w2].clone();
+                                        v.insert(b);
+                                        v.remove(&i);
+                                        used_witness.insert(w2, v);
+                                    }
                                 }
+                                // We need to stop here and continue with the next opcode
+                                // because the merge invalidates the current opcode.
+                                break;
                             }
-                            // We need to stop here and continue with the next opcode
-                            // because the merge invalidate the current opcode
-                            break;
                         }
                     }
                 }
@@ -125,6 +138,19 @@ impl MergeExpressionsOptimizer {
         result
     }
 
+    fn brillig_output_wit(&self, output: &BrilligOutputs) -> BTreeSet<Witness> {
+        let mut result = BTreeSet::new();
+        match output {
+            BrilligOutputs::Simple(witness) => {
+                result.insert(*witness);
+            }
+            BrilligOutputs::Array(witnesses) => {
+                result.extend(witnesses);
+            }
+        }
+        result
+    }
+
     // Returns the input witnesses used by the opcode
     fn witness_inputs<F: AcirField>(&self, opcode: &Opcode<F>) -> BTreeSet<Witness> {
         let mut witnesses = BTreeSet::new();
@@ -146,16 +172,22 @@ impl MergeExpressionsOptimizer {
             Opcode::MemoryInit { block_id: _, init, block_type: _ } => {
                 init.iter().cloned().collect()
             }
-            Opcode::BrilligCall { inputs, .. } => {
+            Opcode::BrilligCall { inputs, outputs, .. } => {
                 for i in inputs {
                     witnesses.extend(self.brillig_input_wit(i));
                 }
+                for i in outputs {
+                    witnesses.extend(self.brillig_output_wit(i));
+                }
                 witnesses
             }
-            Opcode::Call { id: _, inputs, outputs: _, predicate } => {
+            Opcode::Call { id: _, inputs, outputs, predicate } => {
                 for i in inputs {
                     witnesses.insert(*i);
                 }
+                for i in outputs {
+                    witnesses.insert(*i);
+                }
                 if let Some(p) = predicate {
                     witnesses.extend(CircuitSimulator::expr_wit(p));
                 }
@@ -195,3 +227,129 @@ impl MergeExpressionsOptimizer {
         None
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::compiler::{optimizers::MergeExpressionsOptimizer, CircuitSimulator};
+    use acir::{
+        acir_field::AcirField,
+        circuit::{
+            brillig::{BrilligFunctionId, BrilligOutputs},
+            opcodes::FunctionInput,
+            Circuit, ExpressionWidth, Opcode, PublicInputs,
+        },
+        native_types::{Expression, Witness},
+        FieldElement,
+    };
+    use std::collections::BTreeSet;
+
+    fn check_circuit(circuit: Circuit<FieldElement>) {
+        assert!(CircuitSimulator::default().check_circuit(&circuit));
+        let mut merge_optimizer = MergeExpressionsOptimizer::new();
+        let acir_opcode_positions = vec![0; 20];
+        let (opcodes, _) =
+            merge_optimizer.eliminate_intermediate_variable(&circuit, acir_opcode_positions);
+        let mut optimized_circuit = circuit;
+        optimized_circuit.opcodes = opcodes;
+        // check that the circuit is still valid after optimization
+        assert!(CircuitSimulator::default().check_circuit(&optimized_circuit));
+    }
+
+    #[test]
+    fn does_not_eliminate_witnesses_returned_from_brillig() {
+        let opcodes = vec![
+            Opcode::BrilligCall {
+                id: BrilligFunctionId::default(),
+                inputs: Vec::new(),
+                outputs: vec![BrilligOutputs::Simple(Witness(1))],
+                predicate: None,
+            },
+            Opcode::AssertZero(Expression {
+                mul_terms: Vec::new(),
+                linear_combinations: vec![
+                    (FieldElement::from(2_u128), Witness(0)),
+                    (FieldElement::from(3_u128), Witness(1)),
+                    (FieldElement::from(1_u128), Witness(2)),
+                ],
+                q_c: FieldElement::one(),
+            }),
+            Opcode::AssertZero(Expression {
+                mul_terms: Vec::new(),
+                linear_combinations: vec![
+                    (FieldElement::from(2_u128), Witness(0)),
+                    (FieldElement::from(2_u128), Witness(1)),
+                    (FieldElement::from(1_u128), Witness(5)),
+                ],
+                q_c: FieldElement::one(),
+            }),
+        ];
+
+        let mut private_parameters = BTreeSet::new();
+        private_parameters.insert(Witness(0));
+
+        let circuit = Circuit {
+            current_witness_index: 1,
+            expression_width: ExpressionWidth::Bounded { width: 4 },
+            opcodes,
+            private_parameters,
+            public_parameters: PublicInputs::default(),
+            return_values: PublicInputs::default(),
+            assert_messages: Default::default(),
+            recursive: false,
+        };
+        check_circuit(circuit);
+    }
+
+    #[test]
+    fn does_not_attempt_to_merge_into_previous_opcodes() {
+        let opcodes = vec![
+            Opcode::AssertZero(Expression {
+                mul_terms: vec![(FieldElement::one(), Witness(0), Witness(0))],
+                linear_combinations: vec![(-FieldElement::one(), Witness(4))],
+                q_c: FieldElement::zero(),
+            }),
+            Opcode::AssertZero(Expression {
+                mul_terms: vec![(FieldElement::one(), Witness(0), Witness(1))],
+                linear_combinations: vec![(FieldElement::one(), Witness(5))],
+                q_c: FieldElement::zero(),
+            }),
+            Opcode::AssertZero(Expression {
+                mul_terms: Vec::new(),
+                linear_combinations: vec![
+                    (-FieldElement::one(), Witness(2)),
+                    (FieldElement::one(), Witness(4)),
+                    (FieldElement::one(), Witness(5)),
+                ],
+                q_c: FieldElement::zero(),
+            }),
+            Opcode::AssertZero(Expression {
+                mul_terms: Vec::new(),
+                linear_combinations: vec![
+                    (FieldElement::one(), Witness(2)),
+                    (-FieldElement::one(), Witness(3)),
+                    (FieldElement::one(), Witness(4)),
+                    (FieldElement::one(), Witness(5)),
+                ],
+                q_c: FieldElement::zero(),
+            }),
+            Opcode::BlackBoxFuncCall(acir::circuit::opcodes::BlackBoxFuncCall::RANGE {
+                input: FunctionInput::witness(Witness(3), 32),
+            }),
+        ];
+
+        let mut private_parameters = BTreeSet::new();
+        private_parameters.insert(Witness(0));
+        private_parameters.insert(Witness(1));
+        let circuit = Circuit {
+            current_witness_index: 5,
+            expression_width: ExpressionWidth::Bounded { width: 4 },
+            opcodes,
+            private_parameters,
+            public_parameters: PublicInputs::default(),
+            return_values: PublicInputs::default(),
+            assert_messages: Default::default(),
+            recursive: false,
+        };
+        check_circuit(circuit);
+    }
+}

compiler/noirc_frontend/Cargo.toml

@@ -36,6 +36,8 @@ strum_macros = "0.24"
 
 [dev-dependencies]
 base64.workspace = true
+proptest.workspace = true
+proptest-derive = "0.5.0"
 
 [features]
 experimental_parser = []

compiler/noirc_frontend/proptest-regressions/tests/arithmetic_generics.txt

@@ -0,0 +1,7 @@
+# Seeds for failure cases proptest has generated in the past. It is
+# automatically read and these particular cases re-run before any
+# novel cases are generated.
+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc fc27f4091dfa5f938973048209b5fcf22aefa1cfaffaaa3e349f30e9b1f93f49 # shrinks to infix_and_bindings = (((0: numeric bool) % (Numeric(Shared(RefCell { value: Unbound('2, Numeric(bool)) }): bool) + Numeric(Shared(RefCell { value: Unbound('0, Numeric(bool)) }): bool))), [('0, (0: numeric bool)), ('1, (0: numeric bool)), ('2, (0: numeric bool))])

compiler/noirc_frontend/src/ast/expression.rs

@@ -98,7 +98,7 @@ impl UnresolvedGeneric {
             UnresolvedGeneric::Variable(_) => Ok(Kind::Normal),
             UnresolvedGeneric::Numeric { typ, .. } => {
                 let typ = self.resolve_numeric_kind_type(typ)?;
-                Ok(Kind::Numeric(Box::new(typ)))
+                Ok(Kind::numeric(typ))
             }
             UnresolvedGeneric::Resolved(..) => {
                 panic!("Don't know the kind of a resolved generic here")

compiler/noirc_frontend/src/ast/mod.rs

@@ -19,6 +19,9 @@ pub use visitor::Visitor;
 pub use expression::*;
 pub use function::*;
 
+#[cfg(test)]
+use proptest_derive::Arbitrary;
+
 use acvm::FieldElement;
 pub use docs::*;
 use noirc_errors::Span;
@@ -37,6 +40,7 @@ use crate::{
 use acvm::acir::AcirField;
 use iter_extended::vecmap;
 
+#[cfg_attr(test, derive(Arbitrary))]
 #[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, Ord, PartialOrd)]
 pub enum IntegerBitSize {
     One,

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -641,7 +641,7 @@ impl<'context> Elaborator<'context> {
             let typ = if unresolved_typ.is_type_expression() {
                 self.resolve_type_inner(
                     unresolved_typ.clone(),
-                    &Kind::Numeric(Box::new(Type::default_int_type())),
+                    &Kind::numeric(Type::default_int_type()),
                 )
             } else {
                 self.resolve_type(unresolved_typ.clone())
@@ -654,7 +654,7 @@ impl<'context> Elaborator<'context> {
                     });
                 self.push_err(unsupported_typ_err);
             }
-            Kind::Numeric(Box::new(typ))
+            Kind::numeric(typ)
         } else {
             Kind::Normal
         }

compiler/noirc_frontend/src/elaborator/types.rs

@@ -414,7 +414,7 @@ impl<'context> Elaborator<'context> {
                 let kind = self
                     .interner
                     .get_global_let_statement(id)
-                    .map(|let_statement| Kind::Numeric(Box::new(let_statement.r#type)))
+                    .map(|let_statement| Kind::numeric(let_statement.r#type))
                     .unwrap_or(Kind::u32());
 
                 // TODO(https://github.com/noir-lang/noir/issues/6238):

compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -541,7 +541,7 @@ impl<'a> ModCollector<'a> {
                                 name: Rc::new(name.to_string()),
                                 type_var: TypeVariable::unbound(
                                     type_variable_id,
-                                    Kind::Numeric(Box::new(typ)),
+                                    Kind::numeric(typ),
                                 ),
                                 span: name.span(),
                             });