[1 changes] fix: prevent no_predicates from removing predicates in …

…calling function (noir-lang/noir#5452)

feat: lsp rename/find-all-references for globals (noir-lang/noir#5415)
feat: remove redundant `EnableSideEffects` instructions (noir-lang/noir#5440)

.noir-sync-commit

@@ -1 +1 @@
-30c50f52a6d58163e39006b73f4eb5003afc239b
+66244b6e5b505f692c7e9a41bdc061c77fd1284d

noir/noir-repo/.github/workflows/mirror-external_libs.yml

@@ -0,0 +1,8 @@
+name: Mirror Repositories
+on:
+  workflow_dispatch: {} 
+jobs:
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo Dummy workflow TODO

noir/noir-repo/acvm-repo/acvm/src/pwg/brillig.rs

@@ -162,63 +162,27 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
             VMStatus::Finished { .. } => Ok(BrilligSolverStatus::Finished),
             VMStatus::InProgress => Ok(BrilligSolverStatus::InProgress),
             VMStatus::Failure { reason, call_stack } => {
+                let call_stack = call_stack
+                    .iter()
+                    .map(|brillig_index| OpcodeLocation::Brillig {
+                        acir_index: self.acir_index,
+                        brillig_index: *brillig_index,
+                    })
+                    .collect();
                 let payload = match reason {
                     FailureReason::RuntimeError { message } => {
                         Some(ResolvedAssertionPayload::String(message))
                     }
                     FailureReason::Trap { revert_data_offset, revert_data_size } => {
-                        // Since noir can only revert with strings currently, we can parse return data as a string
-                        if revert_data_size == 0 {
-                            None
-                        } else {
-                            let memory = self.vm.get_memory();
-                            let mut revert_values_iter = memory
-                                [revert_data_offset..(revert_data_offset + revert_data_size)]
-                                .iter();
-                            let error_selector = ErrorSelector::new(
-                                revert_values_iter
-                                    .next()
-                                    .expect("Incorrect revert data size")
-                                    .try_into()
-                                    .expect("Error selector is not u64"),
-                            );
-
-                            match error_selector {
-                                STRING_ERROR_SELECTOR => {
-                                    // If the error selector is 0, it means the error is a string
-                                    let string = revert_values_iter
-                                        .map(|memory_value| {
-                                            let as_u8: u8 = memory_value
-                                                .try_into()
-                                                .expect("String item is not u8");
-                                            as_u8 as char
-                                        })
-                                        .collect();
-                                    Some(ResolvedAssertionPayload::String(string))
-                                }
-                                _ => {
-                                    // If the error selector is not 0, it means the error is a custom error
-                                    Some(ResolvedAssertionPayload::Raw(RawAssertionPayload {
-                                        selector: error_selector,
-                                        data: revert_values_iter
-                                            .map(|value| value.to_field())
-                                            .collect(),
-                                    }))
-                                }
-                            }
-                        }
+                        extract_failure_payload_from_memory(
+                            self.vm.get_memory(),
+                            revert_data_offset,
+                            revert_data_size,
+                        )
                     }
                 };
-                Err(OpcodeResolutionError::BrilligFunctionFailed {
-                    payload,
-                    call_stack: call_stack
-                        .iter()
-                        .map(|brillig_index| OpcodeLocation::Brillig {
-                            acir_index: self.acir_index,
-                            brillig_index: *brillig_index,
-                        })
-                        .collect(),
-                })
+
+                Err(OpcodeResolutionError::BrilligFunctionFailed { payload, call_stack })
             }
             VMStatus::ForeignCallWait { function, inputs } => {
                 Ok(BrilligSolverStatus::ForeignCallWait(ForeignCallWaitInfo { function, inputs }))
@@ -283,6 +247,50 @@ impl<'b, B: BlackBoxFunctionSolver<F>, F: AcirField> BrilligSolver<'b, F, B> {
     }
 }
 
+/// Extracts a `ResolvedAssertionPayload` from a block of memory of a Brillig VM instance.
+///
+/// Returns `None` if the amount of memory requested is zero.
+fn extract_failure_payload_from_memory<F: AcirField>(
+    memory: &[MemoryValue<F>],
+    revert_data_offset: usize,
+    revert_data_size: usize,
+) -> Option<ResolvedAssertionPayload<F>> {
+    // Since noir can only revert with strings currently, we can parse return data as a string
+    if revert_data_size == 0 {
+        None
+    } else {
+        let mut revert_values_iter =
+            memory[revert_data_offset..(revert_data_offset + revert_data_size)].iter();
+        let error_selector = ErrorSelector::new(
+            revert_values_iter
+                .next()
+                .expect("Incorrect revert data size")
+                .try_into()
+                .expect("Error selector is not u64"),
+        );
+
+        match error_selector {
+            STRING_ERROR_SELECTOR => {
+                // If the error selector is 0, it means the error is a string
+                let string = revert_values_iter
+                    .map(|memory_value| {
+                        let as_u8: u8 = memory_value.try_into().expect("String item is not u8");
+                        as_u8 as char
+                    })
+                    .collect();
+                Some(ResolvedAssertionPayload::String(string))
+            }
+            _ => {
+                // If the error selector is not 0, it means the error is a custom error
+                Some(ResolvedAssertionPayload::Raw(RawAssertionPayload {
+                    selector: error_selector,
+                    data: revert_values_iter.map(|value| value.to_field()).collect(),
+                }))
+            }
+        }
+    }
+}
+
 /// Encapsulates a request from a Brillig VM process that encounters a [foreign call opcode][acir::brillig_vm::Opcode::ForeignCall]
 /// where the result of the foreign call has not yet been provided.
 ///

noir/noir-repo/acvm-repo/acvm_js/build.sh

@@ -25,7 +25,7 @@ function run_if_available {
 require_command jq
 require_command cargo
 require_command wasm-bindgen
-#require_command wasm-opt
+require_command wasm-opt
 
 self_path=$(dirname "$(readlink -f "$0")")
 pname=$(cargo read-manifest | jq -r '.name')

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

@@ -36,6 +36,7 @@ use self::{
 };
 
 mod acir_gen;
+mod checks;
 pub(super) mod function_builder;
 pub mod ir;
 mod opt;

noir/noir-repo/compiler/noirc_evaluator/src/ssa/checks/mod.rs

@@ -0,0 +1 @@
+mod check_for_underconstrained_values;

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

@@ -45,23 +45,23 @@ impl Ssa {
     /// This step should run after runtime separation, since it relies on the runtime of the called functions being final.
     #[tracing::instrument(level = "trace", skip(self))]
     pub(crate) fn inline_functions(self) -> Ssa {
-        Self::inline_functions_inner(self, true)
+        Self::inline_functions_inner(self, false)
     }
 
     // Run the inlining pass where functions marked with `InlineType::NoPredicates` as not entry points
     pub(crate) fn inline_functions_with_no_predicates(self) -> Ssa {
-        Self::inline_functions_inner(self, false)
+        Self::inline_functions_inner(self, true)
     }
 
-    fn inline_functions_inner(mut self, no_predicates_is_entry_point: bool) -> Ssa {
+    fn inline_functions_inner(mut self, inline_no_predicates_functions: bool) -> Ssa {
         let recursive_functions = find_all_recursive_functions(&self);
         self.functions = btree_map(
-            get_functions_to_inline_into(&self, no_predicates_is_entry_point),
+            get_functions_to_inline_into(&self, inline_no_predicates_functions),
             |entry_point| {
                 let new_function = InlineContext::new(
                     &self,
                     entry_point,
-                    no_predicates_is_entry_point,
+                    inline_no_predicates_functions,
                     recursive_functions.clone(),
                 )
                 .inline_all(&self);
@@ -86,7 +86,13 @@ struct InlineContext {
     // The FunctionId of the entry point function we're inlining into in the old, unmodified Ssa.
     entry_point: FunctionId,
 
-    no_predicates_is_entry_point: bool,
+    /// Whether the inlining pass should inline any functions marked with [`InlineType::NoPredicates`]
+    /// or whether these should be preserved as entrypoint functions.
+    ///
+    /// This is done as we delay inlining of functions with the attribute `#[no_predicates]` until after
+    /// the control flow graph has been flattened.
+    inline_no_predicates_functions: bool,
+
     // We keep track of the recursive functions in the SSA to avoid inlining them in a brillig context.
     recursive_functions: BTreeSet<FunctionId>,
 }
@@ -179,7 +185,7 @@ fn find_all_recursive_functions(ssa: &Ssa) -> BTreeSet<FunctionId> {
 ///  - Any Acir functions with a [fold inline type][InlineType::Fold],
 fn get_functions_to_inline_into(
     ssa: &Ssa,
-    no_predicates_is_entry_point: bool,
+    inline_no_predicates_functions: bool,
 ) -> BTreeSet<FunctionId> {
     let mut brillig_entry_points = BTreeSet::default();
     let mut acir_entry_points = BTreeSet::default();
@@ -190,10 +196,9 @@ fn get_functions_to_inline_into(
         }
 
         // If we have not already finished the flattening pass, functions marked
-        // to not have predicates should be marked as entry points.
-        let no_predicates_is_entry_point =
-            no_predicates_is_entry_point && function.is_no_predicates();
-        if function.runtime().is_entry_point() || no_predicates_is_entry_point {
+        // to not have predicates should be preserved.
+        let preserve_function = !inline_no_predicates_functions && function.is_no_predicates();
+        if function.runtime().is_entry_point() || preserve_function {
             acir_entry_points.insert(*func_id);
         }
 
@@ -228,7 +233,7 @@ impl InlineContext {
     fn new(
         ssa: &Ssa,
         entry_point: FunctionId,
-        no_predicates_is_entry_point: bool,
+        inline_no_predicates_functions: bool,
         recursive_functions: BTreeSet<FunctionId>,
     ) -> InlineContext {
         let source = &ssa.functions[&entry_point];
@@ -239,7 +244,7 @@ impl InlineContext {
             recursion_level: 0,
             entry_point,
             call_stack: CallStack::new(),
-            no_predicates_is_entry_point,
+            inline_no_predicates_functions,
             recursive_functions,
         }
     }
@@ -470,19 +475,37 @@ impl<'function> PerFunctionContext<'function> {
     /// Inline each instruction in the given block into the function being inlined into.
     /// This may recurse if it finds another function to inline if a call instruction is within this block.
     fn inline_block_instructions(&mut self, ssa: &Ssa, block_id: BasicBlockId) {
+        let mut side_effects_enabled: Option<ValueId> = None;
+
         let block = &self.source_function.dfg[block_id];
         for id in block.instructions() {
             match &self.source_function.dfg[*id] {
                 Instruction::Call { func, arguments } => match self.get_function(*func) {
                     Some(func_id) => {
                         if self.should_inline_call(ssa, func_id) {
                             self.inline_function(ssa, *id, func_id, arguments);
+
+                            // This is only relevant during handling functions with `InlineType::NoPredicates` as these
+                            // can pollute the function they're being inlined into with `Instruction::EnabledSideEffects`,
+                            // resulting in predicates not being applied properly.
+                            //
+                            // Note that this doesn't cover the case in which there exists an `Instruction::EnabledSideEffects`
+                            // within the function being inlined whilst the source function has not encountered one yet.
+                            // In practice this isn't an issue as the last `Instruction::EnabledSideEffects` in the
+                            // function being inlined will be to turn off predicates rather than to create one.
+                            if let Some(condition) = side_effects_enabled {
+                                self.context.builder.insert_enable_side_effects_if(condition);
+                            }
                         } else {
                             self.push_instruction(*id);
                         }
                     }
                     None => self.push_instruction(*id),
                 },
+                Instruction::EnableSideEffects { condition } => {
+                    side_effects_enabled = Some(self.translate_value(*condition));
+                    self.push_instruction(*id);
+                }
                 _ => self.push_instruction(*id),
             }
         }
@@ -495,10 +518,10 @@ impl<'function> PerFunctionContext<'function> {
             // If the called function is acir, we inline if it's not an entry point
 
             // If we have not already finished the flattening pass, functions marked
-            // to not have predicates should be marked as entry points.
-            let no_predicates_is_entry_point =
-                self.context.no_predicates_is_entry_point && function.is_no_predicates();
-            !inline_type.is_entry_point() && !no_predicates_is_entry_point
+            // to not have predicates should be preserved.
+            let preserve_function =
+                !self.context.inline_no_predicates_functions && function.is_no_predicates();
+            !inline_type.is_entry_point() && !preserve_function
         } else {
             // If the called function is brillig, we inline only if it's into brillig and the function is not recursive
             ssa.functions[&self.context.entry_point].runtime() == RuntimeType::Brillig

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

@@ -7,7 +7,6 @@ mod array_set;
 mod as_slice_length;
 mod assert_constant;
 mod bubble_up_constrains;
-mod check_for_underconstrained_values;
 mod constant_folding;
 mod defunctionalize;
 mod die;