Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Change Id to use a u32 #6807

Merged
merged 2 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/acir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1880,7 +1880,7 @@ impl<'a> Context<'a> {
// This conversion is for debugging support only, to allow the
// debugging instrumentation code to work. Taking the reference
// of a function in ACIR is useless.
let id = self.acir_context.add_constant(function_id.to_usize());
let id = self.acir_context.add_constant(function_id.to_u32());
AcirValue::Var(id, AcirType::field())
}
Value::ForeignFunction(_) => unimplemented!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1614,7 +1614,7 @@ impl<'block> BrilligBlock<'block> {

self.brillig_context.const_instruction(
new_variable.extract_single_addr(),
value_id.to_usize().into(),
value_id.to_u32().into(),
);
new_variable
}
Expand Down
33 changes: 17 additions & 16 deletions compiler/noirc_evaluator/src/ssa/ir/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
collections::BTreeMap,
hash::Hash,
str::FromStr,
sync::atomic::{AtomicUsize, Ordering},
sync::atomic::{AtomicU32, Ordering},
};
use thiserror::Error;

Expand All @@ -18,22 +18,23 @@
/// another map where it will likely be invalid.
#[derive(Serialize, Deserialize)]
pub(crate) struct Id<T> {
index: usize,
index: u32,
// If we do not skip this field it will simply serialize as `"_marker":null` which is useless extra data
#[serde(skip)]
_marker: std::marker::PhantomData<T>,
}

impl<T> Id<T> {
/// Constructs a new Id for the given index.
/// This constructor is deliberately private to prevent
/// constructing invalid IDs.
pub(crate) fn new(index: usize) -> Self {
///
/// This is private so that we can guarantee ids created from this function
/// point to valid T values in their external maps.
fn new(index: u32) -> Self {
Self { index, _marker: std::marker::PhantomData }
}

/// Returns the underlying index of this Id.
pub(crate) fn to_usize(self) -> usize {
pub(crate) fn to_u32(self) -> u32 {
self.index
}

Expand All @@ -43,7 +44,7 @@
/// 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 {
pub(crate) fn test_new(index: u32) -> Self {
Self::new(index)
}
}
Expand Down Expand Up @@ -187,15 +188,15 @@
/// Adds an element to the map.
/// Returns the identifier/reference to that element.
pub(crate) fn insert(&mut self, element: T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.push(element);
id
}

/// Given the Id of the element being created, adds the element
/// returned by the given function to the map
pub(crate) fn insert_with_id(&mut self, f: impl FnOnce(Id<T>) -> T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.push(f(id));
id
}
Expand All @@ -204,7 +205,7 @@
///
/// The id-element pairs are ordered by the numeric values of the ids.
pub(crate) fn iter(&self) -> impl ExactSizeIterator<Item = (Id<T>, &T)> {
let ids_iter = (0..self.storage.len()).map(|idx| Id::new(idx));
let ids_iter = (0..self.storage.len() as u32).map(|idx| Id::new(idx));
ids_iter.zip(self.storage.iter())
}
}
Expand All @@ -219,13 +220,13 @@
type Output = T;

fn index(&self, id: Id<T>) -> &Self::Output {
&self.storage[id.index]
&self.storage[id.index as usize]
}
}

impl<T> std::ops::IndexMut<Id<T>> for DenseMap<T> {
fn index_mut(&mut self, id: Id<T>) -> &mut Self::Output {
&mut self.storage[id.index]
&mut self.storage[id.index as usize]
}
}

Expand Down Expand Up @@ -253,15 +254,15 @@
/// Adds an element to the map.
/// Returns the identifier/reference to that element.
pub(crate) fn insert(&mut self, element: T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.insert(id, element);
id
}

/// Given the Id of the element being created, adds the element
/// returned by the given function to the map
pub(crate) fn insert_with_id(&mut self, f: impl FnOnce(Id<T>) -> T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.insert(id, f(id));
id
}
Expand Down Expand Up @@ -300,7 +301,7 @@
}

/// A TwoWayMap is a map from both key to value and value to key.
/// This is accomplished by keeping the map bijective - for every

Check warning on line 304 in compiler/noirc_evaluator/src/ssa/ir/map.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (bijective)
/// value there is exactly one key and vice-versa. Any duplicate values
/// are prevented in the call to insert.
#[derive(Debug)]
Expand Down Expand Up @@ -365,15 +366,15 @@
/// This type wraps an AtomicUsize so it can safely be used across threads.
#[derive(Debug, Serialize, Deserialize)]
pub(crate) struct AtomicCounter<T> {
next: AtomicUsize,
next: AtomicU32,
_marker: std::marker::PhantomData<T>,
}

impl<T> AtomicCounter<T> {
/// Create a new counter starting after the given Id.
/// Use AtomicCounter::default() to start at zero.
pub(crate) fn starting_after(id: Id<T>) -> Self {
Self { next: AtomicUsize::new(id.index + 1), _marker: Default::default() }
Self { next: AtomicU32::new(id.index + 1), _marker: Default::default() }
}

/// Return the next fresh id
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ fn create_apply_functions(
}

fn function_id_to_field(function_id: FunctionId) -> FieldElement {
(function_id.to_usize() as u128).into()
(function_id.to_u32() as u128).into()
}

/// Creates an apply function for the given signature and variants
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/unrolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,7 @@
use super::{is_new_size_ok, BoilerplateStats, Loops};

/// Tries to unroll all loops in each SSA function once, calling the `Function` directly,
/// bypassing the iterative loop done by the SSA which does further optimisations.

Check warning on line 1015 in compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (optimisations)
///
/// If any loop cannot be unrolled, it is left as-is or in a partially unrolled state.
fn try_unroll_loops(mut ssa: Ssa) -> (Ssa, Vec<RuntimeError>) {
Expand Down Expand Up @@ -1146,7 +1146,7 @@

let refs = loop0.find_pre_header_reference_values(function, &loops.cfg).unwrap();
assert_eq!(refs.len(), 1);
assert!(refs.contains(&ValueId::new(2)));
assert!(refs.contains(&ValueId::test_new(2)));

let (loads, stores) = loop0.count_loads_and_stores(function, &refs);
assert_eq!(loads, 1);
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@ impl Translator {
// A FunctionBuilder must be created with a main Function, so here wer remove it
// from the parsed SSA to avoid adding it twice later on.
let main_function = parsed_ssa.functions.remove(0);
let main_id = FunctionId::new(0);
let main_id = FunctionId::test_new(0);
let mut builder = FunctionBuilder::new(main_function.external_name.clone(), main_id);
builder.set_runtime(main_function.runtime_type);

// Map function names to their IDs so calls can be resolved
let mut function_id_counter = 1;
let mut functions = HashMap::new();
for function in &parsed_ssa.functions {
let function_id = FunctionId::new(function_id_counter);
let function_id = FunctionId::test_new(function_id_counter);
function_id_counter += 1;

functions.insert(function.internal_name.clone(), function_id);
Expand Down
Loading