From db68face527f6c9d432de3ac71a7fe50364c7be6 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 9 Dec 2024 14:37:47 -0600 Subject: [PATCH 1/5] Try replace callstack with a linked list --- .../src/ssa/function_builder/mod.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 4 +- .../noirc_evaluator/src/ssa/ir/instruction.rs | 3 +- compiler/noirc_evaluator/src/ssa/ir/list.rs | 177 ++++++++++++++++++ compiler/noirc_evaluator/src/ssa/ir/mod.rs | 1 + compiler/noirc_evaluator/src/ssa/opt/die.rs | 6 +- .../src/ssa/opt/flatten_cfg.rs | 2 +- 7 files changed, 185 insertions(+), 10 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/ir/list.rs diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 0ae61404442..5063e06339c 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -195,7 +195,7 @@ impl FunctionBuilder { } pub(crate) fn set_location(&mut self, location: Location) -> &mut FunctionBuilder { - self.call_stack = im::Vector::unit(location); + self.call_stack = CallStack::unit(location); self } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 827944e22d1..2aa7997b179 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -97,7 +97,7 @@ pub(crate) struct DataFlowGraph { pub(crate) data_bus: DataBus, } -pub(crate) type CallStack = im::Vector; +pub(crate) type CallStack = super::list::List; impl DataFlowGraph { /// Creates a new basic block with no parameters. @@ -496,7 +496,7 @@ impl DataFlowGraph { pub(crate) fn get_value_call_stack(&self, value: ValueId) -> CallStack { match &self.values[self.resolve(value)] { Value::Instruction { instruction, .. } => self.get_call_stack(*instruction), - _ => im::Vector::new(), + _ => CallStack::new(), } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 76409f6a20a..fa618c46c9d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -1,4 +1,3 @@ -use noirc_errors::Location; use serde::{Deserialize, Serialize}; use std::hash::{Hash, Hasher}; @@ -1233,7 +1232,7 @@ impl TerminatorInstruction { } } - pub(crate) fn call_stack(&self) -> im::Vector { + pub(crate) fn call_stack(&self) -> CallStack { match self { TerminatorInstruction::JmpIf { call_stack, .. } | TerminatorInstruction::Jmp { call_stack, .. } diff --git a/compiler/noirc_evaluator/src/ssa/ir/list.rs b/compiler/noirc_evaluator/src/ssa/ir/list.rs new file mode 100644 index 00000000000..152b9c5f1e4 --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/ir/list.rs @@ -0,0 +1,177 @@ +use serde::{Deserialize, Serialize}; +use std::sync::Arc; + +/// A shared linked list type intended to be cloned +#[derive(Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub struct List { + head: Arc>, + len: usize, +} + +#[derive(Default, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +enum Node { + #[default] + Nil, + Cons(T, Arc>), +} + +impl Default for List { + fn default() -> Self { + List { head: Arc::new(Node::Nil), len: 0 } + } +} + +impl List { + pub fn new() -> Self { + Self::default() + } + + pub fn push_back(&mut self, value: T) { + self.len += 1; + self.head = Arc::new(Node::Cons(value, self.head.clone())); + } + + pub fn iter(&self) -> Iter { + Iter { head: &self.head, len: self.len } + } + + pub fn clear(&mut self) { + *self = Self::default(); + } + + pub fn append(&mut self, other: Self) + where + T: Copy, + { + let other = other.collect::>(); + + for item in other.into_iter().rev() { + self.push_back(item); + } + } + + pub fn len(&self) -> usize { + self.len + } + + pub fn is_empty(&self) -> bool { + self.len == 0 + } + + pub fn pop_back(&mut self) -> Option + where + T: Copy, + { + match self.head.as_ref() { + Node::Nil => None, + Node::Cons(value, rest) => { + let value = *value; + self.head = rest.clone(); + self.len -= 1; + Some(value) + } + } + } + + pub fn truncate(&mut self, len: usize) + where + T: Copy, + { + if self.len > len { + for _ in 0..self.len - len { + self.pop_back(); + } + } + } + + pub fn unit(item: T) -> Self { + let mut this = Self::default(); + this.push_back(item); + this + } + + pub fn back(&self) -> Option<&T> { + match self.head.as_ref() { + Node::Nil => None, + Node::Cons(item, _) => Some(item), + } + } +} + +impl Iterator for List +where + T: Copy, +{ + type Item = T; + + fn next(&mut self) -> Option { + self.pop_back() + } +} + +pub struct Iter<'a, T> { + head: &'a Node, + len: usize, +} + +impl<'a, T> IntoIterator for &'a List { + type Item = &'a T; + + type IntoIter = Iter<'a, T>; + + fn into_iter(self) -> Self::IntoIter { + self.iter() + } +} + +impl<'a, T> Iterator for Iter<'a, T> { + type Item = &'a T; + + fn next(&mut self) -> Option { + match self.head { + Node::Nil => None, + Node::Cons(value, rest) => { + self.head = rest; + Some(value) + } + } + } + + fn size_hint(&self) -> (usize, Option) { + (0, Some(self.len)) + } +} + +impl<'a, T> ExactSizeIterator for Iter<'a, T> {} + +impl std::fmt::Debug for List +where + T: std::fmt::Debug, +{ + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "[")?; + for (i, item) in self.iter().enumerate() { + if i != 0 { + write!(f, ", ")?; + } + write!(f, "{item:?}")?; + } + write!(f, "]") + } +} + +impl std::fmt::Display for List +where + T: std::fmt::Display, +{ + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "[")?; + for (i, item) in self.iter().enumerate() { + if i != 0 { + write!(f, ", ")?; + } + write!(f, "{item}")?; + } + write!(f, "]") + } +} diff --git a/compiler/noirc_evaluator/src/ssa/ir/mod.rs b/compiler/noirc_evaluator/src/ssa/ir/mod.rs index 3ef680dda0f..89ba22e8b79 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/mod.rs @@ -5,6 +5,7 @@ pub(crate) mod dom; pub(crate) mod function; pub(crate) mod function_inserter; pub(crate) mod instruction; +pub mod list; pub(crate) mod map; pub(crate) mod post_order; pub(crate) mod printer; diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index f7ac6f7b313..87f5d53921d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -1,14 +1,12 @@ //! Dead Instruction Elimination (DIE) pass: Removes any instruction without side-effects for //! which the results are unused. use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; -use im::Vector; -use noirc_errors::Location; use rayon::iter::{IntoParallelRefMutIterator, ParallelIterator}; use crate::ssa::{ ir::{ basic_block::{BasicBlock, BasicBlockId}, - dfg::DataFlowGraph, + dfg::{CallStack, DataFlowGraph}, function::Function, instruction::{BinaryOp, Instruction, InstructionId, Intrinsic}, post_order::PostOrder, @@ -484,7 +482,7 @@ fn apply_side_effects( rhs: ValueId, function: &mut Function, block_id: BasicBlockId, - call_stack: Vector, + call_stack: CallStack, ) -> (ValueId, ValueId) { // See if there's an active "enable side effects" condition let Some(condition) = side_effects_condition else { diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 3fbccf93ec9..9f7d10e966b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -340,7 +340,7 @@ impl<'f> Context<'f> { self.insert_instruction_with_typevars( Instruction::EnableSideEffectsIf { condition: one }, None, - im::Vector::new(), + CallStack::new(), ); self.push_instruction(*instruction); self.insert_current_side_effects_enabled(); From d427768f3f9bc8dce4e1d237d376e9e63180d63c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 9 Dec 2024 14:43:56 -0600 Subject: [PATCH 2/5] Fix unit tests --- compiler/noirc_evaluator/src/acir/mod.rs | 6 ++++-- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 63facac5a17..69c2b947287 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -2892,7 +2892,7 @@ mod test { brillig::Brillig, ssa::{ function_builder::FunctionBuilder, - ir::{function::FunctionId, instruction::BinaryOp, map::Id, types::Type}, + ir::{function::FunctionId, instruction::BinaryOp, map::Id, types::Type, dfg::CallStack}, }, }; @@ -2914,7 +2914,9 @@ mod test { builder.new_function("foo".into(), foo_id, inline_type); } // Set a call stack for testing whether `brillig_locations` in the `GeneratedAcir` was accurately set. - builder.set_call_stack(vector![Location::dummy(), Location::dummy()]); + let mut stack = CallStack::unit(Location::dummy()); + stack.push_back(Location::dummy()); + builder.set_call_stack(stack); let foo_v0 = builder.add_parameter(Type::field()); let foo_v1 = builder.add_parameter(Type::field()); diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index 38e6efa5b9a..2268e6b2191 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -231,7 +231,7 @@ mod tests { func.dfg[block2_id].set_terminator(TerminatorInstruction::Jmp { destination: ret_block_id, arguments: vec![], - call_stack: im::Vector::new(), + call_stack: CallStack::new(), }); func.dfg[block0_id].set_terminator(TerminatorInstruction::JmpIf { condition: cond, From d304ebc33a65c48d0b1aa12ae4fb884793c22e68 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 9 Dec 2024 14:46:45 -0600 Subject: [PATCH 3/5] Clippy --- compiler/noirc_evaluator/src/acir/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 69c2b947287..d95f08c1b93 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -2882,7 +2882,6 @@ mod test { }, FieldElement, }; - use im::vector; use noirc_errors::Location; use noirc_frontend::monomorphization::ast::InlineType; use std::collections::BTreeMap; From 7ed0bf4982b83e2c53b085fa7cbde608fefb96d9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 9 Dec 2024 14:48:37 -0600 Subject: [PATCH 4/5] Format --- compiler/noirc_evaluator/src/acir/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index d95f08c1b93..f0087c3bf44 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -2891,7 +2891,9 @@ mod test { brillig::Brillig, ssa::{ function_builder::FunctionBuilder, - ir::{function::FunctionId, instruction::BinaryOp, map::Id, types::Type, dfg::CallStack}, + ir::{ + dfg::CallStack, function::FunctionId, instruction::BinaryOp, map::Id, types::Type, + }, }, }; From 8075551ea4e14a5c349bfe4edc5ef5369dc753cb Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 10 Dec 2024 07:18:08 -0600 Subject: [PATCH 5/5] Test memory usage of location --- compiler/noirc_errors/src/position.rs | 6 ++ compiler/noirc_evaluator/src/ssa/ir/list.rs | 74 ++++++--------------- 2 files changed, 26 insertions(+), 54 deletions(-) diff --git a/compiler/noirc_errors/src/position.rs b/compiler/noirc_errors/src/position.rs index c7a64c4f422..5d9b6bd1b81 100644 --- a/compiler/noirc_errors/src/position.rs +++ b/compiler/noirc_errors/src/position.rs @@ -129,6 +129,12 @@ pub struct Location { pub file: FileId, } +impl Default for Location { + fn default() -> Self { + Self::dummy() + } +} + impl Location { pub fn new(span: Span, file: FileId) -> Self { Self { span, file } diff --git a/compiler/noirc_evaluator/src/ssa/ir/list.rs b/compiler/noirc_evaluator/src/ssa/ir/list.rs index 152b9c5f1e4..27d01394e57 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/list.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/list.rs @@ -4,20 +4,19 @@ use std::sync::Arc; /// A shared linked list type intended to be cloned #[derive(Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub struct List { - head: Arc>, - len: usize, + m: std::marker::PhantomData, } -#[derive(Default, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -enum Node { - #[default] - Nil, - Cons(T, Arc>), -} +// #[derive(Default, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +// enum Node { +// #[default] +// Nil, +// Cons(T, Arc>), +// } impl Default for List { fn default() -> Self { - List { head: Arc::new(Node::Nil), len: 0 } + List { m: std::marker::PhantomData } } } @@ -27,74 +26,48 @@ impl List { } pub fn push_back(&mut self, value: T) { - self.len += 1; - self.head = Arc::new(Node::Cons(value, self.head.clone())); } pub fn iter(&self) -> Iter { - Iter { head: &self.head, len: self.len } + Iter { m: std::marker::PhantomData } } pub fn clear(&mut self) { - *self = Self::default(); } pub fn append(&mut self, other: Self) where T: Copy, { - let other = other.collect::>(); - - for item in other.into_iter().rev() { - self.push_back(item); - } } pub fn len(&self) -> usize { - self.len + 0 } pub fn is_empty(&self) -> bool { - self.len == 0 + true } pub fn pop_back(&mut self) -> Option where - T: Copy, + T: Default, { - match self.head.as_ref() { - Node::Nil => None, - Node::Cons(value, rest) => { - let value = *value; - self.head = rest.clone(); - self.len -= 1; - Some(value) - } - } + Some(T::default()) } pub fn truncate(&mut self, len: usize) where T: Copy, { - if self.len > len { - for _ in 0..self.len - len { - self.pop_back(); - } - } } pub fn unit(item: T) -> Self { - let mut this = Self::default(); - this.push_back(item); - this + Self::default() } - pub fn back(&self) -> Option<&T> { - match self.head.as_ref() { - Node::Nil => None, - Node::Cons(item, _) => Some(item), - } + pub fn back(&self) -> Option where T: Default { + Some(T::default()) } } @@ -105,13 +78,12 @@ where type Item = T; fn next(&mut self) -> Option { - self.pop_back() + None } } pub struct Iter<'a, T> { - head: &'a Node, - len: usize, + m: std::marker::PhantomData<&'a T>, } impl<'a, T> IntoIterator for &'a List { @@ -128,17 +100,11 @@ impl<'a, T> Iterator for Iter<'a, T> { type Item = &'a T; fn next(&mut self) -> Option { - match self.head { - Node::Nil => None, - Node::Cons(value, rest) => { - self.head = rest; - Some(value) - } - } + None } fn size_hint(&self) -> (usize, Option) { - (0, Some(self.len)) + (0, Some(0)) } }