From 4eb0288019094724daf2dd56c4ef2db7960511b2 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 2 Jan 2024 18:09:03 +0000 Subject: [PATCH 1/8] feat: add `remove_enable_side_effects` SSA pass --- compiler/noirc_evaluator/src/ssa.rs | 2 + .../noirc_evaluator/src/ssa/ir/instruction.rs | 1 + compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + .../src/ssa/opt/remove_enable_side_effects.rs | 159 ++++++++++++++++++ 4 files changed, 163 insertions(+) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index a45cf3af608..148d8f507ec 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -60,7 +60,9 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores .run_pass(Ssa::mem2reg, "After Mem2Reg:") + .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:") .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:") diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 2b23cc1c1e8..a7b326729be 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -287,6 +287,7 @@ impl Instruction { false } } + Cast(_, _) | Not(_) | Truncate { .. } diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 479010b1ed8..71fe9d5494e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -14,5 +14,6 @@ mod inlining; mod mem2reg; mod rc; mod remove_bit_shifts; +mod remove_enable_side_effects; mod simplify_cfg; mod unrolling; diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs new file mode 100644 index 00000000000..5fee50c22c2 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -0,0 +1,159 @@ +//! 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 crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + dfg::DataFlowGraph, + function::Function, + instruction::{BinaryOp, Instruction, InstructionId, 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, + block_queue: Vec, +} + +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: Option = 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) { + dfg.get_numeric_constant(binary.rhs).is_none() + } else { + false + } + } + + Cast(_, _) + | Not(_) + | Truncate { .. } + | Constrain(..) + | RangeCheck { .. } + | IncrementRc { .. } => false, + + EnableSideEffects { .. } | ArrayGet { .. } | ArraySet { .. } | Allocate => 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, + }, + + Store { .. } | Load { .. } => { + unreachable!("Instructions of type {instruction:?} expected to be removed") + } + } + } +} From 20d7b8ae2f5c26718f2cc4d15b3fa70731ab829b Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 3 Apr 2024 23:08:54 +0100 Subject: [PATCH 2/8] Update compiler/noirc_evaluator/src/ssa/ir/instruction.rs --- compiler/noirc_evaluator/src/ssa/ir/instruction.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index a7b326729be..2b23cc1c1e8 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -287,7 +287,6 @@ impl Instruction { false } } - Cast(_, _) | Not(_) | Truncate { .. } From 9a5e0702276eff9eb79cca35d32268149aaccdc7 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 3 Apr 2024 23:11:00 +0100 Subject: [PATCH 3/8] chore: fix merge --- .../noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 5fee50c22c2..53131c0928b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -119,7 +119,8 @@ impl Context { | Truncate { .. } | Constrain(..) | RangeCheck { .. } - | IncrementRc { .. } => false, + | IncrementRc { .. } + | DecrementRc { .. } => false, EnableSideEffects { .. } | ArrayGet { .. } | ArraySet { .. } | Allocate => true, From 9d5b5fea73919d0dea99ced2889564276dc6a323 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 3 Apr 2024 23:15:12 +0100 Subject: [PATCH 4/8] chore: fix --- .../src/ssa/opt/remove_enable_side_effects.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 53131c0928b..9fec0202f3d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -122,7 +122,12 @@ impl Context { | IncrementRc { .. } | DecrementRc { .. } => false, - EnableSideEffects { .. } | ArrayGet { .. } | ArraySet { .. } | Allocate => true, + 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] { @@ -151,10 +156,6 @@ impl Context { _ => false, }, - - Store { .. } | Load { .. } => { - unreachable!("Instructions of type {instruction:?} expected to be removed") - } } } } From 1e96c8eb33367adb1218133bd3a0a336ec8a4d50 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 3 Apr 2024 23:36:58 +0100 Subject: [PATCH 5/8] chore: fix faulty condition --- .../src/ssa/opt/remove_enable_side_effects.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 9fec0202f3d..83f7f0aef6f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -10,6 +10,8 @@ //! 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, @@ -108,7 +110,11 @@ impl Context { match instruction { Binary(binary) => { if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) { - dfg.get_numeric_constant(binary.rhs).is_none() + if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) { + rhs == FieldElement::zero() + } else { + true + } } else { false } From 3e11a5773d40a1ccd33ddd35bdf7aaa3bc780a72 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 5 Apr 2024 19:42:53 +0100 Subject: [PATCH 6/8] Update compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs Co-authored-by: jfecher --- .../noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 83f7f0aef6f..3f31c08c3e4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -62,7 +62,7 @@ impl Context { ) { let instructions = function.dfg[block].take_instructions(); - let mut last_side_effects_enabled_instruction: Option = None; + let mut last_side_effects_enabled_instruction = None; let mut new_instructions = Vec::with_capacity(instructions.len()); for instruction_id in instructions { From fab31c523861745da84c2b2396097e98b9c62f05 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 5 Apr 2024 19:47:59 +0100 Subject: [PATCH 7/8] chore: remove second pass of `remove_enable_side_effects` --- compiler/noirc_evaluator/src/ssa.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index dbc4376168b..9ecf6960333 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -61,7 +61,6 @@ pub(crate) fn optimize_into_acir( .run_pass(Ssa::remove_bit_shifts, "After Removing Bit Shifts:") // Run mem2reg once more with the flattened CFG to catch any remaining loads/stores .run_pass(Ssa::mem2reg, "After Mem2Reg:") - .run_pass(Ssa::remove_enable_side_effects, "After EnableSideEffects removal:") .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:") From 1938860ed0f2c88f5d7007e77ade7b58e9cfed40 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Fri, 5 Apr 2024 20:02:01 +0100 Subject: [PATCH 8/8] Update compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs --- .../noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 3f31c08c3e4..8535dc2661f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -17,7 +17,7 @@ use crate::ssa::{ basic_block::BasicBlockId, dfg::DataFlowGraph, function::Function, - instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, + instruction::{BinaryOp, Instruction, Intrinsic}, value::Value, }, ssa_gen::Ssa,