From 4e4c496653e2a5b875723c02e077756781e1fb6a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 26 Sep 2024 17:48:36 +0000 Subject: [PATCH 1/5] remove useless paired rc within a block during die --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 148 +++++++++++++++++++- compiler/noirc_evaluator/src/ssa/opt/rc.rs | 32 +++-- 2 files changed, 163 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 7356998ceb8..0ee708a5dc1 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -1,7 +1,6 @@ //! Dead Instruction Elimination (DIE) pass: Removes any instruction without side-effects for //! which the results are unused. -use std::collections::HashSet; - +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use im::Vector; use noirc_errors::Location; @@ -18,6 +17,8 @@ use crate::ssa::{ ssa_gen::{Ssa, SSA_WORD_SIZE}, }; +use super::rc::{pop_rc_for, IncRc}; + impl Ssa { /// Performs Dead Instruction Elimination (DIE) to remove any instructions with /// unused results. @@ -76,6 +77,12 @@ struct Context { rc_instructions: Vec<(InstructionId, BasicBlockId)>, } +// struct IncRc { +// id: InstructionId, +// array: ValueId, +// possibly_mutated: bool, +// } + impl Context { /// Steps backwards through the instruction of the given block, amassing a set of used values /// as it goes, and at the same time marking instructions for removal if they haven't appeared @@ -105,6 +112,16 @@ 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 extra 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> = HashMap::default(); + let mut inc_rcs_to_remove = HashSet::default(); + // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. let mut possible_index_out_of_bounds_indexes = Vec::new(); @@ -131,6 +148,17 @@ impl Context { }); } } + + self.track_inc_rcs_to_remove( + *instruction_id, + function, + &mut inc_rcs, + &mut inc_rcs_to_remove, + ); + } + + for id in inc_rcs_to_remove { + self.instructions_to_remove.insert(id); } // If there are some instructions that might trigger an out of bounds error, @@ -155,6 +183,44 @@ impl Context { false } + fn track_inc_rcs_to_remove( + &self, + instruction_id: InstructionId, + function: &Function, + inc_rcs: &mut HashMap>, + inc_rcs_to_remove: &mut HashSet, + ) { + let instruction = &function.dfg[instruction_id]; + // 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, inc_rcs) { + if !inc_rc.possibly_mutated { + inc_rcs_to_remove.insert(inc_rc.id); + inc_rcs_to_remove.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 inc_rc = IncRc { id: instruction_id, array: *value, possibly_mutated: false }; + inc_rcs.entry(typ).or_default().push(inc_rc); + } + Instruction::ArraySet { array, .. } => { + let typ = function.dfg.type_of_value(*array); + if let Some(inc_rcs) = inc_rcs.get_mut(&typ) { + for inc_rc in inc_rcs { + inc_rc.possibly_mutated = true; + } + } + } + _ => {} + } + } + /// Returns true if an instruction can be removed. /// /// An instruction can be removed as long as it has no side-effects, and none of its result @@ -509,10 +575,12 @@ fn apply_side_effects( #[cfg(test)] mod test { + use std::sync::Arc; + use crate::ssa::{ function_builder::FunctionBuilder, ir::{ - instruction::{BinaryOp, Intrinsic}, + instruction::{BinaryOp, Instruction, Intrinsic}, map::Id, types::Type, }, @@ -642,4 +710,78 @@ mod test { assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); } + + #[test] + fn remove_useless_paired_rcs_even_when_used() { + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // inc_rc v0 + // v2 = array_get v0, index u32 0 + // dec_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); + let zero = builder.numeric_constant(0u128, Type::unsigned(32)); + let v1 = builder.insert_array_get(v0, zero, Type::field()); + builder.decrement_array_reference_count(v0); + builder.terminate_with_return(vec![v1]); + + 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(), 3); + + // 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 { .. })); + } + + #[test] + fn keep_paired_rcs_with_array_set() { + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // inc_rc v0 + // v2 = array_set v0, index u32 0, value u32 0 + // dec_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); + let zero = builder.numeric_constant(0u128, Type::unsigned(32)); + let v1 = builder.insert_array_set(v0, zero, zero); + builder.decrement_array_reference_count(v0); + builder.terminate_with_return(vec![v1]); + + 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(), 3); + + // 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(), 3); + } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/compiler/noirc_evaluator/src/ssa/opt/rc.rs index 1750f2d80a5..01861c249bc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::ssa::{ ir::{ @@ -38,10 +38,10 @@ struct Context { inc_rcs: HashMap>, } -struct IncRc { - id: InstructionId, - array: ValueId, - possibly_mutated: bool, +pub(crate) struct IncRc { + pub(crate) id: InstructionId, + pub(crate) array: ValueId, + pub(crate) possibly_mutated: bool, } /// This function is very simplistic for now. It takes advantage of the fact that dec_rc @@ -107,11 +107,11 @@ impl Context { /// is not possibly mutated, then we can remove them both. Returns each such pair. fn find_rcs_to_remove(&mut self, function: &Function) -> HashSet { let last_block = function.find_last_block(); - let mut to_remove = HashSet::new(); + let mut to_remove = HashSet::default(); for instruction in function.dfg[last_block].instructions() { if let Instruction::DecrementRc { value } = &function.dfg[*instruction] { - if let Some(inc_rc) = self.pop_rc_for(*value, function) { + if let Some(inc_rc) = pop_rc_for(*value, function, &mut self.inc_rcs) { if !inc_rc.possibly_mutated { to_remove.insert(inc_rc.id); to_remove.insert(*instruction); @@ -122,16 +122,20 @@ impl Context { to_remove } +} - /// Finds and pops the IncRc for the given array value if possible. - fn pop_rc_for(&mut self, value: ValueId, function: &Function) -> Option { - let typ = function.dfg.type_of_value(value); +/// Finds and pops the IncRc for the given array value if possible. +pub(crate) fn pop_rc_for( + value: ValueId, + function: &Function, + inc_rcs: &mut HashMap>, +) -> Option { + let typ = function.dfg.type_of_value(value); - let rcs = self.inc_rcs.get_mut(&typ)?; - let position = rcs.iter().position(|inc_rc| inc_rc.array == value)?; + let rcs = inc_rcs.get_mut(&typ)?; + let position = rcs.iter().position(|inc_rc| inc_rc.array == value)?; - Some(rcs.remove(position)) - } + Some(rcs.remove(position)) } fn remove_instructions(to_remove: HashSet, function: &mut Function) { From bbfdbd9c7b20b991c08d53ed2250715567ab60ad Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 26 Sep 2024 13:58:44 -0400 Subject: [PATCH 2/5] Update compiler/noirc_evaluator/src/ssa/opt/die.rs --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 0ee708a5dc1..1cb383819ce 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -77,12 +77,6 @@ struct Context { rc_instructions: Vec<(InstructionId, BasicBlockId)>, } -// struct IncRc { -// id: InstructionId, -// array: ValueId, -// possibly_mutated: bool, -// } - impl Context { /// Steps backwards through the instruction of the given block, amassing a set of used values /// as it goes, and at the same time marking instructions for removal if they haven't appeared From 5cd33a2c9adf2dfc18f98b1fb6c801336ca1e5db Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 26 Sep 2024 14:00:02 -0400 Subject: [PATCH 3/5] Update compiler/noirc_evaluator/src/ssa/opt/die.rs --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 1cb383819ce..b04055ec877 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -109,7 +109,7 @@ impl Context { // 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 extra pointless logic if there is no array set between the increment and the decrement of the reference counter. + // 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. From 466f40b6179abc1ddd0ee5d9a39dd688d30bc378 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 26 Sep 2024 18:00:52 +0000 Subject: [PATCH 4/5] IncRc -> RcInstruction --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 8 ++++---- compiler/noirc_evaluator/src/ssa/opt/rc.rs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 0ee708a5dc1..f1a54fd2a57 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -17,7 +17,7 @@ use crate::ssa::{ ssa_gen::{Ssa, SSA_WORD_SIZE}, }; -use super::rc::{pop_rc_for, IncRc}; +use super::rc::{pop_rc_for, RcInstruction}; impl Ssa { /// Performs Dead Instruction Elimination (DIE) to remove any instructions with @@ -119,7 +119,7 @@ impl Context { // 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> = HashMap::default(); + let mut inc_rcs: HashMap> = HashMap::default(); let mut inc_rcs_to_remove = HashSet::default(); // Indexes of instructions that might be out of bounds. @@ -187,7 +187,7 @@ impl Context { &self, instruction_id: InstructionId, function: &Function, - inc_rcs: &mut HashMap>, + inc_rcs: &mut HashMap>, inc_rcs_to_remove: &mut HashSet, ) { let instruction = &function.dfg[instruction_id]; @@ -206,7 +206,7 @@ impl Context { let typ = function.dfg.type_of_value(*value); // We assume arrays aren't mutated until we find an array_set - let inc_rc = IncRc { id: instruction_id, array: *value, possibly_mutated: false }; + let inc_rc = RcInstruction { id: instruction_id, array: *value, possibly_mutated: false }; inc_rcs.entry(typ).or_default().push(inc_rc); } Instruction::ArraySet { array, .. } => { diff --git a/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/compiler/noirc_evaluator/src/ssa/opt/rc.rs index 01861c249bc..c2b15298b7c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -35,10 +35,10 @@ struct Context { // // The type of the array being operated on is recorded. // If an array_set to that array type is encountered, that is also recorded. - inc_rcs: HashMap>, + inc_rcs: HashMap>, } -pub(crate) struct IncRc { +pub(crate) struct RcInstruction { pub(crate) id: InstructionId, pub(crate) array: ValueId, pub(crate) possibly_mutated: bool, @@ -80,7 +80,7 @@ impl Context { let typ = function.dfg.type_of_value(*value); // We assume arrays aren't mutated until we find an array_set - let inc_rc = IncRc { id: *instruction, array: *value, possibly_mutated: false }; + let inc_rc = RcInstruction { id: *instruction, array: *value, possibly_mutated: false }; self.inc_rcs.entry(typ).or_default().push(inc_rc); } } @@ -128,8 +128,8 @@ impl Context { pub(crate) fn pop_rc_for( value: ValueId, function: &Function, - inc_rcs: &mut HashMap>, -) -> Option { + inc_rcs: &mut HashMap>, +) -> Option { let typ = function.dfg.type_of_value(value); let rcs = inc_rcs.get_mut(&typ)?; From 2ae4966adcbe7ee68cf9ddc354ff38374efc3698 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 26 Sep 2024 18:07:44 +0000 Subject: [PATCH 5/5] fmt --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 3 ++- compiler/noirc_evaluator/src/ssa/opt/rc.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index a95605a3b29..02737f5645b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -200,7 +200,8 @@ impl Context { let typ = function.dfg.type_of_value(*value); // We assume arrays aren't mutated until we find an array_set - let inc_rc = RcInstruction { id: instruction_id, array: *value, possibly_mutated: false }; + let inc_rc = + RcInstruction { id: instruction_id, array: *value, possibly_mutated: false }; inc_rcs.entry(typ).or_default().push(inc_rc); } Instruction::ArraySet { array, .. } => { diff --git a/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/compiler/noirc_evaluator/src/ssa/opt/rc.rs index c2b15298b7c..06025fd9e8b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -80,7 +80,8 @@ impl Context { let typ = function.dfg.type_of_value(*value); // We assume arrays aren't mutated until we find an array_set - let inc_rc = RcInstruction { id: *instruction, array: *value, possibly_mutated: false }; + let inc_rc = + RcInstruction { id: *instruction, array: *value, possibly_mutated: false }; self.inc_rcs.entry(typ).or_default().push(inc_rc); } }