feat: add remove_enable_side_effects SSA pass (#4224)

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

The `EnableSideEffects` instruction can hamper dead instruction
elimination in the case where it's not covering any instructions which
have side effects (and so are affected by the `EnableSideEffects`
instruction). In this case we cannot perform any DIE on instructions
used to calculate the predicate as the compiler sees their results as
being used.

This PR adds a new SSA pass which delays inserting `EnableSideEffects`
instructions until the next instruction which would be affected by them.
This means that if we hit another `EnableSideEffects` instruction first
then we can omit the previous one, unlocking more DIE opportunities.


## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: jfecher <[email protected]>

Author: TomAFrench

compiler/noirc_evaluator/src/ssa.rs

@@ -62,6 +62,7 @@ pub(crate) fn optimize_into_acir(
         // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores
         .run_pass(Ssa::mem2reg, "After Mem2Reg:")
         .run_pass(Ssa::fold_constants, "After Constant Folding:")
+        .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:")
         .run_pass(Ssa::fold_constants_using_constraints, "After Constraint Folding:")
         .run_pass(Ssa::dead_instruction_elimination, "After Dead Instruction Elimination:")
         .run_pass(Ssa::array_set_optimization, "After Array Set Optimizations:")

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

@@ -15,5 +15,6 @@ mod inlining;
 mod mem2reg;
 mod rc;
 mod remove_bit_shifts;
+mod remove_enable_side_effects;
 mod simplify_cfg;
 mod unrolling;

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

@@ -0,0 +1,167 @@
+//! The goal of the "remove enable side effects" optimization pass is to delay any [Instruction::EnableSideEffects]
+//! instructions such that they cover the minimum number of instructions possible.
+//!
+//! The pass works as follows:
+//! - Insert instructions until an [Instruction::EnableSideEffects] is encountered, save this [InstructionId].
+//! - Continue inserting instructions until either
+//!     - Another [Instruction::EnableSideEffects] is encountered, if so then drop the previous [InstructionId] in favour
+//!       of this one.
+//!     - An [Instruction] with side-effects is encountered, if so then insert the currently saved [Instruction::EnableSideEffects]
+//!       before the [Instruction]. Continue inserting instructions until the next [Instruction::EnableSideEffects] is encountered.
+use std::collections::HashSet;
+
+use acvm::FieldElement;
+
+use crate::ssa::{
+    ir::{
+        basic_block::BasicBlockId,
+        dfg::DataFlowGraph,
+        function::Function,
+        instruction::{BinaryOp, Instruction, Intrinsic},
+        value::Value,
+    },
+    ssa_gen::Ssa,
+};
+
+impl Ssa {
+    /// See [`remove_enable_side_effects`][self] module for more information.
+    #[tracing::instrument(level = "trace", skip(self))]
+    pub(crate) fn remove_enable_side_effects(mut self) -> Ssa {
+        for function in self.functions.values_mut() {
+            remove_enable_side_effects(function);
+        }
+        self
+    }
+}
+
+fn remove_enable_side_effects(function: &mut Function) {
+    let mut context = Context::default();
+    context.block_queue.push(function.entry_block());
+
+    while let Some(block) = context.block_queue.pop() {
+        if context.visited_blocks.contains(&block) {
+            continue;
+        }
+
+        context.visited_blocks.insert(block);
+        context.remove_enable_side_effects_in_block(function, block);
+    }
+}
+
+#[derive(Default)]
+struct Context {
+    visited_blocks: HashSet<BasicBlockId>,
+    block_queue: Vec<BasicBlockId>,
+}
+
+impl Context {
+    fn remove_enable_side_effects_in_block(
+        &mut self,
+        function: &mut Function,
+        block: BasicBlockId,
+    ) {
+        let instructions = function.dfg[block].take_instructions();
+
+        let mut last_side_effects_enabled_instruction = None;
+
+        let mut new_instructions = Vec::with_capacity(instructions.len());
+        for instruction_id in instructions {
+            let instruction = &function.dfg[instruction_id];
+
+            // If we run into another `Instruction::EnableSideEffects` before encountering any
+            // instructions with side effects then we can drop the instruction we're holding and
+            // continue with the new `Instruction::EnableSideEffects`.
+            if let Instruction::EnableSideEffects { condition } = instruction {
+                // If we're seeing an `enable_side_effects u1 1` then we want to insert it immediately.
+                // This is because we want to maximize the effect it will have.
+                if function
+                    .dfg
+                    .get_numeric_constant(*condition)
+                    .map_or(false, |condition| condition.is_one())
+                {
+                    new_instructions.push(instruction_id);
+                    last_side_effects_enabled_instruction = None;
+                    continue;
+                }
+
+                last_side_effects_enabled_instruction = Some(instruction_id);
+                continue;
+            }
+
+            // If we hit an instruction which is affected by the side effects var then we must insert the
+            // `Instruction::EnableSideEffects` before we insert this new instruction.
+            if Self::responds_to_side_effects_var(&function.dfg, instruction) {
+                if let Some(enable_side_effects_instruction_id) =
+                    last_side_effects_enabled_instruction.take()
+                {
+                    new_instructions.push(enable_side_effects_instruction_id);
+                }
+            }
+            new_instructions.push(instruction_id);
+        }
+
+        *function.dfg[block].instructions_mut() = new_instructions;
+
+        self.block_queue.extend(function.dfg[block].successors());
+    }
+
+    fn responds_to_side_effects_var(dfg: &DataFlowGraph, instruction: &Instruction) -> bool {
+        use Instruction::*;
+        match instruction {
+            Binary(binary) => {
+                if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) {
+                    if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) {
+                        rhs == FieldElement::zero()
+                    } else {
+                        true
+                    }
+                } else {
+                    false
+                }
+            }
+
+            Cast(_, _)
+            | Not(_)
+            | Truncate { .. }
+            | Constrain(..)
+            | RangeCheck { .. }
+            | IncrementRc { .. }
+            | DecrementRc { .. } => false,
+
+            EnableSideEffects { .. }
+            | ArrayGet { .. }
+            | ArraySet { .. }
+            | Allocate
+            | Store { .. }
+            | Load { .. } => true,
+
+            // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is.
+            Call { func, .. } => match dfg[*func] {
+                Value::Intrinsic(intrinsic) => match intrinsic {
+                    Intrinsic::SlicePushBack
+                    | Intrinsic::SlicePushFront
+                    | Intrinsic::SlicePopBack
+                    | Intrinsic::SlicePopFront
+                    | Intrinsic::SliceInsert
+                    | Intrinsic::SliceRemove => true,
+
+                    Intrinsic::ArrayLen
+                    | Intrinsic::AssertConstant
+                    | Intrinsic::ApplyRangeConstraint
+                    | Intrinsic::StrAsBytes
+                    | Intrinsic::ToBits(_)
+                    | Intrinsic::ToRadix(_)
+                    | Intrinsic::BlackBox(_)
+                    | Intrinsic::FromField
+                    | Intrinsic::AsField
+                    | Intrinsic::AsSlice => false,
+                },
+
+                // We must assume that functions contain a side effect as we cannot inspect more deeply.
+                Value::Function(_) => true,
+
+                _ => false,
+            },
+        }
+    }
+}