From 53f16c7fe75da04c54517b3d3199094b15195ce4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 27 Nov 2024 22:50:59 +0000 Subject: [PATCH] feat(ssa): Deduplicate intrinsics with predicates (#6615) --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 53 +++++++++++++++++-- .../src/ssa/opt/constant_folding.rs | 53 ++++++++++++++++++- 2 files changed, 99 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 9958aadd44..b48c755dbe 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -45,8 +45,7 @@ pub(crate) type InstructionId = Id; /// - Opcodes which the IR knows the target machine has /// special support for. (LowLevel) /// - Opcodes which have no function definition in the -/// source code and must be processed by the IR. An example -/// of this is println. +/// source code and must be processed by the IR. #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub(crate) enum Intrinsic { ArrayLen, @@ -112,6 +111,9 @@ impl Intrinsic { /// Returns whether the `Intrinsic` has side effects. /// /// If there are no side effects then the `Intrinsic` can be removed if the result is unused. + /// + /// An example of a side effect is increasing the reference count of an array, but functions + /// which can fail due to implicit constraints are also considered to have a side effect. pub(crate) fn has_side_effects(&self) -> bool { match self { Intrinsic::AssertConstant @@ -152,6 +154,39 @@ impl Intrinsic { } } + /// Intrinsics which only have a side effect due to the chance that + /// they can fail a constraint can be deduplicated. + pub(crate) fn can_be_deduplicated(&self, deduplicate_with_predicate: bool) -> bool { + match self { + // These apply a constraint in the form of ACIR opcodes, but they can be deduplicated + // if the inputs are the same. If they depend on a side effect variable (e.g. because + // they were in an if-then-else) then `handle_instruction_side_effects` in `flatten_cfg` + // will have attached the condition variable to their inputs directly, so they don't + // directly depend on the corresponding `enable_side_effect` instruction any more. + // However, to conform with the expectations of `Instruction::can_be_deduplicated` and + // `constant_folding` we only use this information if the caller shows interest in it. + Intrinsic::ToBits(_) + | Intrinsic::ToRadix(_) + | Intrinsic::BlackBox( + BlackBoxFunc::MultiScalarMul + | BlackBoxFunc::EmbeddedCurveAdd + | BlackBoxFunc::RecursiveAggregation, + ) => deduplicate_with_predicate, + + // Operations that remove items from a slice don't modify the slice, they just assert it's non-empty. + Intrinsic::SlicePopBack | Intrinsic::SlicePopFront | Intrinsic::SliceRemove => { + deduplicate_with_predicate + } + + Intrinsic::AssertConstant + | Intrinsic::StaticAssert + | Intrinsic::ApplyRangeConstraint + | Intrinsic::AsWitness => deduplicate_with_predicate, + + _ => !self.has_side_effects(), + } + } + /// Lookup an Intrinsic by name and return it if found. /// If there is no such intrinsic by that name, None is returned. pub(crate) fn lookup(name: &str) -> Option { @@ -245,7 +280,7 @@ pub(crate) enum Instruction { /// - `code1` will have side effects iff `condition1` evaluates to `true` /// /// This instruction is only emitted after the cfg flattening pass, and is used to annotate - /// instruction regions with an condition that corresponds to their position in the CFG's + /// instruction regions with a condition that corresponds to their position in the CFG's /// if-branching structure. EnableSideEffectsIf { condition: ValueId }, @@ -331,6 +366,11 @@ impl Instruction { /// If `deduplicate_with_predicate` is set, we assume we're deduplicating with the instruction /// and its predicate, rather than just the instruction. Setting this means instructions that /// rely on predicates can be deduplicated as well. + /// + /// Some instructions get the predicate attached to their inputs by `handle_instruction_side_effects` in `flatten_cfg`. + /// These can be deduplicated because they implicitly depend on the predicate, not only when the caller uses the + /// predicate variable as a key to cache results. However, to avoid tight coupling between passes, we make the deduplication + /// conditional on whether the caller wants the predicate to be taken into account or not. pub(crate) fn can_be_deduplicated( &self, dfg: &DataFlowGraph, @@ -348,7 +388,9 @@ impl Instruction { | DecrementRc { .. } => false, Call { func, .. } => match dfg[*func] { - Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), + Value::Intrinsic(intrinsic) => { + intrinsic.can_be_deduplicated(deduplicate_with_predicate) + } _ => false, }, @@ -421,6 +463,7 @@ impl Instruction { // Explicitly allows removal of unused ec operations, even if they can fail Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) | Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => true, + Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), // All foreign functions are treated as having side effects. @@ -436,7 +479,7 @@ impl Instruction { } } - /// If true the instruction will depends on enable_side_effects context during acir-gen + /// If true the instruction will depend on `enable_side_effects` context during acir-gen. pub(crate) fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool { match self { Instruction::Binary(binary) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 019bace33a..9668380404 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -6,7 +6,7 @@ //! by the [`DataFlowGraph`] automatically as new instructions are pushed. //! - Check whether any input values have been constrained to be equal to a value of a simpler form //! by a [constrain instruction][Instruction::Constrain]. If so, replace the input value with the simpler form. -//! - Check whether the instruction [can_be_replaced][Instruction::can_be_replaced()] +//! - Check whether the instruction [can_be_deduplicated][Instruction::can_be_deduplicated()] //! by duplicate instruction earlier in the same block. //! //! These operations are done in parallel so that they can each benefit from each other @@ -54,6 +54,9 @@ use fxhash::FxHashMap as HashMap; impl Ssa { /// Performs constant folding on each instruction. /// + /// It will not look at constraints to inform simplifications + /// based on the stated equivalence of two instructions. + /// /// See [`constant_folding`][self] module for more information. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants(mut self) -> Ssa { @@ -188,9 +191,12 @@ pub(crate) struct BrilligInfo<'a> { brillig_functions: &'a BTreeMap, } -/// HashMap from (Instruction, side_effects_enabled_var) to the results of the instruction. +/// HashMap from `(Instruction, side_effects_enabled_var)` to the results of the instruction. /// Stored as a two-level map to avoid cloning Instructions during the `.get` call. /// +/// The `side_effects_enabled_var` is optional because we only use them when `Instruction::requires_acir_gen_predicate` +/// is true _and_ the constraint information is also taken into account. +/// /// In addition to each result, the original BasicBlockId is stored as well. This allows us /// to deduplicate instructions across blocks as long as the new block dominates the original. type InstructionResultCache = HashMap, ResultCache>>; @@ -223,6 +229,7 @@ impl<'brillig> Context<'brillig> { fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) { let instructions = function.dfg[block].take_instructions(); + // Default side effect condition variable with an enabled state. let mut side_effects_enabled_var = function.dfg.make_constant(FieldElement::one(), Type::bool()); @@ -403,6 +410,7 @@ impl<'brillig> Context<'brillig> { // If the instruction doesn't have side-effects and if it won't interact with enable_side_effects during acir_gen, // we cache the results so we can reuse them if the same instruction appears again later in the block. + // Others have side effects representing failure, which are implicit in the ACIR code and can also be deduplicated. if instruction.can_be_deduplicated(dfg, self.use_constraint_info) { let use_predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); @@ -417,6 +425,8 @@ impl<'brillig> Context<'brillig> { } } + /// Get the simplification mapping from complex to simpler instructions, + /// which all depend on the same side effect condition variable. fn get_constraint_map( &mut self, side_effects_enabled_var: ValueId, @@ -1341,4 +1351,43 @@ mod test { let ssa = ssa.fold_constants_with_brillig(&brillig); assert_normalized_ssa_equals(ssa, expected); } + + #[test] + fn deduplicates_side_effecting_intrinsics() { + let src = " + // After EnableSideEffectsIf removal: + acir(inline) fn main f0 { + b0(v0: Field, v1: Field, v2: u1): + v4 = call is_unconstrained() -> u1 + v7 = call to_be_radix(v0, u32 256) -> [u8; 1] // `a.to_be_radix(256)`; + inc_rc v7 + v8 = call to_be_radix(v0, u32 256) -> [u8; 1] // duplicate load of `a` + inc_rc v8 + v9 = cast v2 as Field // `if c { a.to_be_radix(256) }` + v10 = mul v0, v9 // attaching `c` to `a` + v11 = call to_be_radix(v10, u32 256) -> [u8; 1] // calling `to_radix(c * a)` + inc_rc v11 + enable_side_effects v2 // side effect var for `c` shifted down by removal + return + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let expected = " + acir(inline) fn main f0 { + b0(v0: Field, v1: Field, v2: u1): + v4 = call is_unconstrained() -> u1 + v7 = call to_be_radix(v0, u32 256) -> [u8; 1] + inc_rc v7 + inc_rc v7 + v8 = cast v2 as Field + v9 = mul v0, v8 + v10 = call to_be_radix(v9, u32 256) -> [u8; 1] + inc_rc v10 + enable_side_effects v2 + return + } + "; + let ssa = ssa.fold_constants_using_constraints(); + assert_normalized_ssa_equals(ssa, expected); + } }