feat(perf): Remove redundant inc rc without instructions between (noi…

…r-lang/noir#6183)

chore: reexport `CrateName` through `nargo` (noir-lang/noir#6177)
feat(perf): Remove inc_rc instructions for arrays which are never mutably borrowed (noir-lang/noir#6168)
chore(docs): Add link to more info about proving backend to Solidity verifier page (noir-lang/noir#5754)
feat: let `Module::functions` and `Module::structs` return them in definition order (noir-lang/noir#6178)
chore: split `test_program`s into modules (noir-lang/noir#6101)
chore: remove `DefCollectorErrorKind::MacroError` (noir-lang/noir#6174)
feat(ssa): Simplify signed casts (noir-lang/noir#6166)
feat: visibility for modules (noir-lang/noir#6165)

.noir-sync-commit

@@ -1 +1 @@
-eec3a6152493e56866ec5338ff52f823c530778e
+be9dcfe56d808b1bd5ef552d41274705b2df7062

noir/noir-repo/compiler/noirc_driver/src/lib.rs

@@ -14,7 +14,6 @@ use noirc_evaluator::create_program;
 use noirc_evaluator::errors::RuntimeError;
 use noirc_evaluator::ssa::SsaProgramArtifact;
 use noirc_frontend::debug::build_debug_crate_file;
-use noirc_frontend::graph::{CrateId, CrateName};
 use noirc_frontend::hir::def_map::{Contract, CrateDefMap};
 use noirc_frontend::hir::Context;
 use noirc_frontend::monomorphization::{
@@ -35,6 +34,7 @@ use debug::filter_relevant_files;
 
 pub use contract::{CompiledContract, CompiledContractOutputs, ContractFunction};
 pub use debug::DebugFile;
+pub use noirc_frontend::graph::{CrateId, CrateName};
 pub use program::CompiledProgram;
 
 const STD_CRATE_NAME: &str = "std";

noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs

@@ -106,15 +106,7 @@ impl Context {
 
         let instructions_len = block.instructions().len();
 
-        // We can track IncrementRc instructions per block to determine whether they are useless.
-        // IncrementRc and DecrementRc instructions are normally side effectual instructions, but we remove
-        // them if their value is not used anywhere in the function. However, even when their value is used, their existence
-        // is pointless logic if there is no array set between the increment and the decrement of the reference counter.
-        // We track per block whether an IncrementRc instruction has a paired DecrementRc instruction
-        // with the same value but no array set in between.
-        // If we see an inc/dec RC pair within a block we can safely remove both instructions.
-        let mut inc_rcs: HashMap<Type, Vec<RcInstruction>> = HashMap::default();
-        let mut inc_rcs_to_remove = HashSet::default();
+        let mut rc_tracker = RcTracker::default();
 
         // Indexes of instructions that might be out of bounds.
         // We'll remove those, but before that we'll insert bounds checks for them.
@@ -143,17 +135,11 @@ impl Context {
                 }
             }
 
-            self.track_inc_rcs_to_remove(
-                *instruction_id,
-                function,
-                &mut inc_rcs,
-                &mut inc_rcs_to_remove,
-            );
+            rc_tracker.track_inc_rcs_to_remove(*instruction_id, function);
         }
 
-        for id in inc_rcs_to_remove {
-            self.instructions_to_remove.insert(id);
-        }
+        self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays());
+        self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove);
 
         // If there are some instructions that might trigger an out of bounds error,
         // first add constrain checks. Then run the DIE pass again, which will remove those
@@ -568,10 +554,109 @@ fn apply_side_effects(
     (lhs, rhs)
 }
 
+#[derive(Default)]
+struct RcTracker {
+    // We can track IncrementRc instructions per block to determine whether they are useless.
+    // IncrementRc and DecrementRc instructions are normally side effectual instructions, but we remove
+    // them if their value is not used anywhere in the function. However, even when their value is used, their existence
+    // is pointless logic if there is no array set between the increment and the decrement of the reference counter.
+    // We track per block whether an IncrementRc instruction has a paired DecrementRc instruction
+    // with the same value but no array set in between.
+    // If we see an inc/dec RC pair within a block we can safely remove both instructions.
+    rcs_with_possible_pairs: HashMap<Type, Vec<RcInstruction>>,
+    rc_pairs_to_remove: HashSet<InstructionId>,
+    // We also separately track all IncrementRc instructions and all arrays which have been mutably borrowed.
+    // If an array has not been mutably borrowed we can then safely remove all IncrementRc instructions on that array.
+    inc_rcs: HashMap<ValueId, HashSet<InstructionId>>,
+    mut_borrowed_arrays: HashSet<ValueId>,
+    // The SSA often creates patterns where after simplifications we end up with repeat
+    // IncrementRc instructions on the same value. We track whether the previous instruction was an IncrementRc,
+    // and if the current instruction is also an IncrementRc on the same value we remove the current instruction.
+    // `None` if the previous instruction was anything other than an IncrementRc
+    previous_inc_rc: Option<ValueId>,
+}
+
+impl RcTracker {
+    fn track_inc_rcs_to_remove(&mut self, instruction_id: InstructionId, function: &Function) {
+        let instruction = &function.dfg[instruction_id];
+
+        if let Instruction::IncrementRc { value } = instruction {
+            if let Some(previous_value) = self.previous_inc_rc {
+                if previous_value == *value {
+                    self.rc_pairs_to_remove.insert(instruction_id);
+                }
+            }
+            self.previous_inc_rc = Some(*value);
+        } else {
+            self.previous_inc_rc = None;
+        }
+
+        // DIE loops over a block in reverse order, so we insert an RC instruction for possible removal
+        // when we see a DecrementRc and check whether it was possibly mutated when we see an IncrementRc.
+        match instruction {
+            Instruction::IncrementRc { value } => {
+                if let Some(inc_rc) =
+                    pop_rc_for(*value, function, &mut self.rcs_with_possible_pairs)
+                {
+                    if !inc_rc.possibly_mutated {
+                        self.rc_pairs_to_remove.insert(inc_rc.id);
+                        self.rc_pairs_to_remove.insert(instruction_id);
+                    }
+                }
+
+                self.inc_rcs.entry(*value).or_default().insert(instruction_id);
+            }
+            Instruction::DecrementRc { value } => {
+                let typ = function.dfg.type_of_value(*value);
+
+                // We assume arrays aren't mutated until we find an array_set
+                let dec_rc =
+                    RcInstruction { id: instruction_id, array: *value, possibly_mutated: false };
+                self.rcs_with_possible_pairs.entry(typ).or_default().push(dec_rc);
+            }
+            Instruction::ArraySet { array, .. } => {
+                let typ = function.dfg.type_of_value(*array);
+                if let Some(dec_rcs) = self.rcs_with_possible_pairs.get_mut(&typ) {
+                    for dec_rc in dec_rcs {
+                        dec_rc.possibly_mutated = true;
+                    }
+                }
+
+                self.mut_borrowed_arrays.insert(*array);
+            }
+            Instruction::Store { value, .. } => {
+                // We are very conservative and say that any store of an array value means it has the potential
+                // to be mutated. This is done due to the tracking of mutable borrows still being per block.
+                let typ = function.dfg.type_of_value(*value);
+                if matches!(&typ, Type::Array(..) | Type::Slice(..)) {
+                    self.mut_borrowed_arrays.insert(*value);
+                }
+            }
+            _ => {}
+        }
+    }
+
+    fn get_non_mutated_arrays(&self) -> HashSet<InstructionId> {
+        self.inc_rcs
+            .keys()
+            .filter_map(|value| {
+                if !self.mut_borrowed_arrays.contains(value) {
+                    Some(&self.inc_rcs[value])
+                } else {
+                    None
+                }
+            })
+            .flatten()
+            .copied()
+            .collect()
+    }
+}
 #[cfg(test)]
 mod test {
     use std::sync::Arc;
 
+    use im::vector;
+
     use crate::ssa::{
         function_builder::FunctionBuilder,
         ir::{
@@ -779,4 +864,160 @@ mod test {
 
         assert_eq!(main.dfg[main.entry_block()].instructions().len(), 3);
     }
+
+    #[test]
+    fn keep_inc_rc_on_borrowed_array_store() {
+        // acir(inline) fn main f0 {
+        //     b0():
+        //       v2 = allocate
+        //       inc_rc [u32 0, u32 0]
+        //       store [u32 0, u32 0] at v2
+        //       inc_rc [u32 0, u32 0]
+        //       jmp b1()
+        //     b1():
+        //       v3 = load v2
+        //       v5 = array_set v3, index u32 0, value u32 1
+        //       return v5
+        //   }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+        let zero = builder.numeric_constant(0u128, Type::unsigned(32));
+        let array_type = Type::Array(Arc::new(vec![Type::unsigned(32)]), 2);
+        let array = builder.array_constant(vector![zero, zero], array_type.clone());
+        let v2 = builder.insert_allocate(array_type.clone());
+        builder.increment_array_reference_count(array);
+        builder.insert_store(v2, array);
+        builder.increment_array_reference_count(array);
+
+        let b1 = builder.insert_block();
+        builder.terminate_with_jmp(b1, vec![]);
+        builder.switch_to_block(b1);
+
+        let v3 = builder.insert_load(v2, array_type);
+        let one = builder.numeric_constant(1u128, Type::unsigned(32));
+        let v5 = builder.insert_array_set(v3, zero, one);
+        builder.terminate_with_return(vec![v5]);
+
+        let ssa = builder.finish();
+        let main = ssa.main();
+
+        // The instruction count never includes the terminator instruction
+        assert_eq!(main.dfg[main.entry_block()].instructions().len(), 4);
+        assert_eq!(main.dfg[b1].instructions().len(), 2);
+
+        // We expect the output to be unchanged
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+
+        assert_eq!(main.dfg[main.entry_block()].instructions().len(), 4);
+        assert_eq!(main.dfg[b1].instructions().len(), 2);
+    }
+
+    #[test]
+    fn keep_inc_rc_on_borrowed_array_set() {
+        // acir(inline) fn main f0 {
+        //     b0(v0: [u32; 2]):
+        //       inc_rc v0
+        //       v3 = array_set v0, index u32 0, value u32 1
+        //       inc_rc v0
+        //       inc_rc v0
+        //       inc_rc v0
+        //       v4 = array_get v3, index u32 1
+        //       return v4
+        //   }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+        let array_type = Type::Array(Arc::new(vec![Type::unsigned(32)]), 2);
+        let v0 = builder.add_parameter(array_type.clone());
+        builder.increment_array_reference_count(v0);
+        let zero = builder.numeric_constant(0u128, Type::unsigned(32));
+        let one = builder.numeric_constant(1u128, Type::unsigned(32));
+        let v3 = builder.insert_array_set(v0, zero, one);
+        builder.increment_array_reference_count(v0);
+        builder.increment_array_reference_count(v0);
+        builder.increment_array_reference_count(v0);
+
+        let v4 = builder.insert_array_get(v3, one, Type::unsigned(32));
+
+        builder.terminate_with_return(vec![v4]);
+
+        let ssa = builder.finish();
+        let main = ssa.main();
+
+        // The instruction count never includes the terminator instruction
+        assert_eq!(main.dfg[main.entry_block()].instructions().len(), 6);
+
+        // We expect the output to be unchanged
+        // Expected output:
+        //
+        // acir(inline) fn main f0 {
+        //     b0(v0: [u32; 2]):
+        //       inc_rc v0
+        //       v3 = array_set v0, index u32 0, value u32 1
+        //       inc_rc v0
+        //       v4 = array_get v3, index u32 1
+        //       return v4
+        //   }
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+
+        let instructions = main.dfg[main.entry_block()].instructions();
+        // We expect only the repeated inc_rc instructions to be collapsed into a single inc_rc.
+        assert_eq!(instructions.len(), 4);
+
+        assert!(matches!(&main.dfg[instructions[0]], Instruction::IncrementRc { .. }));
+        assert!(matches!(&main.dfg[instructions[1]], Instruction::ArraySet { .. }));
+        assert!(matches!(&main.dfg[instructions[2]], Instruction::IncrementRc { .. }));
+        assert!(matches!(&main.dfg[instructions[3]], Instruction::ArrayGet { .. }));
+    }
+
+    #[test]
+    fn remove_inc_rcs_that_are_never_mutably_borrowed() {
+        // acir(inline) fn main f0 {
+        //     b0(v0: [Field; 2]):
+        //       inc_rc v0
+        //       inc_rc v0
+        //       inc_rc v0
+        //       v2 = array_get v0, index u32 0
+        //       inc_rc v0
+        //       return v2
+        //   }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+        let v0 = builder.add_parameter(Type::Array(Arc::new(vec![Type::field()]), 2));
+        builder.increment_array_reference_count(v0);
+        builder.increment_array_reference_count(v0);
+        builder.increment_array_reference_count(v0);
+
+        let zero = builder.numeric_constant(0u128, Type::unsigned(32));
+        let v2 = builder.insert_array_get(v0, zero, Type::field());
+        builder.increment_array_reference_count(v0);
+        builder.terminate_with_return(vec![v2]);
+
+        let ssa = builder.finish();
+        let main = ssa.main();
+
+        // The instruction count never includes the terminator instruction
+        assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5);
+
+        // Expected output:
+        //
+        // acir(inline) fn main f0 {
+        //     b0(v0: [Field; 2]):
+        //       v2 = array_get v0, index u32 0
+        //       return v2
+        //   }
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+
+        let instructions = main.dfg[main.entry_block()].instructions();
+        assert_eq!(instructions.len(), 1);
+        assert!(matches!(&main.dfg[instructions[0]], Instruction::ArrayGet { .. }));
+    }
 }

noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs

@@ -2458,10 +2458,12 @@ fn module_functions(
     let module_id = get_module(self_argument)?;
     let module_data = interpreter.elaborator.get_module(module_id);
     let func_ids = module_data
-        .value_definitions()
+        .definitions()
+        .definitions()
+        .iter()
         .filter_map(|module_def_id| {
             if let ModuleDefId::FunctionId(func_id) = module_def_id {
-                Some(Value::FunctionDefinition(func_id))
+                Some(Value::FunctionDefinition(*func_id))
             } else {
                 None
             }
@@ -2482,10 +2484,12 @@ fn module_structs(
     let module_id = get_module(self_argument)?;
     let module_data = interpreter.elaborator.get_module(module_id);
     let struct_ids = module_data
-        .type_definitions()
+        .definitions()
+        .definitions()
+        .iter()
         .filter_map(|module_def_id| {
             if let ModuleDefId::TypeId(id) = module_def_id {
-                Some(Value::StructDefinition(id))
+                Some(Value::StructDefinition(*id))
             } else {
                 None
             }

noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/errors.rs

@@ -71,8 +71,6 @@ pub enum DefCollectorErrorKind {
         "Either the type or the trait must be from the same crate as the trait implementation"
     )]
     TraitImplOrphaned { span: Span },
-    #[error("macro error : {0:?}")]
-    MacroError(MacroError),
     #[error("The only supported types of numeric generics are integers, fields, and booleans")]
     UnsupportedNumericGenericType { ident: Ident, typ: UnresolvedTypeData },
     #[error("impl has stricter requirements than trait")]
@@ -86,14 +84,6 @@ pub enum DefCollectorErrorKind {
     },
 }
 
-/// An error struct that macro processors can return.
-#[derive(Debug, Clone)]
-pub struct MacroError {
-    pub primary_message: String,
-    pub secondary_message: Option<String>,
-    pub span: Option<Span>,
-}
-
 impl DefCollectorErrorKind {
     pub fn into_file_diagnostic(&self, file: fm::FileId) -> FileDiagnostic {
         Diagnostic::from(self).in_file(file)
@@ -276,9 +266,6 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic {
                 "Either the type or the trait must be from the same crate as the trait implementation".into(),
                 *span,
             ),
-            DefCollectorErrorKind::MacroError(macro_error) => {
-                Diagnostic::simple_error(macro_error.primary_message.clone(), macro_error.secondary_message.clone().unwrap_or_default(), macro_error.span.unwrap_or_default())
-            },
             DefCollectorErrorKind::UnsupportedNumericGenericType { ident, typ } => {
                 let name = &ident.0.contents;