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

fix: Implement slices of structs #2150

Merged
merged 3 commits into from
Aug 3, 2023
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
34 changes: 34 additions & 0 deletions crates/nargo_cli/tests/test_data/slices/src/main.nr
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,39 @@ fn main(x : Field, y : pub Field) {
assert(removed_elem == 2);
assert(remove_slice[3] == 3);
assert(remove_slice.len() == 4);

regression_2083();
}

// Ensure that slices of struct/tuple values work.
fn regression_2083() {
let y = [(1, 2)];
let y = y.push_back((3, 4)); // [(1, 2), (3, 4)]
let y = y.push_back((5, 6)); // [(1, 2), (3, 4), (5, 6)]
assert(y[2].1 == 6);

let y = y.push_front((10, 11)); // [(10, 11), (1, 2), (3, 4), (5, 6)]
let y = y.push_front((12, 13)); // [(12, 13), (10, 11), (1, 2), (3, 4), (5, 6)]

assert(y[1].0 == 10);

let y = y.insert(1, (55, 56)); // [(12, 13), (55, 56), (10, 11), (1, 2), (3, 4), (5, 6)]
assert(y[0].1 == 13);
assert(y[1].1 == 56);
assert(y[2].0 == 10);

let (y, x) = y.remove(2); // [(12, 13), (55, 56), (1, 2), (3, 4), (5, 6)]
assert(y[2].0 == 1);
assert(x.0 == 10);
assert(x.1 == 11);

let (x, y) = y.pop_front(); // [(55, 56), (1, 2), (3, 4), (5, 6)]
assert(y[0].0 == 55);
assert(x.0 == 12);
assert(x.1 == 13);

let (y, x) = y.pop_back(); // [(55, 56), (1, 2), (3, 4)]
assert(y.len() == 3);
assert(x.0 == 5);
assert(x.1 == 6);
}
2 changes: 1 addition & 1 deletion crates/noirc_evaluator/src/ssa/ir/dfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
constants: HashMap<(FieldElement, Type), ValueId>,

/// Contains each function that has been imported into the current function.
/// Each function's Value::Function is uniqued here so any given FunctionId

Check warning on line 49 in crates/noirc_evaluator/src/ssa/ir/dfg.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (uniqued)
/// will always have the same ValueId within this function.
functions: HashMap<FunctionId, ValueId>,

Expand All @@ -56,7 +56,7 @@
intrinsics: HashMap<Intrinsic, ValueId>,

/// Contains each foreign function that has been imported into the current function.
/// This map is used to ensure that the ValueId for any given foreign functôn is always

Check warning on line 59 in crates/noirc_evaluator/src/ssa/ir/dfg.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (functôn)
/// represented by only 1 ValueId within this function.
foreign_functions: HashMap<String, ValueId>,

Expand Down Expand Up @@ -369,7 +369,7 @@
/// Otherwise, this returns None.
pub(crate) fn get_array_constant(&self, value: ValueId) -> Option<(im::Vector<ValueId>, Type)> {
match &self.values[self.resolve(value)] {
// Vectors are shared, so cloning them is cheap
// Arrays are shared, so cloning them is cheap
Value::Array { array, typ } => Some((array.clone(), typ.clone())),
_ => None,
}
Expand Down
96 changes: 65 additions & 31 deletions crates/noirc_evaluator/src/ssa/ir/instruction/call.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::rc::Rc;
use std::{collections::VecDeque, rc::Rc};

use acvm::{acir::BlackBoxFunc, BlackBoxResolutionError, FieldElement};
use iter_extended::vecmap;
Expand Down Expand Up @@ -53,22 +53,22 @@ pub(super) fn simplify_call(
}
Intrinsic::ArrayLen => {
let slice = dfg.get_array_constant(arguments[0]);
if let Some((slice, _)) = slice {
SimplifyResult::SimplifiedTo(
dfg.make_constant((slice.len() as u128).into(), Type::field()),
)
if let Some((slice, typ)) = slice {
let length = FieldElement::from((slice.len() / typ.element_size()) as u128);
SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field()))
} else if let Some(length) = dfg.try_get_array_length(arguments[0]) {
SimplifyResult::SimplifiedTo(
dfg.make_constant((length as u128).into(), Type::field()),
)
let length = FieldElement::from(length as u128);
SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field()))
} else {
SimplifyResult::None
}
}
Intrinsic::SlicePushBack => {
let slice = dfg.get_array_constant(arguments[0]);
if let (Some((mut slice, element_type)), elem) = (slice, arguments[1]) {
slice.push_back(elem);
if let Some((mut slice, element_type)) = slice {
for elem in &arguments[1..] {
slice.push_back(*elem);
}
let new_slice = dfg.make_array(slice, element_type);
SimplifyResult::SimplifiedTo(new_slice)
} else {
Expand All @@ -77,8 +77,10 @@ pub(super) fn simplify_call(
}
Intrinsic::SlicePushFront => {
let slice = dfg.get_array_constant(arguments[0]);
if let (Some((mut slice, element_type)), elem) = (slice, arguments[1]) {
slice.push_front(elem);
if let Some((mut slice, element_type)) = slice {
for elem in arguments[1..].iter().rev() {
slice.push_front(*elem);
}
let new_slice = dfg.make_array(slice, element_type);
SimplifyResult::SimplifiedTo(new_slice)
} else {
Expand All @@ -87,34 +89,58 @@ pub(super) fn simplify_call(
}
Intrinsic::SlicePopBack => {
let slice = dfg.get_array_constant(arguments[0]);
if let Some((mut slice, element_type)) = slice {
let elem =
slice.pop_back().expect("There are no elements in this slice to be removed");
let new_slice = dfg.make_array(slice, element_type);
SimplifyResult::SimplifiedToMultiple(vec![new_slice, elem])
if let Some((mut slice, typ)) = slice {
let element_count = typ.element_size();
let mut results = VecDeque::with_capacity(element_count + 1);

// We must pop multiple elements in the case of a slice of tuples
for _ in 0..element_count {
let elem = slice
.pop_back()
.expect("There are no elements in this slice to be removed");
results.push_front(elem);
}

let new_slice = dfg.make_array(slice, typ);
results.push_front(new_slice);

jfecher marked this conversation as resolved.
Show resolved Hide resolved
SimplifyResult::SimplifiedToMultiple(results.into())
} else {
SimplifyResult::None
}
}
Intrinsic::SlicePopFront => {
let slice = dfg.get_array_constant(arguments[0]);
if let Some((mut slice, element_type)) = slice {
let elem =
slice.pop_front().expect("There are no elements in this slice to be removed");
let new_slice = dfg.make_array(slice, element_type);
SimplifyResult::SimplifiedToMultiple(vec![elem, new_slice])
if let Some((mut slice, typ)) = slice {
let element_count = typ.element_size();

// We must pop multiple elements in the case of a slice of tuples
let mut results = vecmap(0..element_count, |_| {
slice.pop_front().expect("There are no elements in this slice to be removed")
});

let new_slice = dfg.make_array(slice, typ);

// The slice is the last item returned for pop_front
results.push(new_slice);
SimplifyResult::SimplifiedToMultiple(results)
} else {
SimplifyResult::None
}
}
Intrinsic::SliceInsert => {
let slice = dfg.get_array_constant(arguments[0]);
let index = dfg.get_numeric_constant(arguments[1]);
if let (Some((mut slice, element_type)), Some(index), value) =
(slice, index, arguments[2])
{
slice.insert(index.to_u128() as usize, value);
let new_slice = dfg.make_array(slice, element_type);
if let (Some((mut slice, typ)), Some(index)) = (slice, index) {
let elements = &arguments[2..];
let mut index = index.to_u128() as usize * elements.len();

for elem in &arguments[2..] {
slice.insert(index, *elem);
index += 1;
}

let new_slice = dfg.make_array(slice, typ);
SimplifyResult::SimplifiedTo(new_slice)
} else {
SimplifyResult::None
Expand All @@ -123,10 +149,18 @@ pub(super) fn simplify_call(
Intrinsic::SliceRemove => {
let slice = dfg.get_array_constant(arguments[0]);
let index = dfg.get_numeric_constant(arguments[1]);
if let (Some((mut slice, element_type)), Some(index)) = (slice, index) {
let removed_elem = slice.remove(index.to_u128() as usize);
let new_slice = dfg.make_array(slice, element_type);
SimplifyResult::SimplifiedToMultiple(vec![new_slice, removed_elem])
if let (Some((mut slice, typ)), Some(index)) = (slice, index) {
let element_count = typ.element_size();
let mut results = Vec::with_capacity(element_count + 1);
let index = index.to_u128() as usize * element_count;

for _ in 0..element_count {
results.push(slice.remove(index));
}

let new_slice = dfg.make_array(slice, typ);
results.insert(0, new_slice);
SimplifyResult::SimplifiedToMultiple(results)
jfecher marked this conversation as resolved.
Show resolved Hide resolved
} else {
SimplifyResult::None
}
Expand Down
11 changes: 11 additions & 0 deletions crates/noirc_evaluator/src/ssa/ir/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,17 @@ impl Type {
pub(crate) fn field() -> Type {
Type::Numeric(NumericType::NativeField)
}

/// Returns the size of the element type for this array/slice.
/// The size of a type is defined as representing how many Fields are needed
/// to represent the type. This is 1 for every primitive type, and is the number of fields
/// for any flattened tuple type.
pub(crate) fn element_size(&self) -> usize {
jfecher marked this conversation as resolved.
Show resolved Hide resolved
match self {
Type::Array(elements, _) | Type::Slice(elements) => elements.len(),
other => panic!("element_size: Expected array or slice, found {other}"),
}
}
}

/// Composite Types are essentially flattened struct or tuple types.
Expand Down
6 changes: 2 additions & 4 deletions crates/noirc_evaluator/src/ssa/ssa_gen/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -641,10 +641,8 @@
}

fn element_size(&self, array: ValueId) -> FieldElement {
match self.builder.type_of_value(array) {
Type::Array(elements, _) | Type::Slice(elements) => (elements.len() as u128).into(),
t => panic!("Uncaught type error: tried to take element size of non-array type {t}"),
}
let size = self.builder.type_of_value(array).element_size();
FieldElement::from(size as u128)
}

/// Given an lhs containing only references, create a store instruction to store each value of
Expand Down Expand Up @@ -791,7 +789,7 @@
/// and return this new id.
pub(super) fn get_or_queue_function(&self, id: ast::FuncId) -> IrFunctionId {
// Start a new block to guarantee the destructor for the map lock is released
// before map needs to be aquired again in self.functions.write() below

Check warning on line 792 in crates/noirc_evaluator/src/ssa/ssa_gen/context.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (aquired)
{
let map = self.functions.read().expect("Failed to read self.functions");
if let Some(existing_id) = map.get(&id) {
Expand Down