From e1ba4f82c6ad90a8f11c433b66b42a20efcea8ed Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 20 Apr 2023 10:08:11 -0400 Subject: [PATCH] chore(ssa refactor): Add DenseMap and SparseMap types (#1184) * Add DenseMap and SparseMap * Update crates/noirc_evaluator/src/ssa_refactor/ir/map.rs Co-authored-by: kevaundray * Update crates/noirc_evaluator/src/ssa_refactor/ir/map.rs Co-authored-by: kevaundray * Apply suggestions from code review Co-authored-by: kevaundray * Apply cfg test suggestion * Revert removal of Cast * Fix typo --------- Co-authored-by: kevaundray --- .../noirc_evaluator/src/ssa_refactor/dfg.rs | 65 +++---- crates/noirc_evaluator/src/ssa_refactor/ir.rs | 1 + .../src/ssa_refactor/ir/extfunc.rs | 17 +- .../src/ssa_refactor/ir/instruction.rs | 95 +++------- .../src/ssa_refactor/ir/map.rs | 167 ++++++++++++++++++ .../src/ssa_refactor/ir/types.rs | 9 +- .../src/ssa_refactor/ir/value.rs | 11 +- 7 files changed, 239 insertions(+), 126 deletions(-) create mode 100644 crates/noirc_evaluator/src/ssa_refactor/ir/map.rs diff --git a/crates/noirc_evaluator/src/ssa_refactor/dfg.rs b/crates/noirc_evaluator/src/ssa_refactor/dfg.rs index a0830b5ecc3..6dcee5212e2 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/dfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/dfg.rs @@ -1,9 +1,10 @@ use super::{ basic_block::{BasicBlock, BasicBlockId}, ir::{ - extfunc::{SigRef, Signature}, + extfunc::Signature, instruction::{Instruction, InstructionId, Instructions}, - types::Typ, + map::{Id, SparseMap}, + types::Type, value::{Value, ValueId}, }, }; @@ -11,7 +12,7 @@ use std::collections::HashMap; #[derive(Debug, Default)] /// A convenience wrapper to store `Value`s. -pub(crate) struct ValueList(Vec); +pub(crate) struct ValueList(Vec>); impl ValueList { /// Inserts an element to the back of the list and @@ -34,6 +35,7 @@ impl ValueList { &self.0 } } + #[derive(Debug, Default)] pub(crate) struct DataFlowGraph { /// All of the instructions in a function @@ -52,13 +54,13 @@ pub(crate) struct DataFlowGraph { /// Storage for all of the values defined in this /// function. - values: HashMap, + values: SparseMap, /// Function signatures of external methods - signatures: HashMap, + signatures: SparseMap, /// All blocks in a function - blocks: HashMap, + blocks: SparseMap, } impl DataFlowGraph { @@ -69,12 +71,10 @@ impl DataFlowGraph { /// Inserts a new instruction into the DFG. pub(crate) fn make_instruction(&mut self, instruction_data: Instruction) -> InstructionId { - let id = self.instructions.add_instruction(instruction_data); + let id = self.instructions.push(instruction_data); - // Create a new vector to store the potential results - // for the instruction. + // Create a new vector to store the potential results for the instruction. self.results.insert(id, Default::default()); - id } @@ -85,7 +85,7 @@ impl DataFlowGraph { pub(crate) fn make_instruction_results( &mut self, instruction_id: InstructionId, - ctrl_typevar: Typ, + ctrl_typevar: Type, ) -> usize { // Clear all of the results instructions associated with this // instruction. @@ -111,10 +111,10 @@ impl DataFlowGraph { fn instruction_result_types( &self, instruction_id: InstructionId, - ctrl_typevar: Typ, - ) -> Vec { + ctrl_typevar: Type, + ) -> Vec { // Check if it is a call instruction. If so, we don't support that yet - let ins_data = self.instructions.get_instruction(instruction_id); + let ins_data = &self.instructions[instruction_id]; match ins_data { Instruction::Call { .. } => todo!("function calls are not supported yet"), ins => ins.return_types(ctrl_typevar), @@ -122,37 +122,26 @@ impl DataFlowGraph { } /// Appends a result type to the instruction. - pub(crate) fn append_result(&mut self, instruction_id: InstructionId, typ: Typ) -> ValueId { - let next_value_id = self.next_value(); + pub(crate) fn append_result(&mut self, instruction_id: InstructionId, typ: Type) -> ValueId { + let results = self.results.get_mut(&instruction_id).unwrap(); + let expected_res_position = results.len(); - // Add value to the list of results for this instruction - let res_position = self.results.get_mut(&instruction_id).unwrap().push(next_value_id); - - self.make_value(Value::Instruction { + let value_id = self.values.push(Value::Instruction { typ, - position: res_position as u16, + position: expected_res_position as u16, instruction: instruction_id, - }) - } + }); - /// Stores a value and returns its `ValueId` reference. - fn make_value(&mut self, data: Value) -> ValueId { - let next_value = self.next_value(); - - self.values.insert(next_value, data); - - next_value - } - - /// Returns the next `ValueId` - fn next_value(&self) -> ValueId { - ValueId(self.values.len() as u32) + // Add value to the list of results for this instruction + let actual_res_position = results.push(value_id); + assert_eq!(actual_res_position, expected_res_position); + value_id } /// Returns the number of instructions /// inserted into functions. pub(crate) fn num_instructions(&self) -> usize { - self.instructions.num_instructions() + self.instructions.len() } /// Returns all of result values which are attached to this instruction. @@ -166,7 +155,7 @@ mod tests { use super::DataFlowGraph; use crate::ssa_refactor::ir::{ instruction::Instruction, - types::{NumericType, Typ}, + types::{NumericType, Type}, }; use acvm::FieldElement; @@ -177,7 +166,7 @@ mod tests { let ins_id = dfg.make_instruction(ins); let num_results = - dfg.make_instruction_results(ins_id, Typ::Numeric(NumericType::NativeField)); + dfg.make_instruction_results(ins_id, Type::Numeric(NumericType::NativeField)); let results = dfg.instruction_results(ins_id); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir.rs b/crates/noirc_evaluator/src/ssa_refactor/ir.rs index aa07393203d..bdb722cd456 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir.rs @@ -1,5 +1,6 @@ pub(crate) mod extfunc; mod function; pub(crate) mod instruction; +pub(crate) mod map; pub(crate) mod types; pub(crate) mod value; diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/extfunc.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/extfunc.rs index b0e573822cf..0ec7d6f5fc0 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/extfunc.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/extfunc.rs @@ -1,23 +1,20 @@ //! Like Crane-lift all functions outside of the current function is seen as //! external. -//! To reference external functions, one uses +//! To reference external functions, one must first import the function signature +//! into the current function's context. -use super::types::Typ; +use super::types::Type; #[derive(Debug, Default, Clone)] pub(crate) struct Signature { - pub(crate) params: Vec, - pub(crate) returns: Vec, + pub(crate) params: Vec, + pub(crate) returns: Vec, } -/// Reference to a `Signature` in a map inside of -/// a functions DFG. -#[derive(Debug, Default, Clone, Copy)] -pub(crate) struct SigRef(pub(crate) u32); #[test] fn sign_smoke() { let mut signature = Signature::default(); - signature.params.push(Typ::Numeric(super::types::NumericType::NativeField)); - signature.returns.push(Typ::Numeric(super::types::NumericType::Unsigned { bit_size: 32 })); + signature.params.push(Type::Numeric(super::types::NumericType::NativeField)); + signature.returns.push(Type::Numeric(super::types::NumericType::Unsigned { bit_size: 32 })); } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs index 04d933d8f9e..33715b67293 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs @@ -1,42 +1,18 @@ -use std::collections::HashMap; - use acvm::FieldElement; -use super::{function::FunctionId, types::Typ, value::ValueId}; +use super::{ + function::FunctionId, + map::{Id, SparseMap}, + types::Type, + value::ValueId, +}; use crate::ssa_refactor::basic_block::{BasicBlockId, BlockArguments}; -/// Map of instructions. -/// This is similar to Arena. -#[derive(Debug, Default)] -pub(crate) struct Instructions(HashMap); - -impl Instructions { - /// Adds an instruction to the map and returns a - /// reference to the instruction. - pub(crate) fn add_instruction(&mut self, ins: Instruction) -> InstructionId { - let id = InstructionId(self.0.len() as u32); - self.0.insert(id, ins); - id - } - - /// Fetch the instruction corresponding to this - /// instruction id. - /// - /// Panics if there is no such instruction, since instructions cannot be - /// deleted. - pub(crate) fn get_instruction(&self, ins_id: InstructionId) -> &Instruction { - self.0.get(&ins_id).expect("ICE: instructions cannot be deleted") - } +// Container for all Instructions, per-function +pub(crate) type Instructions = SparseMap; - /// Returns the number of instructions stored in the map. - pub(crate) fn num_instructions(&self) -> usize { - self.0.len() - } -} - -#[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)] /// Reference to an instruction -pub(crate) struct InstructionId(u32); +pub(crate) type InstructionId = Id; #[derive(Debug, PartialEq, Eq, Hash, Clone)] /// These are similar to built-ins in other languages. @@ -52,52 +28,35 @@ pub(crate) struct IntrinsicOpcodes; /// Instructions are used to perform tasks. /// The instructions that the IR is able to specify are listed below. pub(crate) enum Instruction { - // Binary Operations + /// Binary Operations like +, -, *, /, ==, != Binary(Binary), - // Unary Operations - // /// Converts `Value` into Typ - Cast(ValueId, Typ), + Cast(ValueId, Type), /// Computes a bit wise not Not(ValueId), /// Truncates `value` to `bit_size` - Truncate { - value: ValueId, - bit_size: u32, - max_bit_size: u32, - }, + Truncate { value: ValueId, bit_size: u32, max_bit_size: u32 }, /// Constrains a value to be equal to true Constrain(ValueId), /// Performs a function call with a list of its arguments. - Call { - func: FunctionId, - arguments: Vec, - }, + Call { func: FunctionId, arguments: Vec }, /// Performs a call to an intrinsic function and stores the /// results in `return_arguments`. - Intrinsic { - func: IntrinsicOpcodes, - arguments: Vec, - }, + Intrinsic { func: IntrinsicOpcodes, arguments: Vec }, /// Loads a value from memory. Load(ValueId), /// Writes a value to memory. - Store { - destination: ValueId, - value: ValueId, - }, + Store { destination: ValueId, value: ValueId }, /// Stores an Immediate value - Immediate { - value: FieldElement, - }, + Immediate { value: FieldElement }, } impl Instruction { @@ -106,7 +65,7 @@ impl Instruction { pub(crate) fn num_fixed_results(&self) -> usize { match self { Instruction::Binary(_) => 1, - Instruction::Cast(_, _) => 0, + Instruction::Cast(..) => 0, Instruction::Not(_) => 1, Instruction::Truncate { .. } => 1, Instruction::Constrain(_) => 0, @@ -125,7 +84,7 @@ impl Instruction { pub(crate) fn num_fixed_arguments(&self) -> usize { match self { Instruction::Binary(_) => 2, - Instruction::Cast(_, _) => 1, + Instruction::Cast(..) => 1, Instruction::Not(_) => 1, Instruction::Truncate { .. } => 1, Instruction::Constrain(_) => 1, @@ -141,7 +100,7 @@ impl Instruction { } /// Returns the types that this instruction will return. - pub(crate) fn return_types(&self, ctrl_typevar: Typ) -> Vec { + pub(crate) fn return_types(&self, ctrl_typevar: Type) -> Vec { match self { Instruction::Binary(_) => vec![ctrl_typevar], Instruction::Cast(_, typ) => vec![*typ], @@ -221,14 +180,16 @@ pub(crate) enum BinaryOp { #[test] fn smoke_instructions_map_duplicate() { - let ins = Instruction::Cast(ValueId(0), Typ::Unit); - let same_ins = Instruction::Cast(ValueId(0), Typ::Unit); + let id = Id::test_new(0); + + let ins = Instruction::Not(id); + let same_ins = Instruction::Not(id); let mut ins_map = Instructions::default(); // Document what happens when we insert the same instruction twice - let id = ins_map.add_instruction(ins); - let id_same_ins = ins_map.add_instruction(same_ins); + let id = ins_map.push(ins); + let id_same_ins = ins_map.push(same_ins); // The map is quite naive and does not check if the instruction has ben inserted // before. We simply assign a different Id. @@ -241,9 +202,9 @@ fn num_instructions_smoke() { let mut ins_map = Instructions::default(); for i in 0..n { - let ins = Instruction::Cast(ValueId(i as u32), Typ::Unit); - ins_map.add_instruction(ins); + let ins = Instruction::Not(Id::test_new(i)); + ins_map.push(ins); } - assert_eq!(n, ins_map.num_instructions()) + assert_eq!(n, ins_map.len()) } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/map.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/map.rs new file mode 100644 index 00000000000..6c7511b5bdd --- /dev/null +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/map.rs @@ -0,0 +1,167 @@ +use std::collections::HashMap; + +/// A unique ID corresponding to a value of type T. +/// This type can be used to retrieve a value of type T from +/// either a DenseMap or SparseMap. +/// +/// Note that there is nothing in an Id binding it to a particular +/// DenseMap or SparseMap. If an Id was created to correspond to one +/// particular map type, users need to take care not to use it with +/// another map where it will likely be invalid. +pub(crate) struct Id { + index: usize, + _marker: std::marker::PhantomData, +} + +impl Id { + /// Constructs a new Id for the given index. + /// This constructor is deliberately private to prevent + /// constructing invalid IDs. + fn new(index: usize) -> Self { + Self { index, _marker: std::marker::PhantomData } + } + + /// Creates a test Id with the given index. + /// The name of this function makes it apparent it should only + /// be used for testing. Obtaining Ids in this way should be avoided + /// as unlike DenseMap::push and SparseMap::push, the Ids created + /// here are likely invalid for any particularly map. + #[cfg(test)] + pub(crate) fn test_new(index: usize) -> Self { + Self::new(index) + } +} + +// Need to manually implement most impls on Id. +// Otherwise rust assumes that Id: Hash only if T: Hash, +// which isn't true since the T is not used internally. +impl std::hash::Hash for Id { + fn hash(&self, state: &mut H) { + self.index.hash(state); + } +} + +impl Eq for Id {} + +impl PartialEq for Id { + fn eq(&self, other: &Self) -> bool { + self.index == other.index + } +} + +impl Copy for Id {} + +impl Clone for Id { + fn clone(&self) -> Self { + Self { index: self.index, _marker: self._marker } + } +} + +impl std::fmt::Debug for Id { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Deliberately formatting as a tuple with 1 element here and omitting + // the _marker: PhantomData field which would just clutter output + f.debug_tuple("Id").field(&self.index).finish() + } +} + +/// A DenseMap is a Vec wrapper where each element corresponds +/// to a unique ID that can be used to access the element. No direct +/// access to indices is provided. Since IDs must be stable and correspond +/// to indices in the internal Vec, operations that would change element +/// ordering like pop, remove, swap_remove, etc, are not possible. +#[derive(Debug)] +pub(crate) struct DenseMap { + storage: Vec, +} + +impl DenseMap { + /// Returns the number of elements in the map. + pub(crate) fn len(&self) -> usize { + self.storage.len() + } + /// Adds an element to the map. + /// Returns the identifier/reference to that element. + pub(crate) fn push(&mut self, element: T) -> Id { + let id = Id::new(self.storage.len()); + self.storage.push(element); + id + } +} + +impl Default for DenseMap { + fn default() -> Self { + Self { storage: Vec::new() } + } +} + +impl std::ops::Index> for DenseMap { + type Output = T; + + fn index(&self, id: Id) -> &Self::Output { + &self.storage[id.index] + } +} + +impl std::ops::IndexMut> for DenseMap { + fn index_mut(&mut self, id: Id) -> &mut Self::Output { + &mut self.storage[id.index] + } +} + +/// A SparseMap is a HashMap wrapper where each element corresponds +/// to a unique ID that can be used to access the element. No direct +/// access to indices is provided. +/// +/// Unlike DenseMap, SparseMap's IDs are stored within the structure +/// and are thus stable after element removal. +/// +/// Note that unlike DenseMap, it is possible to panic when retrieving +/// an element if the element's Id has been invalidated by a previous +/// call to .remove(). +#[derive(Debug)] +pub(crate) struct SparseMap { + storage: HashMap, T>, +} + +impl SparseMap { + /// Returns the number of elements in the map. + pub(crate) fn len(&self) -> usize { + self.storage.len() + } + + /// Adds an element to the map. + /// Returns the identifier/reference to that element. + pub(crate) fn push(&mut self, element: T) -> Id { + let id = Id::new(self.storage.len()); + self.storage.insert(id, element); + id + } + + /// Remove an element from the map and return it. + /// This may return None if the element was already + /// previously removed from the map. + pub(crate) fn remove(&mut self, id: Id) -> Option { + self.storage.remove(&id) + } +} + +impl Default for SparseMap { + fn default() -> Self { + Self { storage: HashMap::new() } + } +} + +impl std::ops::Index> for SparseMap { + type Output = T; + + fn index(&self, id: Id) -> &Self::Output { + &self.storage[&id] + } +} + +impl std::ops::IndexMut> for SparseMap { + fn index_mut(&mut self, id: Id) -> &mut Self::Output { + self.storage.get_mut(&id).expect("Invalid id used in SparseMap::index_mut") + } +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs index 9cf75e5ae7f..f2797423e30 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/types.rs @@ -13,12 +13,11 @@ pub(crate) enum NumericType { NativeField, } -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] /// All types representable in the IR. -pub(crate) enum Typ { - /// Represents numeric types in the IR - /// including field elements +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub(crate) enum Type { + /// Represents numeric types in the IR, including field elements Numeric(NumericType), - /// Represents the absence of a Type. + /// The Unit type with a single value Unit, } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs index c245cf95f24..ddd00efb38f 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs @@ -1,10 +1,9 @@ -use super::{instruction::InstructionId, types::Typ}; +use super::{instruction::InstructionId, map::Id, types::Type}; -#[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)] -/// Value is the most basic type allowed in the IR. -/// Transition Note: This is similar to `NodeId` in our previous IR. -pub(crate) struct ValueId(pub(crate) u32); +pub(crate) type ValueId = Id; +/// Value is the most basic type allowed in the IR. +/// Transition Note: A Id is similar to `NodeId` in our previous IR. #[derive(Debug, PartialEq, Eq, Hash, Clone)] pub(crate) enum Value { /// This value was created due to an instruction @@ -16,5 +15,5 @@ pub(crate) enum Value { /// Example, if you add two numbers together, then the resulting /// value would have position `0`, the typ would be the type /// of the operands, and the instruction would map to an add instruction. - Instruction { typ: Typ, position: u16, instruction: InstructionId }, + Instruction { typ: Type, position: u16, instruction: InstructionId }, }